1270707 - URLs from LaterRun should be be passed through urlFormatter.formatURLPref()

Status: RESOLVED FIXED

(Firefox :: Tours, defect)

Description

[Nick Thomas [:nthomas] (UTC+12)]

While working on bug 1267305 we wanted to use to set browser.laterrun.pages.test.url to 
 https://www.mozilla.org/%LOCALE%/%APP%/%VERSION%/secondrun/?f=76&v=2
The page was loaded without all the %VAR% interpolated.

This appears to be beacause neither LaterRun or nsBrowserContentHandler.js pass the urls through urlFormatter(), eg at https://dxr.mozilla.org/mozilla-central/rev/369a5ee3a2880a4a98df3a00bf3db8d8f36b181b/browser/components/nsBrowserContentHandler.js#547
See lines 501, 502, 536, etc for comparison.

We aren't blocked in bug 1267035 because we only have one locale/app/version and can hardcode the values.

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1270707 - make LaterRun.jsm use nsIURLFormatter to format its URLs, r?jaws

Review commit: https://reviewboard.mozilla.org/r/51037/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51037/

Comment 2

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/51037/#review47707

::: browser/modules/LaterRun.jsm:140
(Diff revision 1)
>          try {
> -          uri = Services.io.newURI(pageData.url.trim(), null, null);
> +          let urlString = Services.urlFormatter.formatURL(pageData.url.trim());
> +          uri = Services.io.newURI(urlString, null, null);
>          } catch (ex) {
>            Cu.reportError("Invalid LaterRun page URL " + pageData.url + " ignored.");
> +          continue;

This line isn't technically related to this patch, so let me clarify:

When I initially implemented the test for this, I didn't realize I had to mock nsIXULAppInfo. So the `newURI` call threw, because the `formatURL` call just gave back the same URL with the "%foo%" stuff in it, which `newURI` didn't like. It then turned out that that broke the actual code, because the next if statement relies on `uri` being non-null. So I made the code bail if creating `uri` fails - this avoids adding the page data into `rv`, so it will get dropped on the floor (but will still show an error in the console due to the `Cu.reportError` call).

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8749570 [details]
MozReview Request: Bug 1270707 - make LaterRun.jsm use nsIURLFormatter to format its URLs, r?jaws

https://reviewboard.mozilla.org/r/51037/#review47761

Thanks, happy to see how simple it was to update the test.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8256ae1b267

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8256ae1b267

Comment 6

[:Gijs (he/him)]

Comment on attachment 8749570 [details]
MozReview Request: Bug 1270707 - make LaterRun.jsm use nsIURLFormatter to format its URLs, r?jaws

Approval Request Comment
[Feature/regressing bug #]: showing later run pages
[User impact if declined]: we can't make the page URLs include e.g. Firefox version, locale, etc., making it hard/impossible to target them well
[Describe test coverage new/current, TreeHerder]: has unit tests, which the patch updates
[Risks and why]: very very low - this is basically only used for funnelcakes and has thorough test coverage
[String/UUID change made/needed]: no.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8749570 [details]
MozReview Request: Bug 1270707 - make LaterRun.jsm use nsIURLFormatter to format its URLs, r?jaws

Aurora48+, Beta47+

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1dd42a351054

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8375426e0550