webbrowser-app

Merge lp://staging/~rpadovani/webbrowser-app/data-address into lp://staging/webbrowser-app

data-address
Merge into trunk

Proposed by Riccardo Padovani on 2015-04-15

Status:

Merged

Approved by:

Olivier Tilloy on 2015-05-11

Approved revision:

974

Merged at revision:

1003

Proposed branch:

lp://staging/~rpadovani/webbrowser-app/data-address

Merge into:

lp://staging/webbrowser-app

Prerequisite:

lp://staging/~osomon/webbrowser-app/about-blank

Diff against target:

77 lines (+15/-3)

3 files modified

src/app/webbrowser/AddressBar.qml (+1/-1)
src/app/webbrowser/urlManagement.js (+6/-1)
tests/unittests/qml/tst_AddressBar.qml (+8/-1)

To merge this branch:

bzr merge lp://staging/~rpadovani/webbrowser-app/data-address

Related bugs:

Bug #1377953: data: URIs don't work	Medium	Fix Released
Bug #1441281: Address bar doesn't support new TLDs	Medium	Fix Released
Bug #1450154: Address bar doesn't support new TLDs	Medium	In Progress

Link a bug report

Reviewer	Review Type	Date Requested	Status
Olivier Tilloy		2015-04-15	Approve on 2015-05-11
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-04-28
Review via email: mp+256367@code.staging.launchpad.net

Commit message

Add support for data: URIs in the address bar, and remove length limitation for TLDs.

Description of the change

Fix bug #1377953 - data: URIs don't work

You can find some tests here: http://www-archive.mozilla.org/quality/networking/testing/datatests.html

They don't work all as expected (as described in the page), but they work as Chromium on vivid, so I think it's acceptable

Fix bug #1441281 - update looksLikeAUrl().

Remove .tdl regex length limitation (min 2, max *)

Based on lp:~osomon/webbrowser-app/about-blank

lp://staging/~rpadovani/webbrowser-app/data-address updated on 2015-04-15

969. By Riccardo Padovani on 2015-04-15: Update tests

Revision history for this message

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

FAILED: Continuous integration, rev:969
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1650/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2262/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/407/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/407/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/407/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2260/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-04-16:

8 + } else if (address.substr(0, 4).toLowerCase == "data:") {
9 + return address.substr(0, 4).toLowerCase + address.substr(5);

I’d suggest using a regexp instead of String.substr(…), and no need to do the substr() dance again if you know that the scheme in lowercase is "data":

    if (address.match(/^data:/i)) {
        return "data:" + address.substr(5)
    }

Note that the CI job failed because your changes break the existing unit tests, probably because the call to substr should have been with a length of 5, not 4.

review: Needs Fixing

lp://staging/~rpadovani/webbrowser-app/data-address updated on 2015-04-16

970. By Riccardo Padovani on 2015-04-16: Use regex instead of substr

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-04-16:

Thanks for the feedback, you're totally right :-)

Fixed!

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-04-16:

I’m still seeing unit test failures. To run the unit tests locally and fix them:

ctest -V -R tst_QmlTests

review: Needs Fixing

Revision history for this message

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

FAILED: Continuous integration, rev:970
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1660/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2290/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/417/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/417/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/417/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2288/console

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

review: Needs Fixing (continuous-integration)

lp://staging/~rpadovani/webbrowser-app/data-address updated on 2015-04-27

971. By Riccardo Padovani on 2015-04-27: Fix data uri tests
972. By Riccardo Padovani on 2015-04-27: Merge from upstream
973. By Riccardo Padovani on 2015-04-27: Fix implementation of urlManagement for data: address

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-04-27:

Tests fail because my implementation was wrong :-)

So, the data: address could has space in the url, so I need to check if is a data: address before checking if there are space in the address.

Then, for tdl, I need to check if the tld is > 2 ({2,}) and not 2 ({2}). Now should be all ok!

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-04-27:

There’s no new unit test for longer TLDs.
The changes otherwise look good.

review: Needs Fixing

lp://staging/~rpadovani/webbrowser-app/data-address updated on 2015-04-28

974. By Riccardo Padovani on 2015-04-28: Add test for tld longer than 3 chars

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-04-28:

Right.

I used http://com.google because it's the only valid one I know, is it ok?

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-05-11:

That looks good now, thanks!

review: Approve

Preview Diff

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

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Riccardo Padovani

Ubuntu Phablet Team

to status/vote changes:

Rachel Liu