1381992 - Ability to tell if a tab is in reader mode and to move and tab in and out of reader mode

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, enhancement, P3)

Description

[Andy McKay]

It would be nice if you could tell if a tab is in reader mode or not. We could add that into https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab and also trigger the update event each time the reader mode status changes: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onUpdated

Comment 1

[Shane Caraveo (:mixedpuppy)]

This information should already be in the url and update event already, is there something more to this?

Comment 2

[:Gijs (out for now; he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> This information should already be in the url and update event already, is
> there something more to this?

I think for compatibility, we shouldn't expose URLs like about:reader?url=foo, we should just expose "foo" and then expose the reader mode bit separately, also because other browsers (e.g. edge) use a different way of expressing reader mode (special read protocol, ie read:foo). Would that make sense?

Comment 3

[Shane Caraveo (:mixedpuppy)]

A read protocol would be nice.  bsilverberg will be looking at reader mode soon, redirecting.

Comment 4

[Andy McKay]

It might be nice to work in conjunction with bug 1371793. Because you can't just take the about:reader? URL from tabs.query and pass it through to tabs.create at this time. Re-opening based on the last two comments.

Comment 5

[Bob Silverberg [:bsilverberg]]

Thanks for the feedback, Shane. I had mentioned this in my "intent to implement" email as well. I feel like transitioning to a read: protocol is outside the scope of creating this API, but if we do implement this feature then a change from about:reader to a read: protocol won't have any effect on consumers of the API.

Comment 6

[Bob Silverberg [:bsilverberg]]

I am adding the ability to put a tab into reader mode and to take a tab out of reader mode into the scope of this bug.

Comment 8

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1381992 - Add some reader mode support to the tabs API,

This adds a hasReaderMode function to the tabs API which reports back
true or false.

Review commit: https://reviewboard.mozilla.org/r/178426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178426/

Comment 9

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1381992 - Part 2: Detect if a tab is in reader mode and enter/leave reader mode,

This adds three methods to the tabs API:

isInReaderMode() which reports whether a tab is currently in reader mode.
enterReaderMode() which puts a tab into reader mode, if possible.
leaveReaderMode() which takes a tab out of reader mode, if it is currently in reader mode.

Review commit: https://reviewboard.mozilla.org/r/178424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178424/

Comment 10

[Shane Caraveo (:mixedpuppy)]

I'm thinking about how this can be done without loading ReaderMode.jsm into the extension content since it's already loaded into tab-content.  This is what I am thinking.

in tab-content.js add the ability to query the info we need.  In AboutReaderListener, listen for Reader:IsReadable which would respond with something like:

sendAsyncMessage("Reader:Readable", { isArticle: ReaderMode.isProbablyReaderable(content.document) });

isArticle is following the convention for other messages there.

I think we can live with just a toggle, since we have isInReaderMode as well.  tab-content already supports a toggle message, so we can just send that.

This should remove any need to modify ExtensionContent.jsm or extension-process-script.js

What do you think?

Comment 11

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/1-2/

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review183972

::: browser/base/content/tab-content.js:290
(Diff revision 2)
> +
> +      case "Reader:IsAboutReader":
> +        sendAsyncMessage(
> +          "Reader:aboutReader",
> +          { isAboutReader: this.isAboutReader });
> +        break;

I'm thinking we just combine these into one.

"Reader:ReaderState":

if isAboutReader
  return {isArticle:true,isAboutReader:true}
else if isProbablyReaderable:
  return {isArticle:true,isAboutReader:false}
return {isArticle:false,isAboutReader:false}

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/2-3/

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review184016

::: browser/base/content/tab-content.js:283
(Diff revision 3)
> +
> +      case "Reader:GetReaderState":
> +        let response = { isInReaderMode: this.isAboutReader };
> +        response.isArticle = response.isInReaderMode
> +          ? true
> +          : ReaderMode.isProbablyReaderable(content.document);

|| instead of ternary

Comment 15

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/3-4/

Comment 16

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review184016

> || instead of ternary

Nice. :)

Comment 17

[:Gijs (out for now; he/him)]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review184080

I apologize for the driveby, but I think we can do this without adding an extra message to tab-content etc.

Note also that this doesn't seem to implement the same API on android. Do we intend to support it there, too?

::: browser/base/content/tab-content.js:281
(Diff revision 4)
> +        response.isArticle = response.isInReaderMode ||
> +          ReaderMode.isProbablyReaderable(content.document);

`isProbablyreaderable` isn't free, and equally doesn't give the right result if the DOM hasn't been loaded yet.

Instead, can we use ReaderParent.jsm, and just add a method on there that takes a tab/browser and returns `browser.isArticle` OR'd with whether the tab is in reader mode (has an about:reader URI) ?

Comment 18

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/4-5/

Comment 19

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Requesting review on the changes to tab-content.js and browser_readerMode.js, please and thank you.

Comment 20

[Bob Silverberg [:bsilverberg]]

Oops, I added that r? before even seeing your comment, Gijs. I will take a look at it and see what I can do.

Comment 21

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs from comment #17)
> I apologize for the driveby, but I think we can do this without adding an
> extra message to tab-content etc.

Why?  It was useful.

> Note also that this doesn't seem to implement the same API on android. Do we
> intend to support it there, too?

We'll need a followup bug for android.

> Instead, can we use ReaderParent.jsm, and just add a method on there that
> takes a tab/browser and returns `browser.isArticle` OR'd with whether the
> tab is in reader mode (has an about:reader URI) ?

We'd need to modify the toggle function (or add an alternate arg to it) so it takes a browser rather than event object.  Since we could actually just directly check browser.isArticle and look at about:reader, I'm thinking we can just directly send the message rather than use ReaderParent.  OTOH using ReaderParent may avoid issues with any future changes.  Do you have any strong opinion on that Gijs?

Comment 22

[Shane Caraveo (:mixedpuppy)]

Gijs: 

Bob and I have been chatting about isArticle and the fact that we don't have a clean way to know when isArticle is actually determined since a false is only sent on navigation.  That makes it hard for an addon to know when they can rely on the value being accurate unless it is ==true.  note that tab.status=="complete" can happen prior to 

So my thought is: 

- in tab-content.js always send Reader:UpdateReaderButton, do away with the forceNonArticle stuff, probably also makes the onLocationChange handling unnecessary.
- we listen on that message rather than relying on browser.isArticle.
- browser.tabs.Tab gains an isArticle flag, which defaults to undefined.
- we call the tabs.onUpdated event listener for extensions with the value isArticle (also setting isArticle on the tabs.Tab instance)

This way, an extension has a deterministic way to know when the article value is valid.

I thought about just sending Reader:PushState, but if that happens too early (an article check is pending) it is ignored, so it also doesn't provide us with what we need.

Comment 23

[:Gijs (out for now; he/him)]

We used to always send up the result of the reader mode message ("even" false) prior to the fix from bug 1147487, but then enabling reader mode caused performance regressions. Reverting the fix (ie always sending isArticle, "even" false) doesn't seem desirable - it'll likely cause the same perf regressions. Note also that we have extant security bugs about Firefox responding poorly to abusive sites that aggressively history.pushState(), and I would like not to add to that problem.

I don't really understand why an add-on would need an update from "unknown" to "false" state. Presumably it would only care if:

a) reader mode becomes available
b) reader mode stops being available

