Closed Bug 1270707 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Tours, defect)

45 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: nthomas, Assigned: Gijs)

References

Details

Attachments

(1 file)

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.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
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 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.
Attachment #8749570 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/e8256ae1b267
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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.
Attachment #8749570 - Flags: approval-mozilla-beta?
Attachment #8749570 - Flags: approval-mozilla-aurora?
Comment on attachment 8749570 [details]
MozReview Request: Bug 1270707 - make LaterRun.jsm use nsIURLFormatter to format its URLs, r?jaws

Aurora48+, Beta47+
Attachment #8749570 - Flags: approval-mozilla-beta?
Attachment #8749570 - Flags: approval-mozilla-beta+
Attachment #8749570 - Flags: approval-mozilla-aurora?
Attachment #8749570 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.