Bug 1139678 - Don't do reader parse until user clicks on reader button. r=bnicholson

browser/base/content/content.js

@@ -477,19 +477,21 @@ let AboutHomeListener = {
 AboutHomeListener.init(this);
 
 let AboutReaderListener = {
-  _savedArticle: null,
+
+  _articlePromise: null,
 
   init: function() {
     addEventListener("AboutReaderContentLoaded", this, false, true);
     addEventListener("pageshow", this, false);
     addEventListener("pagehide", this, false);
-    addMessageListener("Reader:SavedArticleGet", this);
+    addMessageListener("Reader:ParseDocument", 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);
+        content.document.location = "about:reader?url=" + encodeURIComponent(message.data.url);
         break;
     }
   },
@@ -512,7 +514,7 @@ let AboutReaderListener = {
         if (content.document.body) {
           // Update the toolbar icon to show the "reader active" icon.
           sendAsyncMessage("Reader:UpdateReaderButton");
-          new AboutReader(global, content);
+          new AboutReader(global, content, this._articlePromise);
         }
         break;
 
@@ -525,27 +527,8 @@ let AboutReaderListener = {
           return;
         }
 
-        // Reader mode is disabled until proven enabled.
-        this._savedArticle = null;
-
-        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;
+        let isArticle = ReaderMode.isProbablyReaderable(content.document);
+        sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: isArticle });
     }
   }
 };

browser/modules/ReaderParent.jsm

@@ -136,7 +136,8 @@ let ReaderParent = {
     }
 
     let win = event.target.ownerDocument.defaultView;
-    let url = win.gBrowser.selectedBrowser.currentURI.spec;
+    let browser = win.gBrowser.selectedBrowser;
+    let url = browser.currentURI.spec;
 
     if (url.startsWith("about:reader")) {
       let originalURL = this._getOriginalUrl(url);
@@ -146,7 +147,7 @@ let ReaderParent = {
         win.openUILinkIn(originalURL, "current", {"allowPinnedTabHostChange": true});
       }
     } else {
-      win.openUILinkIn("about:reader?url=" + encodeURIComponent(url), "current", {"allowPinnedTabHostChange": true});
+      browser.messageManager.sendAsyncMessage("Reader:ParseDocument", { url: url });
     }
   },
 
@@ -173,8 +174,7 @@ let ReaderParent = {
   },
 
   /**
-   * Gets an article for a given URL. This method will download and parse a document
-   * if it does not find the article in the browser data.
+   * Gets an article for a given URL. This method will download and parse a document.
    *
    * @param url The article URL.
    * @param browser The browser where the article is currently loaded.
@@ -182,26 +182,6 @@ let ReaderParent = {
    * @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;
-    }
-
-    // Article hasn't been found in the cache, we need to
-    // download the page and parse the article out of it.
     return yield ReaderMode.downloadAndParseDocument(url);
-  }),
-
-  _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");
-    });
-  }
+  })
 };

toolkit/components/reader/AboutReader.jsm

@@ -17,13 +17,9 @@ XPCOMUtils.defineLazyModuleGetter(this, "UITelemetry", "resource://gre/modules/U
 
 const READINGLIST_COMMAND_ID = "readingListSidebar";
 
-function dump(s) {
-  Services.console.logStringMessage("AboutReader: " + s);
-}
-
 let gStrings = Services.strings.createBundle("chrome://global/locale/aboutReader.properties");
 
-let AboutReader = function(mm, win) {
+let AboutReader = function(mm, win, articlePromise) {
   let doc = win.document;
 
   this._mm = mm;
@@ -37,6 +33,10 @@ let AboutReader = function(mm, win) {
 
   this._article = null;
 
+  if (articlePromise) {
+    this._articlePromise = articlePromise;
+  }
+
   this._headerElementRef = Cu.getWeakReference(doc.getElementById("reader-header"));
   this._domainElementRef = Cu.getWeakReference(doc.getElementById("reader-domain"));
   this._titleElementRef = Cu.getWeakReference(doc.getElementById("reader-title"));
@@ -558,7 +558,13 @@ AboutReader.prototype = {
     let url = this._getOriginalUrl();
     this._showProgressDelayed();
 
-    let article = yield this._getArticle(url);
+    let article;
+    if (this._articlePromise) {
+      article = yield this._articlePromise;
+    } else {
+      article = yield this._getArticle(url);
+    }
+
     if (article && article.url == url) {
       this._showContent(article);
     } else {

toolkit/components/reader/ReaderMode.jsm

@@ -61,6 +61,18 @@ this.ReaderMode = {
     }
   },
 
+  /**
+   * Decides whether or not a document is reader-able without parsing the whole thing.
+   * XXX: In the future, this should be smarter (bug 1143844).
+   *
+   * @param doc A document to parse.
+   * @return boolean Whether or not we should show the reader mode button.
+   */
+  isProbablyReaderable: function(doc) {
+    let uri = Services.io.newURI(doc.documentURI, null, null);
+    return this._shouldCheckUri(uri);
+  },
+
   /**
    * Gets an article from a loaded browser's document. This method will not attempt
    * to parse certain URIs (e.g. about: URIs).