webbrowser-app

Merge lp://staging/~abreu-alexandre/webbrowser-app/intent into lp://staging/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://staging/~abreu-alexandre/webbrowser-app/intent

Merge into:

lp://staging/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://staging/~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.staging.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://staging/~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://staging/~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://staging/~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://staging/~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://staging/~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://staging/~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://staging/~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://staging/~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://staging/~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://staging/~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://staging/~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://staging/~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: