1150174 - Use ReaderMode.isProbablyReaderable instead of full Readability parse to determine whether or not to show reader button

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[:Margaret Leibovic]

Similar to what we did for desktop in bug 1143844. This should give us some memory/performance wins on Android.

This doesn't need to be uplifted to Fx38.

Comment 1

[Justin Dolske [:Dolske]]

v2 per comment 0.

Comment 2

[:Margaret Leibovic]

Attachment: WIP - Update Android reader button logic to avoid doing full readability parse on every page load

I'm seeing some toolbar button jittery-ness happening when toggling reader mode, so I need to figure out what's going on there, but this seems to work.

Mike, I know you're not super familiar with how the reader mode logic works, but I basically took exactly what's in desktop's content.js/ReaderParent.js and ported it over to our content.js/Reader.js.

I filed bug 1153485 about making a shared place for the content.js logic, since it's identical, but we have to do different things in Reader.js, since our toolbar UI is different.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8591157 [details] [diff] [review]
WIP - Update Android reader button logic to avoid doing full readability parse on every page load

Review of attachment 8591157 [details] [diff] [review]:
-----------------------------------------------------------------

Given that I don't know the code, seems alright to me.

::: mobile/android/chrome/content/browser.js
@@ +4492,5 @@
>                         ((this.browser.lastURI != null) && fixedURI.equals(this.browser.lastURI) && !fixedURI.equals(aLocationURI));
>      this.browser.lastURI = fixedURI;
>  
> +    // Let reader mode know about this push state change.
> +    if (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT) {

What is a push state change?

Should we be checking some value in flags rather than the whole thing? Like, `flags & IS_PUSH_STATE`?

Why does a location change to the same document dictate a push state change?

Elaborate in the comments as you think necessary but please at least answer the questions once for my edification! :)

Perhaps we should also note that this mirrors the desktop code.

::: mobile/android/chrome/content/content.js
@@ -23,2 @@
>      addEventListener("pageshow", this, false);
> -    addMessageListener("Reader:SavedArticleGet", this);

So if I'm following this, the savedArticle is the parsed article (and roughly-equilavent to _articlePromise?).

@@ -49,5 @@
>  
> -        // If we are restoring multiple reader mode tabs during session restore, duplicate "DOMContentLoaded"
> -        // events may be fired for the visible tab. The inital "DOMContentLoaded" may be received before the
> -        // document body is available, so we avoid instantiating an AboutReader object, expecting that a
> -        // valid message will follow. See bug 925983.

This comment seems like it could still be useful.

Comment 4

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #3)

> ::: mobile/android/chrome/content/browser.js
> @@ +4492,5 @@
> >                         ((this.browser.lastURI != null) && fixedURI.equals(this.browser.lastURI) && !fixedURI.equals(aLocationURI));
> >      this.browser.lastURI = fixedURI;
> >  
> > +    // Let reader mode know about this push state change.
> > +    if (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT) {
> 
> What is a push state change?

This is when web developers modify the browser history without loading a new URL:
https://developer.mozilla.org/en-US/docs/Web/Guide/API/DOM/Manipulating_the_browser_history#The_pushState%28%29_method

Since this can change the content of the page, it's a good opportunity to re-check to see if we should show the reader view button.

> Should we be checking some value in flags rather than the whole thing? Like,
> `flags & IS_PUSH_STATE`?

I'm not sure what you mean by this. Like, is there a more specific flag we should check?

> Why does a location change to the same document dictate a push state change?

Good question. Looking at the docs, this could actually also happen if the user clicks an anchor:
http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgressListener.idl#333

With this new approach, checking whether or not to show the reader button is pretty low-cost, so I think it's still fine to include this logic. Then main point is that we want to catch this case where we don't get a DOMContentLoaded/pageshow event.

> Elaborate in the comments as you think necessary but please at least answer
> the questions once for my edification! :)
>
> Perhaps we should also note that this mirrors the desktop code.

I'll also add some code comments!

> ::: mobile/android/chrome/content/content.js
> @@ -23,2 @@
> >      addEventListener("pageshow", this, false);
> > -    addMessageListener("Reader:SavedArticleGet", this);
> 
> So if I'm following this, the savedArticle is the parsed article (and
> roughly-equilavent to _articlePromise?).

Yes.

Comment 5

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #2)
> Created attachment 8591157 [details] [diff] [review]
> WIP - Update Android reader button logic to avoid doing full readability
> parse on every page load
> 
> I'm seeing some toolbar button jittery-ness happening when toggling reader
> mode, so I need to figure out what's going on there, but this seems to work.

