webbrowser-app

Merge lp:~abreu-alexandre/webbrowser-app/intent into lp:webbrowser-app

intent
Merge into trunk

Proposed by Alexandre Abreu on 2015-01-23

Status:

Merged

Approved by:

Olivier Tilloy on 2015-02-03

Approved revision:

890

Merged at revision:

883

Proposed branch:

lp:~abreu-alexandre/webbrowser-app/intent

Merge into:

lp:webbrowser-app

Diff against target:

1080 lines (+856/-23)

13 files modified

src/app/webcontainer/CMakeLists.txt (+2/-1)
src/app/webcontainer/WebViewImplWebkit.qml (+28/-21)
src/app/webcontainer/intent-filter.cpp (+243/-0)
src/app/webcontainer/intent-filter.h (+105/-0)
src/app/webcontainer/url-pattern-utils.h (+14/-0)
src/app/webcontainer/webapp-container.cpp (+29/-0)
src/app/webcontainer/webapp-container.h (+7/-0)
src/app/webcontainer/webapp-container.qml (+70/-0)
tests/autopilot/webapp_container/tests/__init__.py (+14/-1)
tests/autopilot/webapp_container/tests/test_intent_uri_support.py (+116/-0)
tests/unittests/CMakeLists.txt (+1/-0)
tests/unittests/intent-filter/CMakeLists.txt (+9/-0)
tests/unittests/intent-filter/tst_IntentFilterTests.cpp (+218/-0)

To merge this branch:

bzr merge lp:~abreu-alexandre/webbrowser-app/intent

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-02-02
Olivier Tilloy		2015-01-23	Approve on 2015-02-02
Ted Gould		2015-01-29	Pending
Review via email: mp+247421@code.launchpad.net

Commit message

Add support for intent:// schemes in the container.

Description of the change

Add support for intent:// schemes in the container.

The current approach is to have a basic proper support for intent:// URIs in the URIHandler. Those URIs are detected and the host/uri/scheme are extracted to make up the target requested URI.

In order to add a bit of flexibility on the webapp side on how the intent should be treated, an optional javascript file (as a drop-in file) can be added to the webapp. This file should export a function such as:

(function(intent) { return intent; });

(the body of the function being obviously webapp dependant).

This function gets as a parameter an object with the following keys:

'scheme': the scheme for the intent
'host': the host (optional) for the intent
'uri': the uri for the intent

and is supposed to return an object that has the same "shape". Any failure to do so will make the webapp intent mechanism fallback on a function that acts as the identity function.

One has to add a specific argument to specific a specific custom intent filtering file:

--use-local-intent-filter[=intent-filename]

if no intent filename is specified a default one is looked up with the name:

"local-intent-filter.js"

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-23:

I’m not seeing the handleIntentUri() function being called anywhere.

And to be honest I’m not sure I understand the whole logic of this change.

Here is my understanding of how it should work: when an intent:// URL is clicked either in the browser or in a webapp, its handling will be delegated to the URL dispatcher, which will special case it to determine which app to target with it. The app will be invoked with the intent:// URL unchanged, so in the case of a webapp that registered for it, it needs to transform this URL to a valid http:// one.

So there are two places in the webapp-container’s code that need to handle such a transformation:
- if the app was not previously running, it will be invoked with the intent:// URL as a parameter, the transformation needs to happen before setting the URL on the webview
- if the app was already running, the UriHandler.onOpened handler needs to do the transformation

From a quick glance at the code, I don’t see this happening. Can you shed a light on this please?

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-23:

OK, now that looks better (my comment went in just after your change).

I’m wondering, why does the function receive an object that has 'scheme', 'host' and 'uri' as keys?
Wouldn’t it be enough to pass the function a string (the full intent:// URI) and expect a string (the transformed http:// URI) in return?

Also, is it really useful to allow the custom intent file to have a different name than the default one? It seems it adds a lot of logic for no apparent gain.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-23:

If I’m not mistaken, when the custom JS file is parsed (in intent-filter.cpp), the whole of its contents are evaluated. So there’s no check on whether it contains something else than the expected function. What if the file contains several functions? Or something else?

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-23:

> OK, now that looks better (my comment went in just after your change).
>
> I’m wondering, why does the function receive an object that has 'scheme',
> 'host' and 'uri' as keys?
> Wouldn’t it be enough to pass the function a string (the full intent:// URI)
> and expect a string (the transformed http:// URI) in return?

imo no, I wrote it this way to be very strict & avoid any error that could come
from something that is user defined. I did not want to let the intent parsing
be hosted in the function itself, it would not make sense imo and be possibly
error prone.

Hence, a strict (already parsed) intent description is passed down to the function
and something very strict is expected in return, not some random data,

> Also, is it really useful to allow the custom intent file to have a different
> name than the default one? It seems it adds a lot of logic for no apparent
> gain.

I though it could be something useful, but I can remove this, I am not strongly
opinionated about it :)

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-23:

> If I’m not mistaken, when the custom JS file is parsed (in intent-filter.cpp),
> the whole of its contents are evaluated. So there’s no check on whether it
> contains something else than the expected function. What if the file contains
> several functions? Or something else?

Then the check fails and we fallback to the default filter.
Again, I tried to be strict about what is expected from the intent file (also in its
IO), one need to expose a callback object and if other functions are needed they
need to be hosted in the top level callable object,

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-23:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1386/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-23:

> > Also, is it really useful to allow the custom intent file to have a
> different
> > name than the default one? It seems it adds a lot of logic for no apparent
> > gain.
>
> I though it could be something useful, but I can remove this, I am not
> strongly
> opinionated about it :)

Yeah, I wouldn’t mind if you removed it, that would result in less code to review, and again I think it doesn’t add any real value for webapp developers anyway. Thanks!

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-24

875. By Launchpad Translations on behalf of phablet-team on 2015-01-24: Launchpad automatic translations update.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-24:

FAILED: Continuous integration, rev:876
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1390/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/972
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid/447
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/148
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/148
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/148/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/148
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/861
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/970
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/970/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17415
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-vivid/366
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/539
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/539/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1390/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-24:

> > > Also, is it really useful to allow the custom intent file to have a
> > different
> > > name than the default one? It seems it adds a lot of logic for no apparent
> > > gain.
> >
> > I though it could be something useful, but I can remove this, I am not
> > strongly
> > opinionated about it :)
>
> Yeah, I wouldn’t mind if you removed it, that would result in less code to
> review, and again I think it doesn’t add any real value for webapp developers
> anyway. Thanks!

done.

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-26

876. By Launchpad Translations on behalf of phablet-team on 2015-01-25: Launchpad automatic translations update.
877. By Launchpad Translations on behalf of phablet-team on 2015-01-26: Launchpad automatic translations update.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-26:

Can’t we get rid of the --use-local-intent-filter command-line option? I.e., make its use implicit: if there is a well-formed 'local-intent-filter.js' file, use it, otherwise use the default passthrough filter. Just like for the 'webapp-properties.json' file.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-26:

In src/app/webcontainer/CMakeLists.txt, you should add "Qml" to the list of Qt5 modules that are required (qt5_use_modules(…)).

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-26:

Cosmetics: now that the ability to provide a custom filename for the intent filter has been removed, maybe DEFAULT_LOCAL_INTENT_FILTER_FILENAME could be renamed to LOCAL_INTENT_FILTER_FILENAME ?

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-26:

Why not make the parameter of parseIntentUri(…) a QUrl ?
This would make it much easier to parse the URI, without the need for regexpes. E.g.:

QUrl url("intent://…");

    qDebug() << "URL:" << url;
    qDebug() << "scheme:" << url.scheme();
    qDebug() << "host:" << url.host();
    qDebug() << "path:" << url.path();
    qDebug() << "query:" << url.query();
    qDebug() << "fragment:" << url.fragment();

    QStringList fragments = url.fragment().split(";");
    assert(fragments.takeFirst() == "Intent");
    assert(fragments.takeLast() == "end");
    QMap<QString, QString> tokens;
    Q_FOREACH(const QString& fragment, fragments) {
        QStringList token = fragment.split("=");
        assert(token.size() == 2);
        tokens.insert(token[0], token[1]);
    }
    qDebug() << "tokens:" << tokens;

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-26:

There is a missing autopilot test case: handling of an intent:// URI for a webapp that doesn’t ship a 'local-intent-filter.js' file.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-26:

676 + def get_intent_filtered_uri(self, uri):
[…]
681 + webviewContainer.slots.handleIntentUri(uri)

This is wrong, the test case shouldn’t call a slot on the QML object: this is not testing a real-world use-case any longer. Instead, the webapp’s homepage should have an "intent://" link and the test should click that link and ensure that as a result the URL of the webview changes to the expected transformed URL.

review: Needs Fixing

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-26:

> 676 + def get_intent_filtered_uri(self, uri):
> […]
> 681 + webviewContainer.slots.handleIntentUri(uri)
>
> This is wrong, the test case shouldn’t call a slot on the QML object: this is
> not testing a real-world use-case any longer. Instead, the webapp’s homepage
> should have an "intent://" link and the test should click that link and ensure
> that as a result the URL of the webview changes to the expected transformed
> URL.

I know that this is "wrong", but I did it this way after exploring how
'intrumentable' the url-dispatcher was. And it is not at the required
level to test the whole flow ...

The way that it is it test *some* of the logic and bits of code (closer
to something at the unit level),

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-26:

> There is a missing autopilot test case: handling of an intent:// URI for a
> webapp that doesn’t ship a 'local-intent-filter.js' file.

the test_basic_intent_parsing AP test is meant to tackle that ...

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-26:

> Why not make the parameter of parseIntentUri(…) a QUrl ?
> This would make it much easier to parse the URI, without the need for
> regexpes. E.g.:
>
> QUrl url("intent://…");
>
> qDebug() << "URL:" << url;
> qDebug() << "scheme:" << url.scheme();
> qDebug() << "host:" << url.host();
> qDebug() << "path:" << url.path();
> qDebug() << "query:" << url.query();
> qDebug() << "fragment:" << url.fragment();
>
> QStringList fragments = url.fragment().split(";");
> assert(fragments.takeFirst() == "Intent");
> assert(fragments.takeLast() == "end");
> QMap<QString, QString> tokens;
> Q_FOREACH(const QString& fragment, fragments) {
> QStringList token = fragment.split("=");
> assert(token.size() == 2);
> tokens.insert(token[0], token[1]);
> }
> qDebug() << "tokens:" << tokens;

I'd agree, except that it also brings extra logic to reconstruct
the intent URI parts:
- path + query (if any),
- host (if any) and path,
etc.
plus some other things,

which in the end makes it also a bit convoluted imo,

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-26:

all other comments addressed

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-26:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1396/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Francis Ginther (fginther) wrote on 2015-01-26:

The jenkins node for the i386 and amd64 tests failed, I've restarted a new ci run.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-26:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1397/rebuild

review: Needs Fixing (continuous-integration)

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-28

878. By Launchpad Translations on behalf of phablet-team on 2015-01-27: Launchpad automatic translations update.
879. By Olivier Tilloy on 2015-01-27: Update the m.youtube.com UA override to fix video playback. Fixes: #1415107
Approved by: Alexandre Abreu
880. By CI Train Bot Account on 2015-01-27: Releasing 0.23+15.04.20150127-0ubuntu1
881. By CI Train Bot Account on 2015-01-28: Resync trunk

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-28:

> > Why not make the parameter of parseIntentUri(…) a QUrl ?
> > This would make it much easier to parse the URI, without the need for
> > regexpes. E.g.:
> >
> > QUrl url("intent://…");
> >
> > qDebug() << "URL:" << url;
> > qDebug() << "scheme:" << url.scheme();
> > qDebug() << "host:" << url.host();
> > qDebug() << "path:" << url.path();
> > qDebug() << "query:" << url.query();
> > qDebug() << "fragment:" << url.fragment();
> >
> > QStringList fragments = url.fragment().split(";");
> > assert(fragments.takeFirst() == "Intent");
> > assert(fragments.takeLast() == "end");
> > QMap<QString, QString> tokens;
> > Q_FOREACH(const QString& fragment, fragments) {
> > QStringList token = fragment.split("=");
> > assert(token.size() == 2);
> > tokens.insert(token[0], token[1]);
> > }
> > qDebug() << "tokens:" << tokens;
>
> I'd agree, except that it also brings extra logic to reconstruct
> the intent URI parts:
> - path + query (if any),
> - host (if any) and path,
> etc.
> plus some other things,
>
> which in the end makes it also a bit convoluted imo,

Fair enough (I disagree but let’s agree to disagree). A couple of comments on the logic though:

- the intentRe regexp should end with the ";end;" token
- the assignment of result.host and result.uriPath will fail if s has more than 2 items (e.g. if the path is several levels deep, like example.org/example/path/to/some/resource)

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-28:

> > 676 + def get_intent_filtered_uri(self, uri):
> > […]
> > 681 + webviewContainer.slots.handleIntentUri(uri)
> >
> > This is wrong, the test case shouldn’t call a slot on the QML object: this
> is
> > not testing a real-world use-case any longer. Instead, the webapp’s homepage
> > should have an "intent://" link and the test should click that link and
> ensure
> > that as a result the URL of the webview changes to the expected transformed
> > URL.
>
> I know that this is "wrong", but I did it this way after exploring how
> 'intrumentable' the url-dispatcher was. And it is not at the required
> level to test the whole flow ...
>
> The way that it is it test *some* of the logic and bits of code (closer
> to something at the unit level),

Can’t the 'UriHandler' global object be mocked and made to emit its "opened" signal (without the need to tinker with the URL dispatcher)?

If not, at the very least this test case should have a comment to explain what it does and why it’s not a real integration test.

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-28

882. By Alexandre Abreu on 2015-01-28: rever pot commit

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-28:

> > > Why not make the parameter of parseIntentUri(…) a QUrl ?
> > > This would make it much easier to parse the URI, without the need for
> > > regexpes. E.g.:
> > >
> > > QUrl url("intent://…");
> > >
> > > qDebug() << "URL:" << url;
> > > qDebug() << "scheme:" << url.scheme();
> > > qDebug() << "host:" << url.host();
> > > qDebug() << "path:" << url.path();
> > > qDebug() << "query:" << url.query();
> > > qDebug() << "fragment:" << url.fragment();
> > >
> > > QStringList fragments = url.fragment().split(";");
> > > assert(fragments.takeFirst() == "Intent");
> > > assert(fragments.takeLast() == "end");
> > > QMap<QString, QString> tokens;
> > > Q_FOREACH(const QString& fragment, fragments) {
> > > QStringList token = fragment.split("=");
> > > assert(token.size() == 2);
> > > tokens.insert(token[0], token[1]);
> > > }
> > > qDebug() << "tokens:" << tokens;
> >
> > I'd agree, except that it also brings extra logic to reconstruct
> > the intent URI parts:
> > - path + query (if any),
> > - host (if any) and path,
> > etc.
> > plus some other things,
> >
> > which in the end makes it also a bit convoluted imo,
>
> Fair enough (I disagree but let’s agree to disagree). A couple of comments on
> the logic though:
>
> - the intentRe regexp should end with the ";end;" token
> - the assignment of result.host and result.uriPath will fail if s has more
> than 2 items (e.g. if the path is several levels deep, like
> example.org/example/path/to/some/resource)

ok, I agree to disagree w/ my disagreement, ... this stupid inattention mistake
convinced me,

done (usage of QUrl)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-28:

In tests/unittests/intent-filter/CMakeLists.txt:

838 +set(TEST tst_IntentFilterTests)
839 +set(SOURCES
840 + ${CMAKE_SOURCE_DIR}/src/app/webcontainer/intent-filter.cpp

this should be ${webapp-container_SOURCE_DIR}/intent-filter.cpp

841 + tst_IntentFilterTests.cpp
842 +)
843 +set(WEBAPP_CONTAINER_INTENT_FILTER webapp-container-intent-filter)

the variable defined here seems unused

844 +include_directories(${CMAKE_SOURCE_DIR})

