Merge lp://qastaging/~uriboni/webbrowser-app/find-in-page into lp://qastaging/webbrowser-app

Haven’t done a proper review yet, but here’s one preliminary remark: you’ll need to bump the dependency on liboxideqt-qmlplugin to >= 1.8 in debian/control, for webbrowser-app and qtdeclarative5-ubuntu-web-plugin.

FAILED: Continuous integration, rev:986
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1723/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2626/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/480/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/480/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/480/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2624/console

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

FAILED: Continuous integration, rev:991
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1725/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2637/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/482/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/482/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/482/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2635/console

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

Thanks for working on this. I did a first quick round of testing review, here are a few comments, in no particular order:

The flake8 unit test is failing.

The label of findInPageCounter should be i18n’d, with a proper TRANSLATORS comment.
As a consequence, autopilot tests shouldn’t rely on the value of that label. Can we somehow expose the values of current and count as properties of the label, for testing purposes?

It appears findInPageMode is never assigned a default value. Not sure whether javascript (and its QML implementation) specifies that booleans are false by default, but it would be cautious to have it default to false.

87 + value: textField.text.length > 1 ? textField.text : ""
  Shouldn’t the above be [textField.text.length > 0] ? Or is it intentional that searching doesn’t start for one single character? I can’t open the UX spec right now, if this is intentional would you mind adding a comment to explain the rationale?