I found that this jankiness was due to redundant calls to update the page action icon to show the reader active icon. I don't see this jank with a new version of my patch.

Comment 6

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1150174/margaret

/r/8799 - Bug 1150174 - Update Android reader button logic to avoid doing full readability parse on every page load. r=mcomella

Pull down this commit:

hg pull -r 8f279cc45015b7a512f0e5e6a54e75b45f801ef3 https://reviewboard-hg.mozilla.org/gecko/

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/8799/#review8013

::: mobile/android/chrome/content/Reader.js:115
(Diff revision 1)
> -        tab.browser.isArticle = message.data.isArticle;
> +          tab.browser.isArticle = message.data.isArticle;

nit: This works, but the check for !== undefined is technically not necessary.

::: mobile/android/chrome/content/Reader.js:137
(Diff revision 1)
> -      Messaging.sendRequest({
> +      // We should use ReaderMode.getOriginalUrl when bug 1152121 is fixed.

bug 1152121 is fixed - you can use ReaderMode.getOriginalUrl here.

::: mobile/android/chrome/content/Reader.js:197
(Diff revision 1)
> -        clickCallback: () => this.pageAction.readerModeCallback(tab.id),
> +        clickCallback: () => this.pageAction.readerModeCallback(browser),

This works but I would personally merge this with the pageAction definition above to reduce duplication. You can use local variables to set the values. e.g.:

let icon;
if (browser.currentURI.spec.startsWith("about:reader") {
  icon = "drawable://reader_active";
  ... // Don't forget to do telemetry!
} else if (browser.isArticle) {
  icon = "drawable://reader";
  ...
}

if (icon) {
  this.pageAction.id = PageActions.add({
    icon: icon;
    ...
  });
}

::: mobile/android/chrome/content/content.js:19
(Diff revision 1)
> +// This is copied from desktop's content.js. See bug 1153485 about sharing this code somehow.

nit: desktop's "tab-content.js"

::: mobile/android/chrome/content/content.js:96
(Diff revision 1)
> -          sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: true });
> +      sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: true });

The latest version of tab-content.js seems to have some new lines:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tab-content.js#366

Is this something we'd want?

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/8799/#review8017

Ship It!

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8606045 [details]
MozReview Request: bz://1150174/margaret

https://reviewboard.mozilla.org/r/8797/#review8019

Ship It!

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #9)
> Ship It!

Caveat being I don't know this code super well and I'm r+'ing because of the desktop precedent.

Comment 11

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/8799/#review8083

> nit: This works, but the check for !== undefined is technically not necessary.

Ah, yeah. This was copied from desktop, where we sometimes send a "Reader:UpdateReaderButton" message without the `isArticle` property. Although I now think that that message isn't necessary...

> bug 1152121 is fixed - you can use ReaderMode.getOriginalUrl here.

Good catch, thanks.

> The latest version of tab-content.js seems to have some new lines:
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tab-content.js#366
> 
> Is this something we'd want?

Ah, yes, I just reviewed that patch. I'll update the patch here as well.

> This works but I would personally merge this with the pageAction definition above to reduce duplication. You can use local variables to set the values. e.g.:
> 
> let icon;
> if (browser.currentURI.spec.startsWith("about:reader") {
>   icon = "drawable://reader_active";
>   ... // Don't forget to do telemetry!
> } else if (browser.isArticle) {
>   icon = "drawable://reader";
>   ...
> }
> 
> if (icon) {
>   this.pageAction.id = PageActions.add({
>     icon: icon;
>     ...
>   });
> }

I decided to just create a helper function for this shared logic to avoid needing to change around where we put the telemetry probes.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8606045 [details]
MozReview Request: bz://1150174/margaret

/r/8799 - Bug 1150174 - Update Android reader button logic to avoid doing full readability parse on every page load. r=mcomella

Pull down this commit:

hg pull -r b03fd640921c6b820af97b45f6bdc69ddede568c https://reviewboard-hg.mozilla.org/gecko/

Comment 13

[:Margaret Leibovic]

Comment on attachment 8606045 [details]
MozReview Request: bz://1150174/margaret

Oh, review board. I just pushed my updated patch.

Comment 14

[Pulsebot]

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

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/8ce3581b7247

Comment 17

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1150174 - Update Android reader button logic to avoid doing full readability parse on every page load. r=mcomella