this should be ${webapp-container_SOURCE_DIR}

845 +add_executable(${TEST} ${SOURCES})
846 +qt5_use_modules(${TEST} Core Test Qml)
847 +add_test(${TEST} ${CMAKE_CURRENT_BINARY_DIR}/${TEST} -xunitxml -o ${TEST}.xml)

review: Needs Fixing

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-28

883. By Alexandre Abreu on 2015-01-28: addressed cmakelist.txt fixes

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-28:

> In tests/unittests/intent-filter/CMakeLists.txt:
>
> 838 +set(TEST tst_IntentFilterTests)
> 839 +set(SOURCES
> 840 + ${CMAKE_SOURCE_DIR}/src/app/webcontainer/intent-filter.cpp
>
> this should be ${webapp-container_SOURCE_DIR}/intent-filter.cpp
>
> 841 + tst_IntentFilterTests.cpp
> 842 +)
> 843 +set(WEBAPP_CONTAINER_INTENT_FILTER webapp-container-intent-filter)
>
> the variable defined here seems unused
>
> 844 +include_directories(${CMAKE_SOURCE_DIR})
>
> this should be ${webapp-container_SOURCE_DIR}
>
> 845 +add_executable(${TEST} ${SOURCES})
> 846 +qt5_use_modules(${TEST} Core Test Qml)
> 847 +add_test(${TEST} ${CMAKE_CURRENT_BINARY_DIR}/${TEST} -xunitxml -o
> ${TEST}.xml)

done

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-28:

342 +#include "intent-filter.moc"

this is not needed and generates warning at build time

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-28:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1408/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-28:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1411/rebuild

review: Needs Fixing (continuous-integration)

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-29

884. By Alexandre Abreu on 2015-01-29: remove *.moc file unneeded

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-29:

> 342 +#include "intent-filter.moc"
>
> this is not needed and generates warning at build time

sorry it was a leftover from a previous version

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-29

885. By Alexandre Abreu on 2015-01-29: add AP test comment

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-29:

> > > 676 + def get_intent_filtered_uri(self, uri):
> > > […]
> > > 681 + webviewContainer.slots.handleIntentUri(uri)
> > >
> > > This is wrong, the test case shouldn’t call a slot on the QML object: this
> > is
> > > not testing a real-world use-case any longer. Instead, the webapp’s
> homepage
> > > should have an "intent://" link and the test should click that link and
> > ensure
> > > that as a result the URL of the webview changes to the expected
> transformed
> > > URL.
> >
> > I know that this is "wrong", but I did it this way after exploring how
> > 'intrumentable' the url-dispatcher was. And it is not at the required
> > level to test the whole flow ...
> >
> > The way that it is it test *some* of the logic and bits of code (closer
> > to something at the unit level),
>
> Can’t the 'UriHandler' global object be mocked and made to emit its "opened"
> signal (without the need to tinker with the URL dispatcher)?
>
> If not, at the very least this test case should have a comment to explain what
> it does and why it’s not a real integration test.

I am convinced by that approach at this level (of testing) ... we would still strain
a bit far from a proper function test, and imo wouldn't be that different from
the selective testing above really, ... I added a comment in the test

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-29:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1420/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-29:

In tests/unittests/intent-filter/tst_IntentFilterTests.cpp:

1004 + IntentFilter * pf = new IntentFilter(QString());
1050 + IntentFilter * pf = new IntentFilter(filterFunctionSource);

pf is leaked. Could it be instantiated on the stack instead?

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-29:

In IntentFilter::isValidIntentFilterResult(…), the checks expect the result to have a host, but in the declaration of the IntentUriDescription struct, 'host' is marked optional. So, is it optional or required?

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-29:

In IntentFilter, d_ptr is never destroyed, so it’s leaked. You could use a QScopedPointer to automate memory management.

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-29:

279 + // Fallback to a noop
280 + result = d->evaluate(
281 + IntentFilterPrivate::DEFAULT_PASS_THROUGH_FILTER
282 + , intentDescription).toVariant().toMap();

This looks like a rather complex (and expensive) way of transforming intentDescription into a QVariantMap.
What about getting rid of the IntentUriDescription struct altogether, to replace it everywhere with QVariantMap?

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-29:

307 + trimUriSeparator(path);

Is this correct? A valid URI path normally starts with a leading forward slash, so why trim it? And a path that ends with a slash is (I think) different from a path that doesn’t. E.g.:

http://example.com/test/?q=r

http://example.com/test?q=r

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-30:

> In tests/unittests/intent-filter/tst_IntentFilterTests.cpp:
>
> 1004 + IntentFilter * pf = new IntentFilter(QString());
> 1050 + IntentFilter * pf = new IntentFilter(filterFunctionSource);
>
> pf is leaked. Could it be instantiated on the stack instead?

why 'need fixing'? I did it on purpose ... IntentFilter is a QObject which is not
copy constructiblem and ... this is a test ...

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-30

886. By Alexandre Abreu on 2015-01-30: oops fix leaked intent dptr

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-30:

> In IntentFilter, d_ptr is never destroyed, so it’s leaked. You could use a
> QScopedPointer to automate memory management.

totally, fixed

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-30:

> 279 + // Fallback to a noop
> 280 + result = d->evaluate(
> 281 +
> IntentFilterPrivate::DEFAULT_PASS_THROUGH_FILTER
> 282 + , intentDescription).toVariant().toMap();
>
> This looks like a rather complex (and expensive) way of transforming
> intentDescription into a QVariantMap.
> What about getting rid of the IntentUriDescription struct altogether, to
> replace it everywhere with QVariantMap?

The 'expensive' factor is very relative to the usage context ... but in this
case it not that, ... it has nothing to do w/ intentDescription, but w/ the result
of QJSValue -> QVAriantMap ...

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-30:

> In IntentFilter::isValidIntentFilterResult(…), the checks expect the result to
> have a host, but in the declaration of the IntentUriDescription struct, 'host'
> is marked optional. So, is it optional or required?

optional as you known and as I know ..., but as I said before
I wanted the object sent to the js have a strict & fixed structure,

I can remove if you strongly object,

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-30:

> > In tests/unittests/intent-filter/tst_IntentFilterTests.cpp:
> >
> > 1004 + IntentFilter * pf = new IntentFilter(QString());
> > 1050 + IntentFilter * pf = new IntentFilter(filterFunctionSource);
> >
> > pf is leaked. Could it be instantiated on the stack instead?
>
> why 'need fixing'? I did it on purpose ... IntentFilter is a QObject which is
> not
> copy constructiblem and ... this is a test ...

I’m not sure I understand your point. The fact that it is a test doesn’t mean that it shouldn’t follow good practices and free memory after itself.
But really, why not instantiate the objects on the stack and let them be destroyed automatically when they go out of scope? I.e.:

IntentFilter pf(QString());
QVERIFY(pf.isValidIntentUri(intentUris) == isValid);

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-30:

> > 279 + // Fallback to a noop
> > 280 + result = d->evaluate(
> > 281 +
> > IntentFilterPrivate::DEFAULT_PASS_THROUGH_FILTER
> > 282 + , intentDescription).toVariant().toMap();
> >
> > This looks like a rather complex (and expensive) way of transforming
> > intentDescription into a QVariantMap.
> > What about getting rid of the IntentUriDescription struct altogether, to
> > replace it everywhere with QVariantMap?
>
> The 'expensive' factor is very relative to the usage context ... but in this
> case it not that, ... it has nothing to do w/ intentDescription, but w/ the
> result
> of QJSValue -> QVAriantMap ...

Sure, the cost is probably overall negligible. Still, in this case d->evaluate(…) is called only to turn an IntentUriDescription into a QVariantMap (there’s no need to use a temporary QJSValue for that), and this feels like killing a fly with a sledgehammer to me. But I’m fine with it either way, after all, it works.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-30:

> > In IntentFilter::isValidIntentFilterResult(…), the checks expect the result
> to
> > have a host, but in the declaration of the IntentUriDescription struct,
> 'host'
> > is marked optional. So, is it optional or required?
>
> optional as you known and as I know ..., but as I said before
> I wanted the object sent to the js have a strict & fixed structure,
>
> I can remove if you strongly object,

