Closed
Bug 1270707
Opened 8 years ago
Closed 8 years ago
URLs from LaterRun should be be passed through urlFormatter.formatURLPref()
Categories
(Firefox :: Tours, defect)
Tracking
()
RESOLVED
FIXED
Firefox 49
People
(Reporter: nthomas, Assigned: Gijs)
References
Details
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
jaws
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
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.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51037/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51037/
Attachment #8749570 -
Flags: review?(jaws)
Assignee | ||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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+
Comment 5•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e8256ae1b267
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 6•8 years ago
|
||
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+
status-firefox47:
--- → affected
status-firefox48:
--- → affected
Comment 8•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/1dd42a351054
Comment 9•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8375426e0550
You need to log in
before you can comment on or make changes to this bug.
Description
•