Closed
Bug 1124271
Opened 9 years ago
Closed 9 years ago
Clicking the reader mode button in an app tab opens reader mode in a new tab
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
People
(Reporter: Margaret, Assigned: mathu, Mentored)
References
Details
(Whiteboard: [lang=js][good first bug])
Attachments
(3 files, 3 obsolete files)
1.22 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
2.69 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
3.82 KB,
patch
|
Details | Diff | Splinter Review |
An edge case, but one we'll need to fix.
Updated•9 years ago
|
Flags: firefox-backlog+
Reporter | ||
Comment 1•9 years ago
|
||
Fixing this will involve updating this call to openUILinkIn here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#116 I see an "allowPinnedTabHostChange" property here that looks like exactly what we want: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#189
Mentor: margaret.leibovic
Whiteboard: [lang=js][good first bug]
Updated•9 years ago
|
Mentor: jaws
Updated•9 years ago
|
Assignee: nobody → hammerly.matt
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Hopefully passing json inline to a function like this isn't against any style guidelines. Patch changes two calls to openUILinkIn() by adding an object with one attribute: { "allowPinnedTabHostChange": true } The second changed call allows a user to activate reader mode without spawning a new tab, while the first changed call allows them to deactivate it and return to the page.
Attachment #8565809 -
Flags: review?(jaws)
Comment 3•9 years ago
|
||
Comment on attachment 8565809 [details] [diff] [review] bug1124271_readermodenewtab.diff Review of attachment 8565809 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Would you like to add a new test (or update the current one)? There is a test at /browser/base/content/test/general/browser_readerMode.js that does a few things with reader mode. To run the test, type `mach test browser/base/content/test/general/browser_readerMode.js` from the root directory of your source tree.
Attachment #8565809 -
Flags: review?(jaws) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8565809 [details] [diff] [review] bug1124271_readermodenewtab.diff Review of attachment 8565809 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/ReaderParent.jsm @@ +127,2 @@ > } else { > + win.openUILinkIn("about:reader?url=" + encodeURIComponent(url), "current", { "allowPinnedTabHostChange": true }); We could also refactor this code to remove some of the duplication: > let expectedURL = url.startsWith("about:reader") ? > this._getOriginalUrl(url) : > "about:reader?url=" + encodeURIComponent(url); > win.openUILinkIn(expectedURL, "current", {"allowPinnedTabHostChange": true});
Updated•9 years ago
|
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][do not land yet]
Assignee | ||
Comment 5•9 years ago
|
||
You gave the last one r+, but suggested another approach with less repetition. So here it is. What do I do after r+?
Attachment #8566334 -
Flags: review?(jaws)
Updated•9 years ago
|
Attachment #8565809 -
Attachment is obsolete: true
Comment 6•9 years ago
|
||
Comment on attachment 8566334 [details] [diff] [review] bug1124271_readermodenewtab.diff Review of attachment 8566334 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Matt Hammerly [:mathu] from comment #5) > Created attachment 8566334 [details] [diff] [review] > bug1124271_readermodenewtab.diff > > You gave the last one r+, but suggested another approach with less > repetition. So here it is. What do I do after r+? Normally after r+ the patch will land, but it would be nice to include a test here. I put some tips in comment #3 that you can use as a guide.
Attachment #8566334 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Conflicting code was merged; this new patch shouldn't conflict
Attachment #8567425 -
Flags: review?(jaws)
Assignee | ||
Comment 8•9 years ago
|
||
Adding a test for this fix
Attachment #8566334 -
Attachment is obsolete: true
Attachment #8567431 -
Flags: review?(jaws)
Updated•9 years ago
|
Attachment #8567425 -
Flags: review?(jaws) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8567431 [details] [diff] [review] browser_bug1124271_readermodenewtab.diff Review of attachment 8567431 [details] [diff] [review]: ----------------------------------------------------------------- Reading through the code it looks good, but you'll need to update browser.ini to include a reference to this test.
Attachment #8567431 -
Flags: review?(jaws) → feedback+
Assignee | ||
Comment 10•9 years ago
|
||
Added test to browser.ini in this patch I think I can stop spamming Jared with review requests now
Attachment #8567431 -
Attachment is obsolete: true
Attachment #8567512 -
Flags: review?(jaws)
Updated•9 years ago
|
Attachment #8567512 -
Flags: review?(jaws) → review+
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21dedd807757
Whiteboard: [lang=js][good first bug][do not land yet] → [lang=js][good first bug]
Updated•9 years ago
|
Flags: qe-verify+
QA Contact: andrei.vaida
Comment 12•9 years ago
|
||
The try run linked above is all green, except a build failure on OSX optimized builds but that looks like an infrastructure issue. This looks ready to land now. Nice job Matt :)
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b03801dea264
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b03801dea264
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 39
Updated•9 years ago
|
Iteration: --- → 38.3 - 23 Feb
Comment 15•9 years ago
|
||
Verified fixed on Nightly 39.0a1 (2015-02-25) using Windows 7 (x64), Ubuntu 14.04 (x64), Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Comment 16•9 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: reading list/reader mode [User impact if declined]: 38 will have partial reading list/reader mode support [Describe test coverage new/current, TreeHerder]: automated tests and manual tests [Risks and why]: none expected [String/UUID change made/needed]: none
Attachment #8583401 -
Flags: approval-mozilla-aurora?
Comment 17•9 years ago
|
||
Comment on attachment 8583401 [details] [diff] [review] Patch for 38 It looks like reading list/reader mode has broad approval and I can just use a=readinglist https://hg.mozilla.org/releases/mozilla-aurora/rev/fa8645257f11 for reference
Attachment #8583401 -
Flags: approval-mozilla-aurora?
Comment 18•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/581298c4704e
status-firefox38:
--- → fixed
Comment 19•9 years ago
|
||
Verified fixed on Aurora 38.0a2 (2015-03-29) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
You need to log in
before you can comment on or make changes to this bug.
Description
•