I won’t strongly object :)
I’m just concerned that if someone has to pick up that code in the future, she will be puzzled as to whether host is optional or required.

By the way, could you add a link to https://developer.chrome.com/multidevice/android/intents somewhere in the code, for reference?

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-30

887. By Alexandre Abreu on 2015-01-30: fix path trimmning

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-30:

> 307 + trimUriSeparator(path);
>
> Is this correct? A valid URI path normally starts with a leading forward
> slash, so why trim it? And a path that ends with a slash is (I think)
> different from a path that doesn’t. E.g.:
>
> http://example.com/test/?q=r
>
> !=
>
> http://example.com/test?q=r

I want things to be somewhat normalized (if I can call it this way), for the
web and *in practice* I dont think it makes a difference (unless I miss an
obvious case).

After thinking about it, I think you are right on this, rather be strict ...

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-30:

> > > In tests/unittests/intent-filter/tst_IntentFilterTests.cpp:
> > >
> > > 1004 + IntentFilter * pf = new IntentFilter(QString());
> > > 1050 + IntentFilter * pf = new
> IntentFilter(filterFunctionSource);
> > >
> > > pf is leaked. Could it be instantiated on the stack instead?
> >

ok, I had a compiler error that led me to quickly think that the qobject
based was not statically instantiatable,

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-01-30:

> > > In IntentFilter::isValidIntentFilterResult(…), the checks expect the
> result
> > to
> > > have a host, but in the declaration of the IntentUriDescription struct,
> > 'host'
> > > is marked optional. So, is it optional or required?
> >
> > optional as you known and as I know ..., but as I said before
> > I wanted the object sent to the js have a strict & fixed structure,
> >
> > I can remove if you strongly object,
>
> I won’t strongly object :)
> I’m just concerned that if someone has to pick up that code in the future, she
> will be puzzled as to whether host is optional or required.
>
> By the way, could you add a link to
> https://developer.chrome.com/multidevice/android/intents somewhere in the
> code, for reference?

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-30

888. By Alexandre Abreu on 2015-01-30: fixes

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-30:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1422/rebuild

review: Needs Fixing (continuous-integration)

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-01-30

889. By Alexandre Abreu on 2015-01-30: fix additional free ptr

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-30:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1424/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-02-02:

I tested the branch on a device (using silo 28, in conjunction with https://code.launchpad.net/~osomon/webapps-core/gmaps-intent-hook/+merge/245982), and it works nicely, both when the webapp is already running and when it isn’t.

One thing that I think would be useful is a console.log() to inform that an intent URI was transformed into an http[s] one (with both URIs, to ease debugging in case of problems).

review: Approve

lp:~abreu-alexandre/webbrowser-app/intent updated on 2015-02-02

890. By Alexandre Abreu on 2015-02-02: Add log

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-02-02:

> One thing that I think would be useful is a console.log() to inform that an
> intent URI was transformed into an http[s] one (with both URIs, to ease
> debugging in case of problems).

+1, done,

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-02-02:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1428/rebuild

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alexandre Abreu

Ubuntu Phablet Team

to status/vote changes:

Rachel Liu

 === modified file 'src/app/webcontainer/CMakeLists.txt'
 --- src/app/webcontainer/CMakeLists.txt	2014-09-18 13:37:07 +0000
 +++ src/app/webcontainer/CMakeLists.txt	2015-02-02 13:56:12 +0000
@@ -18,13 +18,14 @@
      webapp-container-helper.cpp
      session-utils.cpp
      url-pattern-utils.cpp
++    intent-filter.cpp
+ )
  add_executable(${WEBAPP_CONTAINER} ${WEBAPP_CONTAINER_SRC})
  target_link_libraries(${WEBAPP_CONTAINER} ${COMMONLIB})
--qt5_use_modules(${WEBAPP_CONTAINER} Core Widgets Quick Sql DBus)
++qt5_use_modules(${WEBAPP_CONTAINER} Core Widgets Quick Qml Sql DBus)
  install(TARGETS ${WEBAPP_CONTAINER}
          RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})
 === modified file 'src/app/webcontainer/WebViewImplWebkit.qml'
 --- src/app/webcontainer/WebViewImplWebkit.qml	2014-11-25 15:00:45 +0000
 +++ src/app/webcontainer/WebViewImplWebkit.qml	2015-02-02 13:56:12 +0000
@@ -96,29 +96,12 @@
          return webappUrlPatterns && webappUrlPatterns.length !== 0
