Code review comment for lp://qastaging/~uriboni/webbrowser-app/find-in-page

Revision history for this message
Ugo Riboni (uriboni) wrote :

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.

« Back to merge proposal