Bug 1172270 - don't cause extra flushes for reader mode, r=margaret,smaug

--HG--
rename : browser/base/content/test/general/browser_readerMode.js => browser/base/content/test/general/browser_readerMode_hidden_nodes.js
rename : browser/base/content/test/general/readerModeArticle.html => browser/base/content/test/general/readerModeArticleHiddenNodes.html
extra : rebase_source : 184f260d55a83e866b20befda517ffe42a704002
extra : histedit_source : 4da59cfad20b681e6af20d53df03d4ed4b1ab2fb

browser/base/content/tab-content.js

@@ -337,6 +337,7 @@ let AboutReaderListener = {
         break;
 
       case "pagehide":
+        this.cancelPotentialPendingReadabilityCheck();
         sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: false });
         break;
 
@@ -353,12 +354,42 @@ let AboutReaderListener = {
 
     }
   },
+
+  /**
+   * NB: this function will update the state of the reader button asynchronously
+   * after the next mozAfterPaint call (assuming reader mode is enabled and 
+   * this is a suitable document). Calling it on things which won't be
+   * painted is not going to work.
+   */
   updateReaderButton: function(forceNonArticle) {
     if (!ReaderMode.isEnabledForParseOnLoad || this.isAboutReader ||
         !(content.document instanceof content.HTMLDocument) ||
         content.document.mozSyntheticDocument) {
       return;
     }
+
+    this.scheduleReadabilityCheckPostPaint(forceNonArticle);
+  },
+
+  cancelPotentialPendingReadabilityCheck: function() {
+    if (this._pendingReadabilityCheck) {
+      removeEventListener("MozAfterPaint", this._pendingReadabilityCheck);
+      delete this._pendingReadabilityCheck;
+    }
+  },
+
+  scheduleReadabilityCheckPostPaint: function(forceNonArticle) {
+    if (this._pendingReadabilityCheck) {
+      // We need to stop this check before we re-add one because we don't know
+      // if forceNonArticle was true or false last time.
+      this.cancelPotentialPendingReadabilityCheck();
+    }
+    this._pendingReadabilityCheck = this.onPaintWhenWaitedFor.bind(this, forceNonArticle);
+    addEventListener("MozAfterPaint", this._pendingReadabilityCheck);
+  },
+
+  onPaintWhenWaitedFor: function(forceNonArticle) {
+    this.cancelPotentialPendingReadabilityCheck();
     // Only send updates when there are articles; there's no point updating with
     // |false| all the time.
     if (ReaderMode.isProbablyReaderable(content.document)) {

browser/base/content/test/general/browser.ini

@@ -478,6 +478,9 @@ skip-if = e10s # bug 1100687 - test directly manipulates content (content.docume
 [browser_readerMode.js]
 support-files =
   readerModeArticle.html
+[browser_readerMode_hidden_nodes.js]
+support-files =
+  readerModeArticleHiddenNodes.html
 [browser_bug1124271_readerModePinnedTab.js]
 support-files =
   readerModeArticle.html

browser/base/content/test/general/browser_readerMode_hidden_nodes.js

@@ -0,0 +1,45 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/**
+ * Test that the reader mode button appears and works properly on
+ * reader-able content, and that ReadingList button can open and close
+ * its Sidebar UI.
+ */
+const TEST_PREFS = [
+  ["reader.parse-on-load.enabled", true],
+  ["browser.reader.detectedFirstArticle", false],
+];
+
+const TEST_PATH = "http://example.com/browser/browser/base/content/test/general/";
+
+let readerButton = document.getElementById("reader-mode-button");
+
+add_task(function* test_reader_button() {
+  registerCleanupFunction(function() {
+    // Reset test prefs.
+    TEST_PREFS.forEach(([name, value]) => {
+      Services.prefs.clearUserPref(name);
+    });
+    while (gBrowser.tabs.length > 1) {
+      gBrowser.removeCurrentTab();
+    }
+  });
+
+  // Set required test prefs.
+  TEST_PREFS.forEach(([name, value]) => {
+    Services.prefs.setBoolPref(name, value);
+  });
+
+  let tab = gBrowser.selectedTab = gBrowser.addTab();
+  is_element_hidden(readerButton, "Reader mode button is not present on a new tab");
+  // Point tab to a test page that is not reader-able due to hidden nodes.
+  let url = TEST_PATH + "readerModeArticleHiddenNodes.html";
+  yield promiseTabLoadEvent(tab, url);
+  yield ContentTask.spawn(tab.linkedBrowser, "", function() {
+    return ContentTaskUtils.waitForEvent(content, "MozAfterPaint");
+  });
+
+  is_element_hidden(readerButton, "Reader mode button is still not present on tab with unreadable content.");
+});

browser/base/content/test/general/readerModeArticleHiddenNodes.html

@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>Article title</title>
+<meta name="description" content="This is the article description." />
+</head>
+<body>
+<style>
+p { display: none }
+</style>
+<header>Site header</header>
+<div>
+<h1>Article title</h1>
+<h2 class="author">by Jane Doe</h2>
+<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec a diam lectus. Sed sit amet ipsum mauris. Maecenas congue ligula ac quam viverra nec consectetur ante hendrerit. Donec et mollis dolor. Praesent et diam eget libero egestas mattis sit amet vitae augue. Nam tincidunt congue enim, ut porta lorem lacinia consectetur. Donec ut libero sed arcu vehicula ultricies a non tortor. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Aenean ut gravida lorem. Ut turpis felis, pulvinar a semper sed, adipiscing id dolor. Pellentesque auctor nisi id magna consequat sagittis. Curabitur dapibus enim sit amet elit pharetra tincidunt feugiat nisl imperdiet. Ut convallis libero in urna ultrices accumsan. Donec sed odio eros. Donec viverra mi quis quam pulvinar at malesuada arcu rhoncus. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. In rutrum accumsan ultricies. Mauris vitae nisi at sem facilisis semper ac in est.</p>
+<p>Vivamus fermentum semper porta. Nunc diam velit, adipiscing ut tristique vitae, sagittis vel odio. Maecenas convallis ullamcorper ultricies. Curabitur ornare, ligula semper consectetur sagittis, nisi diam iaculis velit, id fringilla sem nunc vel mi. Nam dictum, odio nec pretium volutpat, arcu ante placerat erat, non tristique elit urna et turpis. Quisque mi metus, ornare sit amet fermentum et, tincidunt et orci. Fusce eget orci a orci congue vestibulum. Ut dolor diam, elementum et vestibulum eu, porttitor vel elit. Curabitur venenatis pulvinar tellus gravida ornare. Sed et erat faucibus nunc euismod ultricies ut id justo. Nullam cursus suscipit nisi, et ultrices justo sodales nec. Fusce venenatis facilisis lectus ac semper. Aliquam at massa ipsum. Quisque bibendum purus convallis nulla ultrices ultricies. Nullam aliquam, mi eu aliquam tincidunt, purus velit laoreet tortor, viverra pretium nisi quam vitae mi. Fusce vel volutpat elit. Nam sagittis nisi dui.</p>
+<p>Vivamus fermentum semper porta. Nunc diam velit, adipiscing ut tristique vitae, sagittis vel odio. Maecenas convallis ullamcorper ultricies. Curabitur ornare, ligula semper consectetur sagittis, nisi diam iaculis velit, id fringilla sem nunc vel mi. Nam dictum, odio nec pretium volutpat, arcu ante placerat erat, non tristique elit urna et turpis. Quisque mi metus, ornare sit amet fermentum et, tincidunt et orci. Fusce eget orci a orci congue vestibulum. Ut dolor diam, elementum et vestibulum eu, porttitor vel elit. Curabitur venenatis pulvinar tellus gravida ornare. Sed et erat faucibus nunc euismod ultricies ut id justo. Nullam cursus suscipit nisi, et ultrices justo sodales nec. Fusce venenatis facilisis lectus ac semper. Aliquam at massa ipsum. Quisque bibendum purus convallis nulla ultrices ultricies. Nullam aliquam, mi eu aliquam tincidunt, purus velit laoreet tortor, viverra pretium nisi quam vitae mi. Fusce vel volutpat elit. Nam sagittis nisi dui.</p>
+<p>Vivamus fermentum semper porta. Nunc diam velit, adipiscing ut tristique vitae, sagittis vel odio. Maecenas convallis ullamcorper ultricies. Curabitur ornare, ligula semper consectetur sagittis, nisi diam iaculis velit, id fringilla sem nunc vel mi. Nam dictum, odio nec pretium volutpat, arcu ante placerat erat, non tristique elit urna et turpis. Quisque mi metus, ornare sit amet fermentum et, tincidunt et orci. Fusce eget orci a orci congue vestibulum. Ut dolor diam, elementum et vestibulum eu, porttitor vel elit. Curabitur venenatis pulvinar tellus gravida ornare. Sed et erat faucibus nunc euismod ultricies ut id justo. Nullam cursus suscipit nisi, et ultrices justo sodales nec. Fusce venenatis facilisis lectus ac semper. Aliquam at massa ipsum. Quisque bibendum purus convallis nulla ultrices ultricies. Nullam aliquam, mi eu aliquam tincidunt, purus velit laoreet tortor, viverra pretium nisi quam vitae mi. Fusce vel volutpat elit. Nam sagittis nisi dui.</p>
+<p>Vivamus fermentum semper porta. Nunc diam velit, adipiscing ut tristique vitae, sagittis vel odio. Maecenas convallis ullamcorper ultricies. Curabitur ornare, ligula semper consectetur sagittis, nisi diam iaculis velit, id fringilla sem nunc vel mi. Nam dictum, odio nec pretium volutpat, arcu ante placerat erat, non tristique elit urna et turpis. Quisque mi metus, ornare sit amet fermentum et, tincidunt et orci. Fusce eget orci a orci congue vestibulum. Ut dolor diam, elementum et vestibulum eu, porttitor vel elit. Curabitur venenatis pulvinar tellus gravida ornare. Sed et erat faucibus nunc euismod ultricies ut id justo. Nullam cursus suscipit nisi, et ultrices justo sodales nec. Fusce venenatis facilisis lectus ac semper. Aliquam at massa ipsum. Quisque bibendum purus convallis nulla ultrices ultricies. Nullam aliquam, mi eu aliquam tincidunt, purus velit laoreet tortor, viverra pretium nisi quam vitae mi. Fusce vel volutpat elit. Nam sagittis nisi dui.</p>
+</div>
+</body>
+</html>

testing/mochitest/BrowserTestUtils/ContentTaskUtils.jsm

@@ -66,5 +66,55 @@ this.ContentTaskUtils = {
         tries++;
       }, interval);
     });