+     }
--    function navigationRequestedDelegate(request) {
--        if (!request.isMainFrame) {
--            request.action = WebView.AcceptRequest
--            return
--        }
--
--        // Pass-through if we are not running as a named webapp (--webapp='Gmail')
--        // or if we dont have a list of url patterns specified to filter the
--        // browsing actions
--        if ( ! haveValidUrlPatterns() && ! isRunningAsANamedWebapp()) {
--            request.action = WebView.AcceptRequest
--            return
--        }
--
--        var action = WebView.IgnoreRequest
--        var url = request.url.toString()
--
++    function shouldAllowNavigationTo(url) {
          // The list of url patterns defined by the webapp takes precedence over command line
          if (isRunningAsANamedWebapp()) {
              if (unityWebapps.model.exists(unityWebapps.name) &&
                  unityWebapps.model.doesUrlMatchesWebapp(unityWebapps.name, url)) {
--                request.action = WebView.AcceptRequest
--                return;
++                return true;
+             }
+         }
@@ -129,12 +112,36 @@
              for (var i = 0; i < webappUrlPatterns.length; ++i) {
                  var pattern = webappUrlPatterns[i]
                  if (url.match(pattern)) {
--                    action = WebView.AcceptRequest
--                    break
++                    return true;
+                 }
+             }
+         }
++        return false;
++    }
++
++    function navigationRequestedDelegate(request) {
++        if (!request.isMainFrame) {
++            request.action = WebView.AcceptRequest
++            return
++        }
++
++        // Pass-through if we are not running as a named webapp (--webapp='Gmail')
++        // or if we dont have a list of url patterns specified to filter the
++        // browsing actions
++        if ( ! haveValidUrlPatterns() && ! isRunningAsANamedWebapp()) {
++            request.action = WebView.AcceptRequest
++            return
++        }
++
++        var action = WebView.IgnoreRequest
++        var url = request.url.toString()
++
++        if (shouldAllowNavigationTo(url)) {
++            request.action = WebView.AcceptRequest
++            return;
++        }
++
          request.action = action
          if (action === WebView.IgnoreRequest) {
              console.debug('Opening: ' + url + ' in the browser window.')
 === added file 'src/app/webcontainer/intent-filter.cpp'
 --- src/app/webcontainer/intent-filter.cpp	1970-01-01 00:00:00 +0000
 +++ src/app/webcontainer/intent-filter.cpp	2015-02-02 13:56:12 +0000
@@ -0,0 +1,243 @@
++/*
++ * Copyright 2014 Canonical Ltd.
++ *
++ * This file is part of webbrowser-app.
++ *
++ * webbrowser-app is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; version 3.
++ *
++ * webbrowser-app is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ */
++
++#include "intent-filter.h"
++
++#include <QtCore/QRegularExpression>
++#include <QDebug>
++#include <QFile>
++#include <QJSEngine>
++#include <QJSValue>
++#include <QUrl>
++
++
++namespace {
++
++const char INTENT_SCHEME_STRING[] = "intent";
++const char INTENT_START_FRAGMENT_TAG[] = "Intent";
++const char INTENT_URI_PACKAGE_PREFIX[] = "package=";
++const char INTENT_URI_ACTION_PREFIX[] = "action=";
++const char INTENT_URI_CATEGORY_PREFIX[] = "category=";
++const char INTENT_URI_COMPONENT_PREFIX[] = "component=";
++const char INTENT_URI_SCHEME_PREFIX[] = "scheme=";
++const char INTENT_END_FRAGMENT_TAG[] = ";end";
++
++void trimUriSeparator(QString& uriComponent)
++{
++    uriComponent.remove(QRegExp("^/*")).remove(QRegExp("/*$"));
++}
++
++}
++
++class IntentFilterPrivate
++{
++public:
++
++    static const QString DEFAULT_PASS_THROUGH_FILTER;
++
++public:
++
++    IntentFilterPrivate(const QString& content);
++
++    QJSValue evaluate(const IntentUriDescription& intent);
++    QJSValue evaluate(const QString& customContent
++                      , const IntentUriDescription& intent);
++
++private:
++
++    QJSValue callFunction(
++            QJSValue & function
++            , const IntentUriDescription& intent);
++
++    QString _content;
++    QJSEngine _engine;
++    QJSValue _function;
++
++};
++
++// static
++const QString IntentFilterPrivate::DEFAULT_PASS_THROUGH_FILTER =
++        "(function(intent) { return intent; })";
++
++IntentFilterPrivate::IntentFilterPrivate(const QString& content)
++    : _content(content)
++{
++    if (_content.isEmpty())
++    {
++        _content = DEFAULT_PASS_THROUGH_FILTER;
++    }
++    _function = _engine.evaluate(_content);
++}
++
++QJSValue IntentFilterPrivate::callFunction(QJSValue & function
++                                          , const IntentUriDescription& intent)
++{
++    if (!function.isCallable())
++    {
++        qCritical() << "Invalid intent filter function (not callable)";
++        return QJSValue();
++    }
++
++    QVariantMap o;
++    o.insert("scheme", intent.scheme);
++    o.insert("uri", intent.uriPath);
++    o.insert("host", intent.host);
++
++    QJSValueList jsargs;
++    jsargs << _engine.toScriptValue(o);
++    return function.call(jsargs);
++}
++
++QJSValue IntentFilterPrivate::evaluate(const QString& customContent
++                                       , const IntentUriDescription& intent)
++{
++    QJSValue f = _engine.evaluate(customContent);
++    return callFunction(f, intent);
++}
++
++QJSValue IntentFilterPrivate::evaluate(const IntentUriDescription& intent)
++{
++    return callFunction(_function, intent);
++}
++
++// static
++bool IntentFilter::isValidLocalIntentFilterFile(const QString& filename)
++{
++    QFile f(filename);
++    if (!f.exists() || !f.open(QIODevice::ReadOnly))
++    {
++        return false;
++    }
++
++    // Perform basic validation
++    QJSEngine engine;
++    QJSValue result = engine.evaluate(QString(f.readAll()), filename);
++    return !result.isNull() && result.isCallable();
++}
++
++// static
++bool IntentFilter::isValidIntentFilterResult(const QVariantMap& result)
++{
++    return result.contains("scheme")
++        && result.value("scheme").canConvert(QVariant::String)
++        && result.contains("uri")
++        && result.value("uri").canConvert(QVariant::String);
++}
++
++// static
++bool IntentFilter::isValidIntentDescription(const IntentUriDescription& intentDescription)
++{
++    return !intentDescription.uriPath.isEmpty()
++        || !intentDescription.package.isEmpty();
++}
++
++
++IntentFilter::IntentFilter(const QString& content, QObject *parent) :
++    QObject(parent),
++    d_ptr(new IntentFilterPrivate(content))
++{
++}
++
++IntentFilter::~IntentFilter()
++{
++    delete d_ptr;
++}
++
++QVariantMap IntentFilter::applyFilter(const QString& intentUri)
++{
++    Q_D(IntentFilter);
++
++    QVariantMap result;
++    IntentUriDescription intentDescription =
++            parseIntentUri(QUrl::fromUserInput(intentUri));
++    if (!isValidIntentDescription(intentDescription))
++    {
++        return result;
++    }
++    QJSValue value = d->evaluate(intentDescription);
++    if (value.isObject()
++            && value.toVariant().canConvert(QVariant::Map))
++    {
++        QVariantMap r = value.toVariant().toMap();
++        if (isValidIntentFilterResult(r))
++        {
++            result = r;
++        }
++        else
++        {
++            // Fallback to a noop
++            result = d->evaluate(
++                        IntentFilterPrivate::DEFAULT_PASS_THROUGH_FILTER
++                        , intentDescription).toVariant().toMap();
++        }
++    }
++    return result;
++}
++
++bool IntentFilter::isValidIntentUri(const QString& intentUri) const
++{
++    // a bit overkill but anyway ...
++    return isValidIntentDescription(parseIntentUri(QUrl::fromUserInput(intentUri)));
++}
++
++IntentUriDescription
++parseIntentUri(const QUrl& intentUri)
++{
++    IntentUriDescription result;
++    if (intentUri.scheme() != INTENT_SCHEME_STRING
++            || !intentUri.fragment().startsWith(INTENT_START_FRAGMENT_TAG)
++            || !intentUri.fragment().endsWith(INTENT_END_FRAGMENT_TAG))
++    {
++        return result;
++    }
++    QString host = intentUri.host();
++    trimUriSeparator(host);
++    QString path = intentUri.path();
++    if (intentUri.hasQuery())
++    {
++        path += "?" + intentUri.query();
++        trimUriSeparator(path);
++    }
++    result.host = host;
++    result.uriPath = path;
++    QStringList infos = intentUri.fragment().split(";");
++    Q_FOREACH(const QString& info, infos)
++    {
++        if (info.startsWith(INTENT_URI_PACKAGE_PREFIX))
++        {
++            result.package = info.split(INTENT_URI_PACKAGE_PREFIX)[1];
++        }
++        else if (info.startsWith(INTENT_URI_ACTION_PREFIX))
++        {
++            result.action = info.split(INTENT_URI_ACTION_PREFIX)[1];
++        }
++        else if (info.startsWith(INTENT_URI_CATEGORY_PREFIX))
++        {
++            result.category = info.split(INTENT_URI_CATEGORY_PREFIX)[1];
++        }
++        else if (info.startsWith(INTENT_URI_COMPONENT_PREFIX))
++        {
++            result.component = info.split(INTENT_URI_COMPONENT_PREFIX)[1];
++        }
++        else if (info.startsWith(INTENT_URI_SCHEME_PREFIX))
++        {
++            result.scheme = info.split(INTENT_URI_SCHEME_PREFIX)[1];
++        }
++    }
++    return result;
++}
 === added file 'src/app/webcontainer/intent-filter.h'
 --- src/app/webcontainer/intent-filter.h	1970-01-01 00:00:00 +0000
 +++ src/app/webcontainer/intent-filter.h	2015-02-02 13:56:12 +0000
@@ -0,0 +1,105 @@
++/*
++ * Copyright 2014 Canonical Ltd.
++ *
++ * This file is part of webbrowser-app.
++ *
++ * webbrowser-app is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; version 3.
++ *
++ * webbrowser-app is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ */
++
++#ifndef _INTENT_FILTER_H_
++#define _INTENT_FILTER_H_
++
++#include <QObject>
++#include <QString>
++#include <QVariantMap>
++
++
++class QUrl;
++class IntentFilterPrivate;
++struct IntentUriDescription;
++
++/**
++ * @brief The IntentFilter class
++ */
++class IntentFilter : public QObject
++{
++    Q_OBJECT
++
++public:
++    IntentFilter(const QString& content,
++            QObject *parent = 0);
++    ~IntentFilter();
++
++    /**
++     * @brief isValidLocalIntentFilterFile
++     * @return
++     */
++    static bool isValidLocalIntentFilterFile(const QString& filename);
++
++    /**
++     * @brief isValidIntentDescription
++     * @return
++     */
++    static bool isValidIntentDescription(const IntentUriDescription& );
++
++    /**
++     * @brief isValidIntentFilterResult
++     * @return
++     */
++    static bool isValidIntentFilterResult(const QVariantMap& );
++
++    /**
++     * @brief apply
++     * @return
++     */
++    Q_INVOKABLE QVariantMap applyFilter(const QString& intentUri);
++
++    /**
++     * @brief isValidIntentUri
++     * @return
++     */
++    Q_INVOKABLE bool isValidIntentUri(const QString& intentUri) const;
++
++
++private:
++
++    IntentFilterPrivate* d_ptr;
++    Q_DECLARE_PRIVATE(IntentFilter)
++};
++
++
++struct IntentUriDescription
++{
++    QString uriPath;
++
++    // optional
++    QString host;
++
++    QString package;
++    QString action;
++    QString category;
++    QString component;
++    QString scheme;
++};
++
++/**
++ * @brief Parse a URI that is supposed to be an intent as defined here
++ *
++ * https://developer.chrome.com/multidevice/android/intents
++ *
++ * @param intentUri
++ * @return
++ */
++IntentUriDescription parseIntentUri(const QUrl& intentUri);
++
++#endif // _INTENT_FILTER_H_
 === modified file 'src/app/webcontainer/url-pattern-utils.h'
 --- src/app/webcontainer/url-pattern-utils.h	2014-11-19 14:38:16 +0000
 +++ src/app/webcontainer/url-pattern-utils.h	2015-02-02 13:56:12 +0000
@@ -27,11 +27,25 @@
  namespace UrlPatternUtils {
++/**
++ * @brief transformWebappSearchPatternToSafePattern
++ * @param doTransformUrlPath
++ * @return
++ */
  QString transformWebappSearchPatternToSafePattern(const QString&
                                                    , bool doTransformUrlPath = true);
++/**
++ * @brief isLocalHtml5ApplicationHomeUrl
++ * @return
++ */
  bool isLocalHtml5ApplicationHomeUrl(const QUrl&);
++/**
++ * @brief filterAndTransformUrlPatterns
++ * @param includePatterns
++ * @return
++ */
  QStringList filterAndTransformUrlPatterns(const QStringList & includePatterns);
+ }
 === modified file 'src/app/webcontainer/webapp-container.cpp'
 --- src/app/webcontainer/webapp-container.cpp	2014-11-26 15:55:26 +0000
 +++ src/app/webcontainer/webapp-container.cpp	2015-02-02 13:56:12 +0000
@@ -20,6 +20,7 @@
  #include "webapp-container.h"
  #include "chrome-cookie-store.h"
++#include "intent-filter.h"
  #include "local-cookie-store.h"
  #include "online-accounts-cookie-store.h"
  #include "session-utils.h"
@@ -104,6 +105,7 @@
+ }
  const QString WebappContainer::URL_PATTERN_SEPARATOR = ",";
++const QString WebappContainer::LOCAL_INTENT_FILTER_FILENAME = "local-intent-filter.js";
  WebappContainer::WebappContainer(int& argc, char** argv):
@@ -212,6 +214,9 @@
              return false;
+         }
++        // Handle an optional intent filter. The default filter does nothing.
++        setupLocalIntentFilterIfAny(context);
++
          m_component->completeCreate();
          return true;
