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

--HG--
extra : rebase_source : b03fd640921c6b820af97b45f6bdc69ddede568c

mobile/android/base/Tab.java

@@ -614,15 +614,6 @@ public class Tab {
         });
     }
 
-    public void toggleReaderMode() {
-        if (AboutPages.isAboutReader(mUrl)) {
-            Tabs.getInstance().loadUrl(ReaderModeUtils.getUrlFromAboutReader(mUrl));
-        } else {
-            mEnteringReaderMode = true;
-            Tabs.getInstance().loadUrl(ReaderModeUtils.getAboutReaderForUrl(mUrl, mId));
-        }
-    }
-
     public boolean isEnteringReaderMode() {
         return mEnteringReaderMode;
     }

mobile/android/base/Tabs.java

@@ -116,8 +116,7 @@ public class Tabs implements GeckoEventListener {
             "DesktopMode:Changed",
             "Tab:ViewportMetadata",
             "Tab:StreamStart",
-            "Tab:StreamStop",
-            "Reader:Toggle");
+            "Tab:StreamStop");
 
     }
 
@@ -580,8 +579,6 @@ public class Tabs implements GeckoEventListener {
             } else if (event.equals("Tab:StreamStop")) {
                 tab.setRecording(false);
                 notifyListeners(tab, TabEvents.RECORDING_CHANGE);
-            } else if (event.equals("Reader:Toggle")) {
-                tab.toggleReaderMode();
             }
 
         } catch (Exception e) {

mobile/android/chrome/content/Reader.js

@@ -52,8 +52,11 @@ let Reader = {
         break;
       }
       case "Reader:ArticleGet":
-        this._getArticle(message.data.url, message.target).then((article) => {
-          message.target.messageManager.sendAsyncMessage("Reader:ArticleData", { article: article });
+        this._getArticle(message.data.url).then((article) => {
+          // Make sure the target browser is still alive before trying to send data back.
+          if (message.target.messageManager) {
+            message.target.messageManager.sendAsyncMessage("Reader:ArticleData", { article: article });
+          }
         });
         break;
 
@@ -128,11 +131,18 @@ let Reader = {
   },
 
   pageAction: {
-    readerModeCallback: function(tabID) {
-      Messaging.sendRequest({
-        type: "Reader:Toggle",
-        tabID: tabID
-      });
+    readerModeCallback: function(browser) {
+      let url = browser.currentURI.spec;
+      if (url.startsWith("about:reader")) {
+        let originalURL = ReaderMode.getOriginalUrl(url);
+        if (!originalURL) {
+          Cu.reportError("Error finding original URL for about:reader URL: " + url);
+        } else {
+          browser.loadURI(originalURL);
+        }
+      } else {
+        browser.messageManager.sendAsyncMessage("Reader:ParseDocument", { url: url });
+      }
     },
 
     readerModeActiveCallback: function(tabID) {
@@ -151,15 +161,19 @@ let Reader = {
       delete this.pageAction.id;
     }
 
-    let browser = tab.browser;
-    if (browser.currentURI.spec.startsWith("about:reader")) {
+    let showPageAction = (icon, title) => {
       this.pageAction.id = PageActions.add({
-        title: Strings.reader.GetStringFromName("readerView.close"),
-        icon: "drawable://reader_active",
-        clickCallback: () => this.pageAction.readerModeCallback(tab.id),
+        icon: icon,
+        title: title,
+        clickCallback: () => this.pageAction.readerModeCallback(browser),
+        longClickCallback: () => this.pageAction.readerModeActiveCallback(tab.id),
         important: true
       });
+    };
 
+    let browser = tab.browser;
+    if (browser.currentURI.spec.startsWith("about:reader")) {
+      showPageAction("drawable://reader_active", Strings.reader.GetStringFromName("readerView.close"));
       // Only start a reader session if the viewer is in the foreground. We do
       // not track background reader viewers.
       UITelemetry.startSession("reader.1", null);
@@ -170,13 +184,7 @@ let Reader = {
     UITelemetry.stopSession("reader.1", "", null);
 
     if (browser.isArticle) {
-      this.pageAction.id = PageActions.add({
-        title: Strings.reader.GetStringFromName("readerView.enter"),
-        icon: "drawable://reader",
-        clickCallback: () => this.pageAction.readerModeCallback(tab.id),
-        longClickCallback: () => this.pageAction.readerModeActiveCallback(tab.id),
-        important: true
-      });
+      showPageAction("drawable://reader", Strings.reader.GetStringFromName("readerView.enter"));
     }
   },
 
@@ -227,7 +235,7 @@ let Reader = {
     }
 
     let url = tab.browser.currentURI.spec;
-    let article = yield this._getArticle(url, tab.browser).catch(e => {
+    let article = yield this._getArticle(url).catch(e => {
       Cu.reportError("Error getting article for tab: " + e);
       return null;
     });
@@ -263,22 +271,15 @@ let Reader = {
 
   /**
    * Gets an article for a given URL. This method will download and parse a document
-   * if it does not find the article in the tab data or the cache.
+   * if it does not find the article in the cache.
    *
    * @param url The article URL.
-   * @param browser The browser where the article is currently loaded.
    * @return {Promise}
    * @resolves JS object representing the article, or null if no article is found.
    */
-  _getArticle: Task.async(function* (url, browser) {
-    // First, look for a saved article.
-    let article = yield this._getSavedArticle(browser);
-    if (article && article.url == url) {
-      return article;
-    }
-
-    // Next, try to find a parsed article in the cache.
-    article = yield ReaderMode.getArticleFromCache(url);
+  _getArticle: Task.async(function* (url) {
+    // First try to find a parsed article in the cache.
+    let article = yield ReaderMode.getArticleFromCache(url);
     if (article) {
       return article;
     }
@@ -291,18 +292,6 @@ let Reader = {
     });
   }),
 
-  _getSavedArticle: function(browser) {
-    return new Promise((resolve, reject) => {
-      let mm = browser.messageManager;
-      let listener = (message) => {
-        mm.removeMessageListener("Reader:SavedArticleData", listener);
-        resolve(message.data.article);
-      };
-      mm.addMessageListener("Reader:SavedArticleData", listener);
-      mm.sendAsyncMessage("Reader:SavedArticleGet");
-    });
-  },
-
   /**
    * Migrates old indexedDB reader mode cache to new JSON cache.
    */

mobile/android/chrome/content/browser.js

@@ -4219,11 +4219,6 @@ Tab.prototype = {
 
           this.browser.addEventListener("pagehide", listener, true);
         }
-
-        if (docURI.startsWith("about:reader")) {
-          // Update the page action to show the "reader active" icon.
-          Reader.updatePageAction(this);
-        }
         break;
       }
 
@@ -4545,6 +4540,13 @@ Tab.prototype = {
                        ((this.browser.lastURI != null) && fixedURI.equals(this.browser.lastURI) && !fixedURI.equals(aLocationURI));
     this.browser.lastURI = fixedURI;
 
+    // Let the reader logic know about same document changes because we won't get a DOMContentLoaded
+    // or pageshow event, but we'll still want to update the reader view button to account for this change.
+    // This mirrors the desktop logic in TabsProgressListener.
+    if (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT) {
+      this.browser.messageManager.sendAsyncMessage("Reader:PushState", {isArticle: gBrowser.selectedBrowser.isArticle});
+    }
+
     // Reset state of click-to-play plugin notifications.
     clearTimeout(this.pluginDoorhangerTimeout);
     this.pluginDoorhangerTimeout = null;

mobile/android/chrome/content/content.js

@@ -16,19 +16,29 @@ let dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {}).AndroidLog.d.b
 
 let global = this;
 
+// This is copied from desktop's tab-content.js. See bug 1153485 about sharing this code somehow.
 let AboutReaderListener = {
-  _savedArticle: null,
+
+  _articlePromise: null,
 
   init: function() {
     addEventListener("AboutReaderContentLoaded", this, false, true);
+    addEventListener("DOMContentLoaded", this, false);
     addEventListener("pageshow", this, false);
-    addMessageListener("Reader:SavedArticleGet", this);
+    addEventListener("pagehide", this, false);
+    addMessageListener("Reader:ParseDocument", this);
+    addMessageListener("Reader:PushState", this);
   },
 
   receiveMessage: function(message) {
     switch (message.name) {
-      case "Reader:SavedArticleGet":
-        sendAsyncMessage("Reader:SavedArticleData", { article: this._savedArticle });
+      case "Reader:ParseDocument":
+        this._articlePromise = ReaderMode.parseDocument(content.document).catch(Cu.reportError);
+        content.document.location = "about:reader?url=" + encodeURIComponent(message.data.url);
+        break;
+
+      case "Reader:PushState":
+        this.updateReaderButton(!!(message.data && message.data.isArticle));
         break;
     }
   },
@@ -37,12 +47,12 @@ let AboutReaderListener = {
     return content.document.documentURI.startsWith("about:reader");
   },
 
-  handleEvent: function(event) {
-    if (event.originalTarget.defaultView != content) {
+  handleEvent: function(aEvent) {
+    if (aEvent.originalTarget.defaultView != content) {
       return;
     }
 
-    switch (event.type) {
+    switch (aEvent.type) {
       case "AboutReaderContentLoaded":
         if (!this.isAboutReader) {
           return;
@@ -53,39 +63,41 @@ let AboutReaderListener = {
         // document body is available, so we avoid instantiating an AboutReader object, expecting that a
         // valid message will follow. See bug 925983.
         if (content.document.body) {
-          new AboutReader(global, content);
+          new AboutReader(global, content, this._articlePromise);
+          this._articlePromise = null;
         }
         break;
 
+      case "pagehide":
+        sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: false });
+        break;
+
       case "pageshow":
-        if (!ReaderMode.isEnabledForParseOnLoad || this.isAboutReader) {
-          return;
+        // If a page is loaded from the bfcache, we won't get a "DOMContentLoaded"
+        // event, so we need to rely on "pageshow" in this case.
+        if (aEvent.persisted) {
+          this.updateReaderButton();
         }
-
-        // Reader mode is disabled until proven enabled.
-        this._savedArticle = null;
-        sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: false });
-
-        ReaderMode.parseDocument(content.document).then(article => {
-          // Do nothing if there is no article, or if the content window has been destroyed.
-          if (article === null || content === null) {
-            return;
-          }
-
-          // The loaded page may have changed while we were parsing the document.
-          // Make sure we've got the current one.
-          let url = Services.io.newURI(content.document.documentURI, null, null).spec;
-          if (article.url !== url) {
-            return;
-          }
-
-          this._savedArticle = article;
-          sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: true });
-
-        }).catch(e => Cu.reportError("Error parsing document: " + e));
+        break;
+      case "DOMContentLoaded":
+        this.updateReaderButton();
         break;
     }
-  }
+  },
+  updateReaderButton: function(forceNonArticle) {
+    if (!ReaderMode.isEnabledForParseOnLoad || this.isAboutReader ||
+        !(content.document instanceof content.HTMLDocument) ||
+        content.document.mozSyntheticDocument) {
+      return;
+    }
+    // Only send updates when there are articles; there's no point updating with
+    // |false| all the time.
+    if (ReaderMode.isProbablyReaderable(content.document)) {
+      sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: true });
+    } else if (forceNonArticle) {
+      sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: false });
+    }
+  },
 };
 AboutReaderListener.init();