Bob Silverberg [:bsilverberg] Assignee
	Description • 7 years ago

This is a follow up to bug 1381992 which introduced some reader mode APIs. This is for an event to be fired when a tab enters and leaves reader mode, something like tabs.onReaderModeChange.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 1 • 7 years ago

I think we only need an event for when a tab enters reader mode, because a tab always exits reader mode upon navigation, so onUpdated can work for that.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 3 • 7 years ago

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

Gijs, requesting review for the change to tab-content.js. I'm not sure if this is the best way to do this, but it seems to work. 

It doesn't seem like there's anything we can listen for to find out when a tab leaves reader mode, but that may not be an issue as I think an extension can simply use the onUpdated event and notice the URL change. If there is a way to specifically watch for leaving reader mode please let me know.

	:Gijs (he/him)
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

https://reviewboard.mozilla.org/r/184598/#review189784

::: browser/components/extensions/ext-browser.js:378
(Diff revision 1)
>          if (message.data && message.data.isArticle !== undefined) {
>            this.emit("tab-isarticle", message);
>          }

So, the browser code flips the reader mode icon to 'active' based on the URL here. You can use `message.target` to get the browser (see e.g. https://dxr.mozilla.org/mozilla-central/rev/756e10aa8bbd416cbc49b7739f78fb81d5525477/browser/modules/ReaderParent.jsm#57 ) and you can check its `currentURI` to determine whether it's just entered reader mode. I think that means you shouldn't need your own message here.

I would expect this to also work to determine when the tab left reader mode (assuming you keep state yourself on a per-tab basis).

Does that help? Or did I miss something?

	Bob Silverberg [:bsilverberg] Assignee
	Comment 6 • 7 years ago
mozreview-review-reply

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

https://reviewboard.mozilla.org/r/184598/#review189784

> So, the browser code flips the reader mode icon to 'active' based on the URL here. You can use `message.target` to get the browser (see e.g. https://dxr.mozilla.org/mozilla-central/rev/756e10aa8bbd416cbc49b7739f78fb81d5525477/browser/modules/ReaderParent.jsm#57 ) and you can check its `currentURI` to determine whether it's just entered reader mode. I think that means you shouldn't need your own message here.
> 
> I would expect this to also work to determine when the tab left reader mode (assuming you keep state yourself on a per-tab basis).
> 
> Does that help? Or did I miss something?

Thanks, that is a big help, and you're right, I don't need the extra message.

	Shane Caraveo (:mixedpuppy)
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

https://reviewboard.mozilla.org/r/184598/#review189874

Rather than two event notifications, how about we change the api slightly.   have a onReaderUpdate and the details send includes both isArticle and isReaderMode.  But I also wonder if onUpdated shouldn't be fired already on location change.

	Shane Caraveo (:mixedpuppy)
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

https://reviewboard.mozilla.org/r/184598/#review189874

addendum.  seems like these should just be in tabs.onUpdate

	Bob Silverberg [:bsilverberg] Assignee
	Comment 10 • 7 years ago
mozreview-review-reply

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

https://reviewboard.mozilla.org/r/184598/#review189874

I've updated the patch. Please let me know is this is what you meant.

	:Gijs (he/him)
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

https://reviewboard.mozilla.org/r/184598/#review190304

The integrating-with-the-browser side looks great to me, I'll leave Shane to do the "real" review of what exactly add-ons get in terms of notifications. Apologies for the slow review.

::: browser/components/extensions/ext-browser.js:380
(Diff revision 3)
> +        if (message.data === null
> +            && message.target.currentURI.spec.startsWith(READER_MODE_PREFIX)) {
> +          this.emit("tab-onreadermodeactive", message);
> +        }

So right now, I think this means we don't send an "onreadermodeactive" update and thus not necessarily a tab.onUpdated event when the user leaves reader mode, right? (but there'll be other notifications because a different page loads, I imagine) Is that intentional?

	Bob Silverberg [:bsilverberg] Assignee
	Comment 12 • 7 years ago
mozreview-review-reply

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

https://reviewboard.mozilla.org/r/184598/#review190304

> So right now, I think this means we don't send an "onreadermodeactive" update and thus not necessarily a tab.onUpdated event when the user leaves reader mode, right? (but there'll be other notifications because a different page loads, I imagine) Is that intentional?

I didn't see anything that I could listen to which would allow me to know that a tab has specifically exited reader mode, which is why that is omitted. Does such a thing exist? 

Even if we do not do that, yes, onUpdated will fire when the URL changes upon exiting reader mode, so an extension can listen for that, but I do think it would be nice to have an event to inform when reader mode is exited, if that's possible.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 13 • 7 years ago

Gijs, in case it wasn't clear, my last comment includes a question for you about how possible it is to receive a notification when a tab leaves reader mode.

	:Gijs (he/him)
	Comment 14 • 7 years ago