@@ -220,6 +225,30 @@
+     }
+ }
++void WebappContainer::setupLocalIntentFilterIfAny(QQmlContext* context)
++{
++    if(!context)
++    {
++        return;
++    }
++
++    QString localIntentFilterFileContent;
++    if (IntentFilter::isValidLocalIntentFilterFile(LOCAL_INTENT_FILTER_FILENAME))
++    {
++        QFile f(LOCAL_INTENT_FILTER_FILENAME);
++        if (f.open(QIODevice::ReadOnly))
++        {
++            localIntentFilterFileContent = QString(f.readAll());
++        }
++        f.close();
++
++        qDebug() << "Using local intent filter file:"
++                 << LOCAL_INTENT_FILTER_FILENAME;
++    }
++    m_intentFilter.reset(new IntentFilter(localIntentFilterFileContent, NULL));
++    context->setContextProperty("webappIntentFilter", m_intentFilter.data());
++}
++
  bool WebappContainer::isValidLocalApplicationRunningContext() const
+ {
      return m_webappModelSearchPath.isEmpty() &&
 === modified file 'src/app/webcontainer/webapp-container.h'
 --- src/app/webcontainer/webapp-container.h	2014-11-20 16:46:17 +0000
 +++ src/app/webcontainer/webapp-container.h	2015-02-02 13:56:12 +0000
@@ -28,6 +28,9 @@
  #include <QStringList>
  #include <QScopedPointer>
++class IntentFilter;
++class QQmlContext;
++
  class WebappContainer : public BrowserApplication
+ {
      Q_OBJECT
@@ -49,6 +52,8 @@
      bool isValidLocalApplicationRunningContext() const;
      bool isValidLocalResource(const QString& resourceName) const;
      bool shouldNotValidateCommandLineUrls() const;
++    bool isValidLocalIntentFilterFile(const QString& filename) const;
++    void setupLocalIntentFilterIfAny(QQmlContext* context);
  private:
      QString m_webappName;
@@ -64,8 +69,10 @@
      QString m_localCookieStoreDbPath;
      QString m_userAgentOverride;
      QScopedPointer<WebappContainerHelper> m_webappContainerHelper;
++    QScopedPointer<IntentFilter> m_intentFilter;
      static const QString URL_PATTERN_SEPARATOR;
++    static const QString LOCAL_INTENT_FILTER_FILENAME;
  };
  #endif // __WEBAPP_CONTAINER_H__
 === modified file 'src/app/webcontainer/webapp-container.qml'
 --- src/app/webcontainer/webapp-container.qml	2015-01-06 19:14:09 +0000
 +++ src/app/webcontainer/webapp-container.qml	2015-02-02 13:56:12 +0000
@@ -32,6 +32,7 @@
      property string localCookieStoreDbPath: ""
++    property var intentFilterHandler
      property string url: ""
      property string webappName: ""
      property string webappModelSearchPath: ""
@@ -50,6 +51,9 @@
      title: getWindowTitle()
++    // Used for testing
++    signal intentUriHandleResult(string uri)
++
      function getWindowTitle() {
          var webappViewTitle =
                  webappViewLoader.item
@@ -274,6 +278,63 @@
          webappViewLoader.sourceComponent = webappViewComponent
+     }
++    function makeUrlFromIntentResult(intentFilterResult) {
++        var scheme = null
++        var hostname = null
++        var url = root.currentWebview.url || root.url
++        if (intentFilterResult.host
++                && intentFilterResult.host.length !== 0) {
++            hostname = intentFilterResult.host
++        }
++        else {
++            var matchHostname = url.toString().match(/.*:\/\/([^/]*)\/.*/)
++            if (matchHostname.length > 1) {
++                hostname = matchHostname[1]
++            }
++        }
++        if (intentFilterResult.scheme
++                && intentFilterResult.scheme.length !== 0) {
++            scheme = intentFilterResult.scheme
++        }
++        else {
++            var matchScheme = url.toString().match(/(.*):\/\/[^/]*\/.*/)
++            if (matchScheme.length > 1) {
++                scheme = matchScheme[1]
++            }
++        }
++        return scheme
++                + '://'
++                + hostname
++                + "/"
++                + (intentFilterResult.uri
++                    ? intentFilterResult.uri : "")
++    }
++
++    /**
++     * Identity function for non-intent URIs.
++     *
++     * Otherwise if the URI is an intent, tries to apply a webapp
++     * local filter (or identity) and reconstruct the target URI based
++     * on its result.
++     */
++    function handleIntentUri(uri) {
++        var _uri = uri;
++        if (webappIntentFilter
++                && webappIntentFilter.isValidIntentUri(_uri)) {
++            var result = webappIntentFilter.applyFilter(_uri)
++            _uri = makeUrlFromIntentResult(result)
++
++            console.log("Intent URI '" + uri + "' was mapped to '" + _uri + "'")
++        }
++
++        // Report the result of the intent uri filtering (if any)
++        // Done for testing purposed. It is not possible at this point
++        // to have AP call a slot and retrieve its result synchronously.
++        intentUriHandleResult(_uri)
++
++        return _uri
++    }
++
      // Handle runtime requests to open urls as defined
      // by the freedesktop application dbus interface's open
      // method for DBUS application activation:
@@ -294,6 +355,15 @@
                  return;
+             }
++            requestedUrl = handleIntentUri(requestedUrl);
++
++            // Add a small guard to prevent browsing to invalid urls
++            if (currentWebview
++                    && currentWebview.shouldAllowNavigationTo
++                    && !currentWebview.shouldAllowNavigationTo(requestedUrl)) {
++                return;
++            }
++
              root.url = requestedUrl
              root.currentWebview.url = requestedUrl