« This is because the UI Toolkit component TextField does not support adding components after the clear button »
  Can you raise that with design (James Mulholland and the UITK team, to ensure they are aware of the issue?

The size of ChromeButton instances in Chrome.qml has been updated at revision 997, can you please update the new instances too?

In the autopilot emulator for the chrome, the new buttons could use a more explicit naming scheme, e.g. "find_next_button()" and "find_prev_button()".

FAILED: Continuous integration, rev:992
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1747/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2747/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/504/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/504/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/504/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2745/console

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

Fixed everything except the following:

> The label of findInPageCounter should be i18n’d, with a proper TRANSLATORS
> comment.
> As a consequence, autopilot tests shouldn’t rely on the value of that label.
> Can we somehow expose the values of current and count as properties of the
> label, for testing purposes?

I disagree with this. In my opinion numbers are not language specific and the "/" is just a random separator.
Who is going to translate a slash ?

> It appears findInPageMode is never assigned a default value. Not sure whether
> javascript (and its QML implementation) specifies that booleans are false by
> default, but it would be cautious to have it default to false.

Variables are not initialized in JS and an uninitialized variable when interpreted in boolean context has a false value, but I have fixed this for clarity.

> « This is because the UI Toolkit component TextField does not support adding
> components after the clear button »
> Can you raise that with design (James Mulholland and the UITK team, to
> ensure they are aware of the issue?

I spoke with Rae who spoke with James who said for now it is ok to do it the way we do it, and they will see with the UI Toolkit people if this can be changed in the future.

Thanks for the changes, they look good.

> I disagree with this. In my opinion numbers are not language specific
> and the "/" is just a random separator.
> Who is going to translate a slash ?

That’s an opinion, not an informed decision. The "n/m" formatting might look awkward is some languages, or maybe even completely meaningless. Can you check that this formatting is ok for all languages listed at https://en.wikipedia.org/wiki/List_of_language_names ? If you can’t, then the string should be i18n’d.

FAILED: Continuous integration, rev:998
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1753/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2771/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/510/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/510/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/510/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2769/console

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

Looks good and fixes the issue

FAILED: Continuous integration, rev:999
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1754/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2774/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/511/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/511/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/511/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2772/console

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

> Looks good and fixes the issue

Please ignore, commented on the wrong MR

FAILED: Continuous integration, rev:1000
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1756/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2780/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/513/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/513/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/513/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2778/console

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

FAILED: Continuous integration, rev:1001
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1787/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2874/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/544/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/544/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/544/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2872/console

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

There are a couple of conflicts when merging this branch into the latest trunk, can you please resolve them?

In AddressBar.qml, can the following three new properties be marked readonly?
  findInPagePattern
  current
  count

FAILED: Continuous integration, rev:1003
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1809/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2935/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/566/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/566/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/566/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2933/console

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

> In AddressBar.qml, can the following three new properties be marked
> readonly?
> findInPagePattern
> current
> count

searchSuggestions.active should be false when chrome.findInPageMode is true.

Is the removal of the 200 response code in HTTPRequestHandler.do_GET for "/suggest" intentional?

In test_activation(), after leaving the find in page mode, can you also check that the contents of the address bar are restored? Also I think the focus should be unset on the address bar when leaving this mode (and this should be tested too). The internal.resetFocus() function in Browser.qml probably is what you want to use.

FAILED: Continuous integration, rev:1008
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1823/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2966/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/580/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/580/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/580/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2964/console

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

FAILED: Continuous integration, rev:1009
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1834/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3001/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/591/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/591/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/591/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2999/console

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

When running the unit tests in verbose mode, I’m seeing the following warning:

    QWARN : QmlTests::AddressBar::test_exitingFindInPageRestoresUrl() file://…/src/app/webbrowser/AddressBar.qml:58:15: Unable to assign [undefined] to bool

This could easily be addressed by setting findController to a mock in the unit test.

Since UbuntuWebView02.qml imports oxide 1.8, the runtime dependency of qtdeclarative5-ubuntu-web-plugin on liboxideqt-qmlplugin needs to be bumped to 1.8 in debian/control.

In AddressBar.qml:
 - I don’t think the changes to the 'visible' property of the favicon and the 'name' property of the Icon with id 'action' are useful, given that the parent item is invisible when findInPageMode is true.
 - In the findInPageCounter Label, instead of #5d5d5d, use UbuntuColors.darkGrey for the color.

In Browser.qml:
 - The 'findinpage' action should be enabled only if !chrome.findInPageMode.

FAILED: Continuous integration, rev:1014
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1841/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3025/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/598/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/598/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/598/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3023/console

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

I am not convinced the change to src/Ubuntu/Web/UbuntuWebView02.qml is needed (I don’t see why it should be, and reverting it doesn’t seem to break the functionality, at least when run locally on my desktop). There’s nothing in the UbuntuWebView component that requires oxide 1.8, it’s only the browser that makes use of it that requires access to the findController API. The import statement in AddressBar.qml should probably be bumped though. Even though not strictly needed for the code to be parsed and interpreted by the QML engine, it’s assuming the findController object is there.

When launching the app locally I’m seeing the following message printed twice before anything else in the console:

    QString::arg: Argument missing: "" , 0

Could you look into what’s causing it to be printed?

That looks good now, thanks!

Note: I approved but I’m holding off the top-approval until oxide 1.8 (which this MR has a hard dependency on) is released.

FAILED: Continuous integration, rev:1017
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1842/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3028/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/599/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/599/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/599/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3026/console

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

FAILED: Continuous integration, rev:1017
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1873/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3160/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/630/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/630/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/630/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3158/console

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

FAILED: Continuous integration, rev:1020
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1876/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3173/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/633/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/633/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/633/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3171/console

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

Great feature!
Some comments:
- it's impossible now to enter an address and press enter to navigate (search in page intercept the enter key?)
- I expect CTRL+F to use the search
- How could I esc the search mode? ESC button doesn't work, and clicking the back arrow is a bad UX IMO
- I see it's by design you don't trigger the search if only one character is in the textfield, but it's a bit confusing IMO, because all other browsers highlights only one char, so the first time I entered a char I expect it to be evidenced: if nothing is highlighted, I think there isn't any occurrence of that char in the page

Other than that, looks awesome to me, thanks :-)

There are now conflicts when merging this branch into the latest trunk.

Additionally the QML unit tests are failing with errors like this:

    AddressBar.qml:247: TypeError: Property 'next' of object QObject_QML_32(0x2e540a0) is not a function

And as pointed out by Riccardo, validating a URL in the address bar with Enter doesn’t work any longer, and Ctrl+F should activate the find in page mode.

Superseded by https://code.launchpad.net/~osomon/webbrowser-app/find-in-page/+merge/263547.