(In reply to Bob Silverberg [:bsilverberg] from comment #12)
> Comment on attachment 8913197 [details]
> Bug 1402921 - Add an API event for when a tab enters reader mode
> 
> https://reviewboard.mozilla.org/r/184598/#review190304
> 
> > So right now, I think this means we don't send an "onreadermodeactive" update and thus not necessarily a tab.onUpdated event when the user leaves reader mode, right? (but there'll be other notifications because a different page loads, I imagine) Is that intentional?
> 
> I didn't see anything that I could listen to which would allow me to know
> that a tab has specifically exited reader mode, which is why that is
> omitted. Does such a thing exist? 

Oops, sorry for missing this. Yeah, so, I *think* we would send another Reader:UpdateReaderButton message, at which point we're not at an about:reader URL anymore. I tried to mention this in comment #4, but I wasn't very explicit. I'm basically assuming the webextension internal implementation already has some kind of weakly mapped data store per tab for state that it could use to check 'did we think/know this tab was in reader mode'. Based on that + a Reader:UpdateReaderButton message at the point where we're no longer in reader mode, it could send another update to add-ons indicating we'd just left reader mode. Does that help?

	Bob Silverberg [:bsilverberg] Assignee
	Comment 15 • 7 years ago

Thanks Gijs, that makes sense and I can support firing an event on both enter and exit, as long as I keep track of the state of the tab.

	Shane Caraveo (:mixedpuppy)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

https://reviewboard.mozilla.org/r/184598/#review191574

::: browser/components/extensions/ext-browser.js:402
(Diff revision 4)
>          if (message.data && message.data.isArticle !== undefined) {
>            this.emit("tab-isarticle", message);
>          }
> +        if (message.data === null
> +            && message.target.currentURI.spec.startsWith(READER_MODE_PREFIX)) {
> +          this.emit("tab-onreadermodeactive", message);

We're splitting one event (UpdateReaderButton) into two.  Lets just have a tab-readerupdate event here (replace tab-isarticle and tab-onreadermodeactive).  

You could probably also massage the data being sent here rather than at the listener, but that's not really necessary.

readerModeListener in ext-tabs would then always set both isArticle and isReaderMode.

::: browser/components/extensions/ext-tabs.js:304
(Diff revision 4)
>                gBrowser.getTabForBrowser(event.target));
>  
> -            fireForTab(tab, {isArticle: event.data.isArticle});
> +            let details = {};
> +            if (eventName === "tab-isarticle") {
> +              if (!tab.url.startsWith(READER_MODE_PREFIX) && tab._wasInReaderMode) {
> +                tab._wasInReaderMode = false;

_wasInReaderMode depends on the extension always getting the notification, but it will be wrong for extensions installed during a session, if any tabs are already in reader mode.  just checking tab.isInReaderMode will always be right.

I think instead that you can just do

details.isInReaderMode = tab.url.startsWith(READER_MODE_PREFIX) ? "on" : "off", though I'm not sure we need strings, can we just use a boolean?

Also, isArticle here matches tab.isArticle, perhaps we should also match isInReaderMode so the onUpdate and the tab object are consistent.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 18 • 7 years ago
mozreview-review-reply

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

https://reviewboard.mozilla.org/r/184598/#review191574

> _wasInReaderMode depends on the extension always getting the notification, but it will be wrong for extensions installed during a session, if any tabs are already in reader mode.  just checking tab.isInReaderMode will always be right.
> 
> I think instead that you can just do
> 
> details.isInReaderMode = tab.url.startsWith(READER_MODE_PREFIX) ? "on" : "off", though I'm not sure we need strings, can we just use a boolean?
> 
> Also, isArticle here matches tab.isArticle, perhaps we should also match isInReaderMode so the onUpdate and the tab object are consistent.

I'm pretty sure this won't work without somehow keeping track of the tab's status via `_wasInReaderMode`. I understand what you're saying about tabs loaded before the extension was installed, but without someway of keeping track of which tabs were already in reader mode I don't think we have a way of firing an event *only* when a tab exits reader mode. We'll end up firing events with `isInReaderMode` == false in cases where the tab was not in reader mode to begin with.

I can't really see another way around this. Can you suggest anything?

	Shane Caraveo (:mixedpuppy)
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8913197 [details]
Bug 1402921 - Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated,

https://reviewboard.mozilla.org/r/184598/#review193326

	Bob Silverberg [:bsilverberg] Assignee
	Comment 21 • 7 years ago

It turns out that all of the information needed to track when a tab enters and exits reader mode is already available in the onUpdated event. The patch attached to this bug just enhances the test to make sure that that information is accurate.

	Pulsebot
	Comment 22 • 7 years ago

Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb216e2b9870
Enhance browser_ext_tabs_readerMode to verify that correct isInReaderMode is reported for onUpdated, r=Gijs,mixedpuppy

	Bob Silverberg [:bsilverberg] Assignee
	Comment 23 • 7 years ago

We should document how an extension can listen for a tab entering and exiting reader mode, by explaining that one can listen to onUpdated for an event with changeInfo.status == "complete" and then can check the value of tab.isInReaderMode.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 24 • 7 years ago
bugherder

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

	Will Bamberg [:wbamberg]
	Comment 25 • 7 years ago

I've added a bit on this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/toggleReaderMode

Please let me know if this covers it.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 26 • 7 years ago

Looks good Will, sorry for the delayed response.

	marius.santa
	Comment 27 • 7 years ago

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