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)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: Margaret, Assigned: mathu, Mentored)

References

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(3 files, 3 obsolete files)

An edge case, but one we'll need to fix.
Flags: firefox-backlog+
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]
Assignee: nobody → hammerly.matt
Status: NEW → ASSIGNED
Attached patch bug1124271_readermodenewtab.diff (obsolete) — Splinter Review
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 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 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});
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][do not land yet]
Attached patch bug1124271_readermodenewtab.diff (obsolete) — Splinter Review
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)
Attachment #8565809 - Attachment is obsolete: true
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+
Conflicting code was merged; this new patch shouldn't conflict
Attachment #8567425 - Flags: review?(jaws)
Adding a test for this fix
Attachment #8566334 - Attachment is obsolete: true
Attachment #8567431 - Flags: review?(jaws)
Attachment #8567425 - Flags: review?(jaws) → review+
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+
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)
Attachment #8567512 - Flags: review?(jaws) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21dedd807757
Whiteboard: [lang=js][good first bug][do not land yet] → [lang=js][good first bug]
Flags: qe-verify+
QA Contact: andrei.vaida
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
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]
https://hg.mozilla.org/mozilla-central/rev/b03801dea264
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 39
Iteration: --- → 38.3 - 23 Feb
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
Attached patch Patch for 38Splinter Review
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 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?
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.