-  }
+  },
+
+  /**
+   * Waits for an event to be fired on a specified element.
+   *
+   * Usage:
+   *    let promiseEvent = ContentTasKUtils.waitForEvent(element, "eventName");
+   *    // Do some processing here that will cause the event to be fired
+   *    // ...
+   *    // Now yield until the Promise is fulfilled
+   *    let receivedEvent = yield promiseEvent;
+   *
+   * @param {Element} subject
+   *        The element that should receive the event.
+   * @param {string} eventName
+   *        Name of the event to listen to.
+   * @param {bool} capture [optional]
+   *        True to use a capturing listener.
+   * @param {function} checkFn [optional]
+   *        Called with the Event object as argument, should return true if the
+   *        event is the expected one, or false if it should be ignored and
+   *        listening should continue. If not specified, the first event with
+   *        the specified name resolves the returned promise.
+   *
+   * @note Because this function is intended for testing, any error in checkFn
+   *       will cause the returned promise to be rejected instead of waiting for
+   *       the next event, since this is probably a bug in the test.
+   *
+   * @returns {Promise}
+   * @resolves The Event object.
+   */
+  waitForEvent(subject, eventName, capture, checkFn) {
+    return new Promise((resolve, reject) => {
+      subject.addEventListener(eventName, function listener(event) {
+        try {
+          if (checkFn && !checkFn(event)) {
+            return;
+          }
+          subject.removeEventListener(eventName, listener, capture);
+          resolve(event);
+        } catch (ex) {
+          try {
+            subject.removeEventListener(eventName, listener, capture);
+          } catch (ex2) {
+            // Maybe the provided object does not support removeEventListener.
+          }
+          reject(ex);
+        }
+      }, capture);
+    });
+  },
 };

toolkit/components/reader/ReaderMode.jsm

@@ -132,20 +132,11 @@ this.ReaderMode = {
     // We pass in a helper function to determine if a node is visible, because
     // it uses gecko APIs that the engine-agnostic readability code can't rely
     // upon.
-    // NB: we need to do a flush the first time we call this, so we keep track of
-    // this using a property:
-    this._needFlushForVisibilityCheck = true;
     return new Readability(uri, doc).isProbablyReaderable(this.isNodeVisible.bind(this, utils));
   },
 
   isNodeVisible: function(utils, node) {
-    let bounds;
-    if (this._needFlushForVisibilityCheck) {
-      bounds = node.getBoundingClientRect();
-      this._needFlushForVisibilityCheck = false;
-    } else {
-      bounds = utils.getBoundsWithoutFlushing(node);
-    }
+    let bounds = utils.getBoundsWithoutFlushing(node);
     return bounds.height > 0 && bounds.width > 0;
   },