+         }
 === modified file 'tests/autopilot/webapp_container/tests/__init__.py'
 --- tests/autopilot/webapp_container/tests/__init__.py	2015-01-15 08:26:53 +0000
 +++ tests/autopilot/webapp_container/tests/__init__.py	2015-02-02 13:56:12 +0000
@@ -21,7 +21,7 @@
  import fixtures
  from autopilot.testcase import AutopilotTestCase
  from autopilot.platform import model
--from testtools.matchers import Equals
++from testtools.matchers import Equals, GreaterThan
  from autopilot.matchers import Eventually
  import ubuntuuitoolkit as uitk
@@ -96,6 +96,19 @@
                          Eventually(Equals(100), timeout=20))
          self.assertThat(webview.loading, Eventually(Equals(False)))
++    def get_intent_filtered_uri(self, uri):
++        webviewContainer = self.get_webcontainer_window()
++        watcher = webviewContainer.watch_signal(
++            'intentUriHandleResult(QString)')
++        previous = watcher.num_emissions
++        webviewContainer.slots.handleIntentUri(uri)
++        self.assertThat(
++            lambda: watcher.num_emissions,
++            Eventually(GreaterThan(previous)))
++        result = webviewContainer.get_signal_emissions(
++            'intentUriHandleResult(QString)')[-1][0]
++        return result
++
      def browse_to(self, url):
          webview = self.get_oxide_webview()
          webview.url = url
 === added file 'tests/autopilot/webapp_container/tests/test_intent_uri_support.py'
 --- tests/autopilot/webapp_container/tests/test_intent_uri_support.py	1970-01-01 00:00:00 +0000
 +++ tests/autopilot/webapp_container/tests/test_intent_uri_support.py	2015-02-02 13:56:12 +0000
@@ -0,0 +1,116 @@
++# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
++# Copyright 2014 Canonical
++#
++# This program is free software: you can redistribute it and/or modify it
++# under the terms of the GNU General Public License version 3, as published
++# by the Free Software Foundation.
++#
++# This program is distributed in the hope that it will be useful,
++# but WITHOUT ANY WARRANTY; without even the implied warranty of
++# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
++# GNU General Public License for more details.
++#
++# You should have received a copy of the GNU General Public License
++# along with this program. If not, see <http://www.gnu.org/licenses/>.
++
++from contextlib import contextmanager
++import os
++import tempfile
++import shutil
++
++from testtools.matchers import Equals
++from autopilot.matchers import Eventually
++
++from webapp_container.tests import WebappContainerTestCaseWithLocalContentBase
++
++
++@contextmanager
++def generate_temp_webapp_with_intent(intent_filter_content=""):
++    tmpdir = tempfile.mkdtemp()
++    manifest_content = """
++    {
++        "includes": ["http://www.test.com/*"],
++        "name": "test",
++        "domain":"",
++        "homepage":"http://www.test.com/"
++    }
++    """
++    manifest_file = "{}/webapp-properties.json".format(tmpdir)
++    with open(manifest_file, "w+") as f:
++        f.write(manifest_content)
++    if len(intent_filter_content) != 0:
++        intent_filter_file = "{}/local-intent-filter.js".format(tmpdir)
++        with open(intent_filter_file, "w+") as f:
++            f.write(intent_filter_content)
++    old_cwd = os.getcwd()
++    try:
++        os.chdir(tmpdir)
++        yield tmpdir
++    finally:
++        os.chdir(old_cwd)
++        shutil.rmtree(tmpdir)
++
++
++# Those tests rely on get_intent_filtered_uri() which
++# relies on implementation detail to trigger part of the intent handling
++# code. This comes from the fact that the url-dispatcher is not easily
++# instrumentable , so a full feature flow coverage is quite tricky to get.
++# Those tests are not really functional in that sense.
++class WebappContainerIntentUriSupportTestCase(
++        WebappContainerTestCaseWithLocalContentBase):
++    def test_basic_intent_parsing(self):
++        rule = 'MAP *.test.com:80 ' + self.get_base_url_hostname()
++        with generate_temp_webapp_with_intent() as webapp_install_path:
++            args = ['--webappModelSearchPath='+webapp_install_path]
++            self.launch_webcontainer_app(
++                args,
++                {'UBUNTU_WEBVIEW_HOST_MAPPING_RULES': rule})
++            webview = self.get_oxide_webview()
++            webapp_url = 'http://www.test.com/'
++            self.assertThat(webview.url, Eventually(Equals(webapp_url)))
++
++            intent_uri = 'intent://maps.google.es/maps?ie=utf-8&gl=es\
++#Intent;scheme=http;package=com.google.android.apps.maps;end'
++            self.assertThat(
++                'http://maps.google.es/maps?ie=utf-8&gl=es',
++                Equals(self.get_intent_filtered_uri(intent_uri)))
++
++    def test_webapp_with_invalid_default_local_intent(self):
++        rule = 'MAP *.test.com:80 ' + self.get_base_url_hostname()
++        filter = "1"
++        with generate_temp_webapp_with_intent(filter) as webapp_install_path:
++            args = ['--webappModelSearchPath='+webapp_install_path]
++            self.launch_webcontainer_app(
++                args,
++                {'UBUNTU_WEBVIEW_HOST_MAPPING_RULES': rule})
++            webview = self.get_oxide_webview()
++            webapp_url = 'http://www.test.com/'
++            self.assertThat(webview.url, Eventually(Equals(webapp_url)))
++
++            intent_uri = 'intent://www.test.com/maps?ie=utf-8&gl=es\
++#Intent;scheme=http;package=com.google.android.apps.maps;end'
++            self.assertThat(
++                'http://www.test.com/maps?ie=utf-8&gl=es',
++                Equals(self.get_intent_filtered_uri(intent_uri)))
++
++    def test_with_valid_default_local_intent(self):
++        rule = 'MAP *.test.com:80 ' + self.get_base_url_hostname()
++        filter = "(function(r) { \
++            return { \
++                'scheme': 'https', \
++                'host': 'maps.test.com', \
++                'uri': r.uri }; })"
++        with generate_temp_webapp_with_intent(filter) as webapp_install_path:
++            args = ['--webappModelSearchPath='+webapp_install_path]
++            self.launch_webcontainer_app(
++                args,
++                {'UBUNTU_WEBVIEW_HOST_MAPPING_RULES': rule})
++            webview = self.get_oxide_webview()
++            webapp_url = 'http://www.test.com/'
++            self.assertThat(webview.url, Eventually(Equals(webapp_url)))
++
++            intent_uri = 'intent://www.test.com/maps?ie=utf-8&gl=es\
++#Intent;scheme=http;package=com.google.android.apps.maps;end'
++            self.assertThat(
++                'https://maps.test.com/maps?ie=utf-8&gl=es',
++                Equals(self.get_intent_filtered_uri(intent_uri)))
 === modified file 'tests/unittests/CMakeLists.txt'
 --- tests/unittests/CMakeLists.txt	2014-12-02 09:21:38 +0000
 +++ tests/unittests/CMakeLists.txt	2015-02-02 13:56:12 +0000
@@ -18,3 +18,4 @@
  add_subdirectory(session-storage)
  add_subdirectory(favicon-fetcher)
  add_subdirectory(webapp-container-hook)
++add_subdirectory(intent-filter)
 === added directory 'tests/unittests/intent-filter'
 === added file 'tests/unittests/intent-filter/CMakeLists.txt'
 --- tests/unittests/intent-filter/CMakeLists.txt	1970-01-01 00:00:00 +0000
 +++ tests/unittests/intent-filter/CMakeLists.txt	2015-02-02 13:56:12 +0000
