Follow-up to bug 793920. This will also require some changes on the Android side of things to make sure we don't lose any functionality. I would also like to take this opportunity to start e10s-ifying aboutReader.js.

/r/1693 - Bug 1111142 - (Part 1) Turn aboutReader.js into an AboutReader module /r/1695 - Bug 1111142 - (Part 2) Load AboutReader in a frame script on Fennec /r/1697 - Bug 1111142 - (Part 3) Replace fennec-specific logic in AboutReader.jsm with messages Pull down these commits: hg pull review -r 7892284aa1dffdf62006426e22f16eddfcc408ea

I posed a WIP in case anyone is curious how this is going. The last patch still needs some more work, but the end is in sight! I think the biggest challenge will be figuring out how to do the fallback download-and-parse logic. I'm contemplating moving all the logic to find an article out of AboutReader.jsm, and instead just always pass an article in when creating a new AboutReader instance. That way Reader/ReaderMode could just be in charge of the downloading/parsing/caching, and AboutReader can be a dumb UI for the article.

I'm having some trouble with review board, so doing this the old way.

Sorry this turned into such a big patch... it turned out there was a lot of Fennec-specific stuff that needed to be reworked. With these patches applied, there should be no regressions in Fennec. I haven't tried hooking this up to desktop yet, but I don't think it's far away from working. I'd be happy to talk more about how this all works, but basically the idea is that we have a frame script (content.js) that handles the parse-on-load logic and the logic to create a new AboutReader instance. AboutReader is designed to live in the content process (if e10s is enabled) and send messages to the parent process to interact with browser chrome. For Fennec, Reader.js handles these messages, but for desktop I'll need to make a similar (but different) implementation.

