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.

v2 per comment 0.

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 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.

(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.

(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.

/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/

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?

https://reviewboard.mozilla.org/r/8799/#review8017 Ship It!

Comment on attachment 8606045 [details] MozReview Request: bz://1150174/margaret https://reviewboard.mozilla.org/r/8797/#review8019 Ship It!

(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.

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 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 on attachment 8606045 [details] MozReview Request: bz://1150174/margaret Oh, review board. I just pushed my updated patch.