1124271 - Clicking the reader mode button in an app tab opens reader mode in a new tab

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[:Margaret Leibovic]

An edge case, but one we'll need to fix.

Comment 1

[:Margaret Leibovic]

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

Comment 2

[Matt Hammerly [:mathu]]

Attachment: bug1124271_readermodenewtab.diff

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.

Comment 3

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

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.

Comment 4

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

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});

Comment 5

[Matt Hammerly [:mathu]]

Attachment: 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+?

Comment 6

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

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.

Comment 7

[Matt Hammerly [:mathu]]

Attachment: bug1124271_readermodenewtab.diff

Conflicting code was merged; this new patch shouldn't conflict

Comment 8

[Matt Hammerly [:mathu]]

Attachment: browser_bug1124271_readermodenewtab.diff

Adding a test for this fix

Comment 9

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

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.

Comment 10

[Matt Hammerly [:mathu]]

Attachment: browser_bug1124271_readermodenewtab.diff

Added test to browser.ini in this patch

I think I can stop spamming Jared with review requests now

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21dedd807757

Comment 12

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

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 :)

Comment 13

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/b03801dea264

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 39.0a1 (2015-02-25) using Windows 7 (x64), Ubuntu 14.04 (x64), Mac OS X 10.9.5.

Comment 16

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

Attachment: Patch for 38

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

Comment 17

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

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

Comment 18

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/581298c4704e

Comment 19

[Andrei Vaida [:avaida]]

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.