@@ -0,0 +1,9 @@
++set(TEST tst_IntentFilterTests)
++set(SOURCES
++    ${webapp-container_SOURCE_DIR}/intent-filter.cpp
++    tst_IntentFilterTests.cpp
++)
++include_directories(${webapp-container_SOURCE_DIR})
++add_executable(${TEST} ${SOURCES})
++qt5_use_modules(${TEST} Core Test Qml)
++add_test(${TEST} ${CMAKE_CURRENT_BINARY_DIR}/${TEST} -xunitxml -o ${TEST}.xml)
 === added file 'tests/unittests/intent-filter/tst_IntentFilterTests.cpp'
 --- tests/unittests/intent-filter/tst_IntentFilterTests.cpp	1970-01-01 00:00:00 +0000
 +++ tests/unittests/intent-filter/tst_IntentFilterTests.cpp	2015-02-02 13:56:12 +0000
@@ -0,0 +1,218 @@
++/*
++ * Copyright 2014 Canonical Ltd.
++ *
++ * This file is part of webbrowser-app.
++ *
++ * webbrowser-app is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; version 3.
++ *
++ * webbrowser-app is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ */
++
++// Qt
++#include <QtCore/QString>
++#include <QtTest/QtTest>
++
++// local
++#include "intent-filter.h"
++
++class IntentFilterTests : public QObject
++{
++    Q_OBJECT
++
++private Q_SLOTS:
++
++    void parseIntentUris_data()
++    {
++        QTest::addColumn<QString>("intentUris");
++
++        QTest::addColumn<QString>("scheme");
++        QTest::addColumn<QString>("package");
++        QTest::addColumn<QString>("uri");
++        QTest::addColumn<QString>("host");
++        QTest::addColumn<QString>("action");
++        QTest::addColumn<QString>("component");
++        QTest::addColumn<QString>("category");
++
++        QTest::addColumn<bool>("isValid");
++
++        QTest::newRow("Valid intent - host only")
++                << "intent://scan/#Intent;component=com;scheme=zxing;category=BROWSABLE;action=com;package=com.google.zxing.client.android;end"
++                << "zxing"
++                << "com.google.zxing.client.android"
++                << "/"
++                << "scan"
++                << "com"
++                << "com"
++                << "BROWSABLE"
++                << true;
++
++        QTest::newRow("Valid intent - no host w/ uri")
++                << "intent://scan/?a=1/#Intent;component=com;scheme=zxing;category=BROWSABLE;action=com;package=com.google.zxing.client.android;end"
++                << "zxing"
++                << "com.google.zxing.client.android"
++                << "?a=1"
++                << "scan"
++                << "com"
++                << "com"
++                << "BROWSABLE"
++                << true;
++
++        QTest::newRow("Valid intent - w/ host")
++                << "intent://host/my/long/path?a=1/#Intent;component=com;scheme=zxing;category=BROWSABLE;action=com;package=com.google.zxing.client.android;end"
++                << "zxing"
++                << "com.google.zxing.client.android"
++                << "my/long/path?a=1"
++                << "host"
++                << "com"
++                << "com"
++                << "BROWSABLE"
++                << true;
++
++        QTest::newRow("Valid intent - w/o host & uri-path")
++                << "intent://#Intent;scheme=trusper.referrertests;package=trusper.referrertests;end"
++                << "trusper.referrertests"
++                << "trusper.referrertests"
++                << ""
++                << ""
++                << ""
++                << ""
++                << ""
++                << true;
++
++        QTest::newRow("Valid intent - w/o host & uri-path and extra /")
++                << "intent:///#Intent;component=com;scheme=zxing;category=BROWSABLE;action=com;package=com.google.zxing.client.android;end"
++                << "zxing"
++                << "com.google.zxing.client.android"
++                << "/"
++                << ""
++                << "com"
++                << "com"
++                << "BROWSABLE"
++                << true;
++
++        QTest::newRow("Valid intent - w/o host & uri-path impler syntax")
++                << "intent://#Intent;component=com;scheme=zxing;category=BROWSABLE;action=com;package=com.google.zxing.client.android;end"
++                << "zxing"
++                << "com.google.zxing.client.android"
++                << ""
++                << ""
++                << "com"
++                << "com"
++                << "BROWSABLE"
++                << true;
++
++        QTest::newRow("Invalid intent")
++                << "intent:///#Inttent;component=com;scheme=zxing;category=BROWSABLE;action=com;package=com.google.zxing.client.android;end"
++                << ""
++                << ""
++                << ""
++                << ""
++                << ""
++                << ""
++                << ""
++                << false;
++    }
++
++    void parseIntentUris()
++    {
++        QFETCH(QString, intentUris);
++
++        QFETCH(QString, scheme);
++        QFETCH(QString, package);
++        QFETCH(QString, uri);
++        QFETCH(QString, host);
++        QFETCH(QString, action);
++        QFETCH(QString, component);
++        QFETCH(QString, category);
++
++        QFETCH(bool, isValid);
++
++        IntentUriDescription d = parseIntentUri(intentUris);
++
++        QCOMPARE(d.scheme, scheme);
++        QCOMPARE(d.package, package);
++        QCOMPARE(d.uriPath, uri);
++        QCOMPARE(d.host, host);
++        QCOMPARE(d.action, action);
++        QCOMPARE(d.component, component);
++        QCOMPARE(d.category, category);
++
++        QVERIFY(IntentFilter::isValidIntentDescription(d) == isValid);
++
++        QString emptyContent;
++        IntentFilter pf(emptyContent);
++        QVERIFY(pf.isValidIntentUri(intentUris) == isValid);
++    }
++
++    void applyFilters_data()
++    {
++        QTest::addColumn<QString>("intentUris");
++
++        QTest::addColumn<QString>("filterFunctionSource");
++
++        QTest::addColumn<QString>("scheme");
++        QTest::addColumn<QString>("uri");
++        QTest::addColumn<QString>("host");
++
++        QTest::newRow("Valid intent - default filter function")
++                << "intent://scan/#Intent;component=com;scheme=zxing;category=BROWSABLE;action=com;package=com.google.zxing.client.android;end"
++                <<  ""
++                << "zxing"
++                << "/"
++                << "scan";
++
++        QTest::newRow("Valid intent - default filter function")
++                << "intent://scan/#Intent;component=com;scheme=zxing;category=BROWSABLE;action=com;package=com.google.zxing.client.android;end"
++                <<  "(function(result) {return {'scheme': result.scheme+'custom', 'uri': result.uri+'custom', 'host': result.host+'custom'}; })"
++                << "zxingcustom"
++                << "/custom"
++                << "scancustom";
++
++        QTest::newRow("Valid intent - no (optional) host in filter result")
++                << "intent://host/my/long/path?a=1/#Intent;component=com;scheme=zxing;category=BROWSABLE;action=com;package=com.google.zxing.client.android;end"
++                <<  "(function(result) {return {'scheme': result.scheme+'custom', 'uri': result.uri+'custom' }; })"
++                << "zxingcustom"
++                << "my/long/path?a=1custom"
++                << "";
++
++        QTest::newRow("Valid intent - invalid filter fallback to default")
++                << "intent://host/my/long/path?a=1/#Intent;component=com;scheme=zxing;category=BROWSABLE;action=com;package=com.google.zxing.client.android;end"
++                <<  "(function(result) {return { 'uri': result.uri+'custom' }; })"
++                << "zxing"
++                << "my/long/path?a=1"
++                << "host";
++    }
++
++    void applyFilters()
++    {
++        QFETCH(QString, intentUris);
++
++        QFETCH(QString, filterFunctionSource);
++
++        QFETCH(QString, scheme);
++        QFETCH(QString, uri);
++        QFETCH(QString, host);
++
++        IntentFilter pf(filterFunctionSource);
++        QVERIFY(pf.isValidIntentUri(intentUris));
++
++        QVariantMap r = pf.applyFilter(intentUris);
++        QVERIFY(r.contains("scheme"));
++        QVERIFY(r.contains("uri"));
++
++        QCOMPARE(r.value("scheme").toString(), scheme);
++        QCOMPARE(r.value("host").toString(), host);
++        QCOMPARE(r.value("uri").toString(), uri);
++    }
++};
++
++QTEST_MAIN(IntentFilterTests)
++#include "tst_IntentFilterTests.moc"