for any given tab. It seems fine to only send updates in that case, and to expose "not readerable" in all other cases. If we send updates, the add-on will still know a tab is readerable at the first possibility. If you wanted to write an add-on to automatically enter reader mode whenever it was available, say, you would simply listen to tab update messages that send "yep, reader mode is available now" and then enter it.

(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> (In reply to :Gijs from comment #17)
> > I apologize for the driveby, but I think we can do this without adding an
> > extra message to tab-content etc.
> 
> Why?  It was useful.

I don't really understand why. Webextension code (that then communicates to the oop webextension, maybe) in the parent needs to know whether the tab is in reader mode, right, not webextension code in the content process? And the parent process already has this information. So you shouldn't need to talk to the content process.

If you need to send updates to the add-on (which is implied in comment #22) then you could add listeners in the parent process  for UpdateReaderButton.

Does that make sense? :-)

Comment 24

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs from comment #23)

> I don't really understand why an add-on would need an update from "unknown"
> to "false" state. Presumably it would only care if:

It's the indeterminate behavior problem...when do you look to see if it's true?  And if it's false, is it really false or do I wait longer?  

However, I think just adding the update notification is probably enough.

> (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> > (In reply to :Gijs from comment #17)
> > > I apologize for the driveby, but I think we can do this without adding an
> > > extra message to tab-content etc.
> > 
> > Why?  It was useful.
> 
> I don't really understand why. Webextension code (that then communicates to

I think you misread me.  I was saying "Why Apologize, your input was useful".  I should have shortened the quoted bit more.

Comment 25

[:Gijs (out for now; he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #24)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> > > (In reply to :Gijs from comment #17)
> > > > I apologize for the driveby, but I think we can do this without adding an
> > > > extra message to tab-content etc.
> > > 
> > > Why?  It was useful.
> > 
> > I don't really understand why. Webextension code (that then communicates to
> 
> I think you misread me.  I was saying "Why Apologize, your input was
> useful".  I should have shortened the quoted bit more.

Ahhhh. This makes sense.

It probably doesn't matter much, but for whatever it's worth - I apologized because unsolicited feedback (especially of the negative "don't do this" variety) can be a bit demoralizing / "stop energy", and I try not to demoralize people... :-)

Comment 26

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/5-6/

Comment 27

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/6-7/

Comment 28

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Gijs, do you mind taking another look at the latest version of the code?

Comment 29

[:Gijs (out for now; he/him)]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review185196

The approach to getting reader mode article / "in reader mode" data looks great, as does the approach to toggling reader mode. I have a few questions / suggestions below, but as this addresses my main concerns with the previous approach, I think Shane or someone else who knows webextensions better than me would be best off reviewing the rest.

::: browser/components/extensions/ext-tabs.js:297
(Diff revision 7)
> +          let readerModeChangeListener = (eventName, event) => {
> +            let {gBrowser} = event.nativeTab.ownerGlobal;
> +            let tab = tabManager.wrapTab(
> +              gBrowser.getTabForBrowser(event.nativeTab));
> +
> +            fireForTab(tab, {readerMode: event.isArticle});

I'm sure this is me missing something, but it looks like this sends the add-on an object with a 'reader mode' property, with a bool value, but the bool indicates whether or not reader mode is available, not whether the tab is in reader mode.

I don't see any specific code to notify add-ons when the tab enters/leaves reader mode, is that right? Based on the test, it seems like we're assuming that the `isArticle` bool will change when entering / leaving reader mode. I'm not sure that is always (going to be) the case.

I wonder if we should change the property name we use for this update event to be clearer, and if we should have a separate update event for entering/leaving reader mode.

::: browser/components/extensions/ext-tabs.js:931
(Diff revision 7)
>          },
> +
> +        async toggleReaderMode(tabId) {
> +          let tab = await promiseTabWhenReady(tabId);
> +          if (!tab.isInReaderMode && !tab.isArticle) {
> +            return;

Should the add-on get some kind of error back?

::: browser/components/extensions/test/browser/browser_ext_tabs_readerMode.js:70
(Diff revision 7)
> +  tabs = await Promise.all([
> +    extension.awaitMessage("tabUpdated"),
> +    extension.awaitMessage("readerMode:false"),
> +  ]);
> +  ok(tabs[0].url.startsWith(READER_MODE_PREFIX), "Tab url indicates reader mode.");
> +  ok(!tabs[1].isArticle, "Tab is not readerable.");

Is it always the case that a tab that's in reader mode has `isArticle` set to false? If so, I expect it's a side-effect of us displaying the contents after the initial page load, or something... not necessarily reliable. So I'm not sure if it makes sense to assert it - but maybe I'm missing something? :-)

Comment 30

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review185214

Just some small changes and a try, I think we're there.

::: browser/components/extensions/ext-browser.js:546
(Diff revision 7)
> +   *        Whether the document in the tab can be rendered in reader mode.
> +   * @private
> +   */
> +  emitReaderModeUpdated(nativeTab, isArticle) {
> +    let tabId = this.getId(nativeTab);
> +    this.emit("tab-reader-mode-updated", {nativeTab, tabId, isArticle});

I don't think this is updating the mode, but is updating the article flag.  How about just tab-isarticle.  Probably same for function name

::: browser/components/extensions/ext-tabs.js:297
(Diff revision 7)
> +          let readerModeChangeListener = (eventName, event) => {
> +            let {gBrowser} = event.nativeTab.ownerGlobal;
> +            let tab = tabManager.wrapTab(
> +              gBrowser.getTabForBrowser(event.nativeTab));
> +
> +            fireForTab(tab, {readerMode: event.isArticle});

lets call that isArticle rather than readerMode since this isn't really a notification on readerMode.

::: browser/components/extensions/ext-tabs.js:931
(Diff revision 7)
>          },
> +
> +        async toggleReaderMode(tabId) {
> +          let tab = await promiseTabWhenReady(tabId);
> +          if (!tab.isInReaderMode && !tab.isArticle) {
> +            return;

yeah, toss an extensionerror

Comment 31

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/7-8/

Comment 32

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/8-9/

Comment 33

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review185196

> I'm sure this is me missing something, but it looks like this sends the add-on an object with a 'reader mode' property, with a bool value, but the bool indicates whether or not reader mode is available, not whether the tab is in reader mode.
> 
> I don't see any specific code to notify add-ons when the tab enters/leaves reader mode, is that right? Based on the test, it seems like we're assuming that the `isArticle` bool will change when entering / leaving reader mode. I'm not sure that is always (going to be) the case.
> 
> I wonder if we should change the property name we use for this update event to be clearer, and if we should have a separate update event for entering/leaving reader mode.

I have changed the names because it was misleading. As for having an event for entering/leaving reader mode, that's a good idea, but I think I'll file a follow-up bug for that.

> Is it always the case that a tab that's in reader mode has `isArticle` set to false? If so, I expect it's a side-effect of us displaying the contents after the initial page load, or something... not necessarily reliable. So I'm not sure if it makes sense to assert it - but maybe I'm missing something? :-)

It was something I noticed in my manual testing, but you're right, perhaps it's not intentional and therefore I shouldn't assert it.

Comment 34

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/9-10/

Comment 35

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

https://reviewboard.mozilla.org/r/178426/#review185480

Comment 36

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/10-11/

Comment 37

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/11-12/

Comment 38

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/12-13/

Comment 39

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/13-14/

Comment 40

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/14-15/

Comment 41

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/15-16/

Comment 42

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/16-17/

Comment 43

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/17-18/

Comment 44

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/18-19/

Comment 45

[Bob Silverberg [:bsilverberg]]

Thanks for the review, and the help, Shane. The last try was pristine. I need to hold off landing this until after the merge, so I'll just let it sit for a bit.

Comment 46

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/19-20/

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e174306e0b0
Add some reader mode support to the tabs API, r=mixedpuppy

Comment 48

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for e.g. failing chrome's mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html on Android:

https://hg.mozilla.org/integration/autoland/rev/a97948d297e2bbd8503ec3955f89772834f609e8

Push which ran failing tests: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7eb30eea17798e0e46481f381d91e2b63e50f180&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133131613&repo=autoland

[task 2017-09-25T15:43:35.742Z] 15:43:35     INFO -  85 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html
[task 2017-09-25T15:49:06.982Z] 15:49:06     INFO -  Buffered messages logged at 15:43:37
[task 2017-09-25T15:49:06.983Z] 15:49:06     INFO -  86 INFO SpawnTask.js | Entering test test_activeTab_pageAction
[task 2017-09-25T15:49:06.983Z] 15:49:06     INFO -  87 INFO Extension loaded
[task 2017-09-25T15:49:06.983Z] 15:49:06     INFO -  Buffered messages finished
[task 2017-09-25T15:49:06.983Z] 15:49:06     INFO -  88 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | Test timed out.
[task 2017-09-25T15:49:06.983Z] 15:49:06     INFO -  reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:121:7
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:142:7
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  89 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | Extension left running at test shutdown
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  ExtensionTestUtils.loadExtension/<@chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:109:7
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1224:19
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  SimpleTest.finish@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1237:5
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  killTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:130:7
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  delayedKillTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:157:47
[task 2017-09-25T15:49:06.987Z] 15:49:06     INFO -  90 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | message queue is empty - got "[\"page-loaded\"]", expected "[]"
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  ExtensionTestUtils.loadExtension/<@chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:27:7
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1224:19
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  91 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | no tasks awaiting on messages - got "[\"background_page.ready\"]", expected "[]"
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  ExtensionTestUtils.loadExtension/<@chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:31:7
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1224:19
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  92 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | Test left extra windows or tabs: {"extraWindows":[],"extraTabs":["http://example.com/#test_activeTab_pageAction"]}
[task 2017-09-25T15:49:06.988Z] 15:49:06     INFO -  @chrome://mochitests/content/chrome/mobile/android/components/extensions/test/mochitest/head.js:27:7
[task 2017-09-25T15:49:17.385Z] 15:49:17     INFO -  93 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | took 332284ms

And more failures in other chrome tests.

Comment 49

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/20-21/

Comment 50

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8906692 [details]
Bug 1381992 - Add some reader mode support to the tabs API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178426/diff/21-22/

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2c18cfef4e3
Add some reader mode support to the tabs API, r=mixedpuppy

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 54

[Will Bamberg [:wbamberg]]

I've written:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/toggleReaderMode

...and updated:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create (for openInReaderMode)
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab (for isArticle and isInReaderMode)

One thing I thought: for the example (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/toggleReaderMode#Examples) I have some code that automatically switches every page into Reader Mode (if it is eligible).

It works, sort of, but breaks the Back button, because the original page is still in the nav history. So suppose:

(1) you load crookedtimber.org -> it's not an article, so nothing happens
(2) you navigate to http://crookedtimber.org/2017/10/30/the-week-after-open-access-week/ -> this is an article page, so you get readerified.

I think the session history is now like:

* (current) about:reader?http://crookedtimber.org/2017/10/30/the-week-after-open-access-week/
* http://crookedtimber.org/2017/10/30/the-week-after-open-access-week/
* http://crookedtimber.org

So if I go back, I go back to the article, then immediately get readerified again. So I wonder, should there be a "loadReplace" option for this API, like for https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/update? Or is there a better way to do this?

Comment 55

[:Gijs (out for now; he/him)]

Ni for Bob for comment #54 because I know that the "make it readerize everything" option is often requested so I'd like to make it work, but I don't know enough about webextensions to answer the actual question... :-)

Comment 56

[Bob Silverberg [:bsilverberg]]

Thanks for the ping, Gijs. In our current implementation we simply send the "Reader:ToggleReaderMode" message in order to toggle reader mode, so our code doesn't really understand that it may be loading a new URL. If we are to keep the existing implementation (which I prefer, so our API code doesn't have to understand the implementation of reader mode) then it sounds like this is something that would need to be addressed in the reader mode code itself, probably in ReaderMode.enterReaderMode.

Does that make sense to do?

Comment 57

[:Gijs (out for now; he/him)]

(In reply to Bob Silverberg [:bsilverberg] from comment #56)
> Thanks for the ping, Gijs. In our current implementation we simply send the
> "Reader:ToggleReaderMode" message in order to toggle reader mode, so our
> code doesn't really understand that it may be loading a new URL. If we are
> to keep the existing implementation (which I prefer, so our API code doesn't
> have to understand the implementation of reader mode) then it sounds like
> this is something that would need to be addressed in the reader mode code
> itself, probably in ReaderMode.enterReaderMode.
> 
> Does that make sense to do?

Maybe, but it'd have to be optional and supplied by the add-on, and we'd need to make sure the 'exit reader mode' functionality continues to work in that case (I believe right now it'll use history.back() so we can use bfcache if available, but maybe it's already gated on a URI check, I don't remember off-hand).

We don't want to always do this because in normal usage:

1. open page
2. click reader mode icon
3. click 'back'

the user would expect to go back to the non-reader-ized page, not go back to whatever was open before the page. That's different in the case of the add-on that's being suggested, but of course that's not the only possible usecase of reader mode (even for add-ons).

Will, can you file a followup bug? Thanks for raising this!

Comment 58

[Will Bamberg [:wbamberg]]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1414026. I wasn't sure if the summary was too prescriptive of the implementation, so feel free to fix it. Thanks!

Comment 59

[Will Bamberg [:wbamberg]]

NI for Bob to review the docs. Sorry I just realised I forgot to do that.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/toggleReaderMode
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create (for openInReaderMode)
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab (for isArticle and isInReaderMode)

Comment 60

[Bob Silverberg [:bsilverberg]]

(In reply to Will Bamberg [:wbamberg] from comment #59)
> NI for Bob to review the docs. Sorry I just realised I forgot to do that.
> 
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/
> toggleReaderMode
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create
> (for openInReaderMode)
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab (for
> isArticle and isInReaderMode)

Looks great, thanks Will.

Comment 61

[Victor Carciu]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe verify-“ flag.

Thanks!