Comment on attachment 8542283 [details] [diff] [review] (Part 2) Load AboutReader in a frame script on Fennec >+ window.messageManager.loadFrameScript("chrome://browser/content/content.js", true); You originally tried using window.getGroupMessageManager("browsers").loadFrameScript(...), but that was failing. I think that's because we weren't adding message="true" and groupmessagemanager="browsers" to the <browser>s we create in Tab.createBrowser See: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1582 Might be good to try that again (in a followup or here) to keep the messages from polluting other <browser> objects we might create. >diff --git a/mobile/android/chrome/content/content.js b/mobile/android/chrome/content/content.js >+ handleEvent: function(aEvent) { Opportunity to drop the "a" prefix ? >+ if (!aEvent.originalTarget.documentURI.startsWith("about:reader")) { >+ return; >+ } I guess this is a good defensive check, even if we only catch events from aboutReader.js >diff --git a/toolkit/components/reader/AboutReader.jsm b/toolkit/components/reader/AboutReader.jsm >+XPCOMUtils.defineLazyGetter(this, "gChromeWin", () => Services.wm.getMostRecentWindow("navigator:browser")); Thankfully, you are killing this in the next patch. We really wouldn't want this used for realz.

Comment on attachment 8542286 [details] [diff] [review] (Part 3) Replace fennec-specific logic in AboutReader.jsm with messages >diff --git a/mobile/android/chrome/content/Reader.js b/mobile/android/chrome/content/Reader.js >+ receiveMessage: function(message) { >+ case "Reader:FaviconRequest": { >+ let observer = (s, t, d) => { >+ Services.obs.removeObserver(observer, "Reader:FaviconReturn", false); >+ message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", JSON.parse(d)); >+ }; >+ Services.obs.addObserver(observer, "Reader:FaviconReturn", false); >+ Messaging.sendRequest({ >+ type: "Reader:FaviconRequest", >+ url: message.data.url >+ }); I think you want Messaging.sendRequestForResult, like here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/content/aboutReader.js#287 (which is right below here) That way you can skip the explicit observer. >+ case "Reader:ShowToast": >+ NativeWindow.toast.show(message.data.toast, "short"); I am sad that we still don't have a Toast.jsm yet!!! >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+// XXX: Make this into a module? >+Services.scriptloader.loadSubScript("chrome://browser/content/Reader.js", this); I guess it makes no sense to lazy load this? > > // Lazily-loaded browser scripts: >diff --git a/mobile/android/chrome/content/content.js b/mobile/android/chrome/content/content.js >--- a/mobile/android/chrome/content/content.js >+++ b/mobile/android/chrome/content/content.js >@@ -4,35 +4,82 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > let { classes: Cc, interfaces: Ci, utils: Cu } = Components; > > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > XPCOMUtils.defineLazyModuleGetter(this, "AboutReader", "resource://gre/modules/AboutReader.jsm"); >+XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode", "resource://gre/modules/ReaderMode.jsm"); > > let dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {}).AndroidLog.d.bind(null, "Content"); > >+let mm = this; >+ > let AboutReaderListener = { >+ _savedArticle: null, >+ > init: function(chromeGlobal) { if mm is now the chromeGlobal, why not kill the "chromeGlobal" and just use "mm" ? Looks like my Messaging.sendRequestForResult may be moot now too.

Comment on attachment 8542283 [details] [diff] [review] (Part 2) Load AboutReader in a frame script on Fennec Review of attachment 8542283 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/content.js @@ +26,5 @@ > + case "AboutReaderContentLoaded": > + // During browser restart / recovery, duplicate "DOMContentLoaded" messages are received here > + // For the visible tab ... where more than one tab is being reloaded, the inital "DOMContentLoaded" > + // Message can be received before the document body is available ... so we avoid instantiating an > + // AboutReader object, expecting that an eventual valid message will follow. Also a good opportunity to fix this comment up -- the formatting/broken sentences are a bit distracting.

Comment on attachment 8542286 [details] [diff] [review] (Part 3) Replace fennec-specific logic in AboutReader.jsm with messages Review of attachment 8542286 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/Reader.js @@ +54,5 @@ > + case "Gesture:DoubleTap": { > + // XXX: Ideally, we would just do this all with web APIs in AboutReader.jsm. > + let win = BrowserApp.selectedBrowser.contentWindow; > + let scrollBy; > + // Arbitary choice of innerHeight - 50 to give some context after scroll Nit while here: Arbitary -> Arbitrary @@ +94,5 @@ > + url: message.data.url > + }); > + break; > + } > + case "Reader:ListStatusRequest": Nit: New line above case for consistency. ::: mobile/android/chrome/content/browser.js @@ +3565,5 @@ > BrowserApp.deck.removeChild(this.browser); > BrowserApp.deck.selectedPanel = selectedPanel; > > this.browser = null; > + this.isArticle = null; null -> false ::: mobile/android/chrome/content/content.js @@ +13,4 @@ > > let dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {}).AndroidLog.d.bind(null, "Content"); > > +let mm = this; mm is unused. Expanding on Mark's comment, you should just be able to do addEventListener() below (without even using mm) since this is the global scope. ::: toolkit/components/reader/ReaderMode.jsm @@ +30,5 @@ > + get isEnabledForParseOnLoad() { > + delete this.isEnabledForParseOnLoad; > + > + // Listen for future pref changes. > + Services.prefs.addObserver("reader.parse-on-load.", this, false); Should there also be a pref observer for reader.parse-on-load.force-enabled?

(In reply to Mark Finkle (:mfinkle) from comment #7) > Comment on attachment 8542283 [details] [diff] [review] > (Part 2) Load AboutReader in a frame script on Fennec > > >+ window.messageManager.loadFrameScript("chrome://browser/content/content.js", true); > > You originally tried using > window.getGroupMessageManager("browsers").loadFrameScript(...), but that was > failing. I think that's because we weren't adding message="true" and > groupmessagemanager="browsers" to the <browser>s we create in > Tab.createBrowser > > See: > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/ > tabbrowser.xml#1582 > > Might be good to try that again (in a followup or here) to keep the messages > from polluting other <browser> objects we might create. Good catch. I'll file a follow-up for this, to avoid adding even more complexity to this bug. > >diff --git a/mobile/android/chrome/content/content.js b/mobile/android/chrome/content/content.js > > >+ handleEvent: function(aEvent) { > > Opportunity to drop the "a" prefix ? Good call. Too much copy-pasta. > >diff --git a/toolkit/components/reader/AboutReader.jsm b/toolkit/components/reader/AboutReader.jsm > > >+XPCOMUtils.defineLazyGetter(this, "gChromeWin", () => Services.wm.getMostRecentWindow("navigator:browser")); > > Thankfully, you are killing this in the next patch. We really wouldn't want > this used for realz. Fo' realz.(In reply to Brian Nicholson (:bnicholson) from comment #9) > Comment on attachment 8542283 [details] [diff] [review] > (Part 2) Load AboutReader in a frame script on Fennec > > Review of attachment 8542283 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/chrome/content/content.js > @@ +26,5 @@ > > + case "AboutReaderContentLoaded": > > + // During browser restart / recovery, duplicate "DOMContentLoaded" messages are received here > > + // For the visible tab ... where more than one tab is being reloaded, the inital "DOMContentLoaded" > > + // Message can be received before the document body is available ... so we avoid instantiating an > > + // AboutReader object, expecting that an eventual valid message will follow. > > Also a good opportunity to fix this comment up -- the formatting/broken > sentences are a bit distracting. Will do.

(In reply to Mark Finkle (:mfinkle) from comment #8) > Comment on attachment 8542286 [details] [diff] [review] > (Part 3) Replace fennec-specific logic in AboutReader.jsm with messages > > >diff --git a/mobile/android/chrome/content/Reader.js b/mobile/android/chrome/content/Reader.js > > >+ receiveMessage: function(message) { > > >+ case "Reader:FaviconRequest": { > >+ let observer = (s, t, d) => { > >+ Services.obs.removeObserver(observer, "Reader:FaviconReturn", false); > >+ message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", JSON.parse(d)); > >+ }; > >+ Services.obs.addObserver(observer, "Reader:FaviconReturn", false); > >+ Messaging.sendRequest({ > >+ type: "Reader:FaviconRequest", > >+ url: message.data.url > >+ }); > > I think you want Messaging.sendRequestForResult, like here: > > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ > content/aboutReader.js#287 (which is right below here) > > That way you can skip the explicit observer. I will file a follow-up for this, since this will require changing the Java side as well, and we already have enough changes going on here. > >+ case "Reader:ShowToast": > >+ NativeWindow.toast.show(message.data.toast, "short"); > > I am sad that we still don't have a Toast.jsm yet!!! Sad times indeed. > >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js > > >+// XXX: Make this into a module? > >+Services.scriptloader.loadSubScript("chrome://browser/content/Reader.js", this); > > I guess it makes no sense to lazy load this? I decided to get rid of the lazy loading because I need to register the message listeners, but maybe we could find a way to lazily do that, similarly to what we do for notifications. Since content.js is doing the parse-on-load business now, we really don't need Reader.js to wake up until we find an article or load about:reader. I could also file a follow-up bug to investigate this, it might be a good optimization for memory-constrained devices where parse-on-load is disabled. > >diff --git a/mobile/android/chrome/content/content.js b/mobile/android/chrome/content/content.js > >--- a/mobile/android/chrome/content/content.js > >+++ b/mobile/android/chrome/content/content.js > >@@ -4,35 +4,82 @@ > > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > let { classes: Cc, interfaces: Ci, utils: Cu } = Components; > > > > Cu.import("resource://gre/modules/Services.jsm"); > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > > > XPCOMUtils.defineLazyModuleGetter(this, "AboutReader", "resource://gre/modules/AboutReader.jsm"); > >+XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode", "resource://gre/modules/ReaderMode.jsm"); > > > > let dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {}).AndroidLog.d.bind(null, "Content"); > > > >+let mm = this; > >+ > > let AboutReaderListener = { > >+ _savedArticle: null, > >+ > > init: function(chromeGlobal) { > > if mm is now the chromeGlobal, why not kill the "chromeGlobal" and just use > "mm" ? I copied this from desktop's AboutHomeListener, but now I see that desktop's content.js also has a 'global' variable. I'll replace 'mm' with 'global', and then get rid of the 'chromeGlobal' parameter. (In reply to Brian Nicholson (:bnicholson) from comment #10) > ::: mobile/android/chrome/content/browser.js > ::: mobile/android/chrome/content/content.js > @@ +13,4 @@ > > > > let dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {}).AndroidLog.d.bind(null, "Content"); > > > > +let mm = this; > > mm is unused. Expanding on Mark's comment, you should just be able to do > addEventListener() below (without even using mm) since this is the global > scope. No, it's used in the AboutReader constructor, since it needs a reference to a message manager. I'll look into doing addEventListener without an explicit message manager, but it's strange that desktop does this if it's not necessary. > ::: toolkit/components/reader/ReaderMode.jsm > @@ +30,5 @@ > > + get isEnabledForParseOnLoad() { > > + delete this.isEnabledForParseOnLoad; > > + > > + // Listen for future pref changes. > > + Services.prefs.addObserver("reader.parse-on-load.", this, false); > > Should there also be a pref observer for reader.parse-on-load.force-enabled? Yeah, I believe that will be fixed when I rebase this on top of my changes from bug 1116231. (I now really appreciate review board issues, it's much easier to follow up on review comments there!)

https://hg.mozilla.org/integration/fx-team/rev/6b480b80299a https://hg.mozilla.org/integration/fx-team/rev/e66b9aa4b22c https://hg.mozilla.org/integration/fx-team/rev/9b62d0e8b412

Backed out in https://hg.mozilla.org/integration/fx-team/rev/156a06b49da8 for having a rather startling perf impact (and you know it's startling if I even notice it at all). The part I saw was that it pushed half the reftest/crashtest/jsrefest hunks up over an hour, which apparently causes them to kill themselves complaining of "application ran for longer than allowed maximum time," but if you look at the trobocheck2 graph, whatever it is that it measures went through the roof.

I made a try run with my patch for bug 1117224, and that seemed to fix the reftest problems, but there is still a big trobocheck2 regression. I'll have to dig into that to see what's going wrong. https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc61af3da5b3

I made try runs with my patches from bug 1117224 and bug 1117228, and those were green (and the tcheck regression was fixed). https://hg.mozilla.org/integration/fx-team/rev/4118e045b464 https://hg.mozilla.org/integration/fx-team/rev/931fc7ae771e https://hg.mozilla.org/integration/fx-team/rev/93bca334a998

https://hg.mozilla.org/mozilla-central/rev/4118e045b464 https://hg.mozilla.org/mozilla-central/rev/931fc7ae771e https://hg.mozilla.org/mozilla-central/rev/93bca334a998