Bug 998163 - 8% Tp5 Optimized regression on inbound for most all platforms [r=adw]
authorEdward Lee <edilee@mozilla.com>
Fri, 18 Apr 2014 13:58:24 -0700
changeset 179240 5c8763cd059df9396e586d378679bc624ab73a15
parent 179239 f2e5bc417f969d31a5c1eba73cb5fef0391e36d9
child 179241 f8118ec22ed9b560f1503036a09d5a0d04ac9c41
push id6410
push useredilee@gmail.com
push dateFri, 18 Apr 2014 20:59:49 +0000
treeherderfx-team@5c8763cd059d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs998163, 998387
milestone31.0a1
Bug 998163 - 8% Tp5 Optimized regression on inbound for most all platforms [r=adw] Bug 998387 - Middle click on newtab block button blocks the site Only add click listener once at the page level and filter/delegate click handling looking for primary/middle clicks.
browser/base/content/newtab/page.js
browser/base/content/newtab/sites.js
browser/base/content/test/newtab/browser.ini
browser/base/content/test/newtab/browser_newtab_bug991111.js
browser/base/content/test/newtab/browser_newtab_bug998387.js
--- a/browser/base/content/newtab/page.js
+++ b/browser/base/content/newtab/page.js
@@ -14,19 +14,20 @@ let gPage = {
    */
   init: function Page_init() {
     // Add ourselves to the list of pages to receive notifications.
     gAllPages.register(this);
 
     // Listen for 'unload' to unregister this page.
     addEventListener("unload", this, false);
 
-    // Listen for toggle button clicks.
-    let button = document.getElementById("newtab-toggle");
-    button.addEventListener("click", this, false);
+    // XXX bug 991111 - Not all click events are correctly triggered when
+    // listening from xhtml nodes -- in particular middle clicks on sites, so
+    // listen from the xul window and filter then delegate
+    addEventListener("click", this, false);
 
     // Initialize sponsored panel
     this._sponsoredPanel = document.getElementById("sponsored-panel");
     let link = this._sponsoredPanel.querySelector(".text-link");
     link.addEventListener("click", () => this._sponsoredPanel.hidePopup());
 
     // Check if the new tab feature is enabled.
     let enabled = gAllPages.enabled;
@@ -191,17 +192,32 @@ let gPage = {
   handleEvent: function Page_handleEvent(aEvent) {
     switch (aEvent.type) {
       case "unload":
         if (this._mutationObserver)
           this._mutationObserver.disconnect();
         gAllPages.unregister(this);
         break;
       case "click":
-        gAllPages.enabled = !gAllPages.enabled;
+        let {button, target} = aEvent;
+        if (target.id == "newtab-toggle") {
+          if (button == 0) {
+            gAllPages.enabled = !gAllPages.enabled;
+          }
+          break;
+        }
+
+        // Go up ancestors until we find a Site or not
+        while (target) {
+          if (target.hasOwnProperty("_newtabSite")) {
+            target._newtabSite.onClick(aEvent);
+            break;
+          }
+          target = target.parentNode;
+        }
         break;
       case "dragover":
         if (gDrag.isValid(aEvent) && gDrag.draggedSite)
           aEvent.preventDefault();
         break;
       case "drop":
         if (gDrag.isValid(aEvent) && gDrag.draggedSite) {
           aEvent.preventDefault();
--- a/browser/base/content/newtab/sites.js
+++ b/browser/base/content/newtab/sites.js
@@ -165,20 +165,16 @@ Site.prototype = {
    * Adds event handlers for the site and its buttons.
    */
   _addEventHandlers: function Site_addEventHandlers() {
     // Register drag-and-drop event handlers.
     this._node.addEventListener("dragstart", this, false);
     this._node.addEventListener("dragend", this, false);
     this._node.addEventListener("mouseover", this, false);
 
-    // XXX bug 991111 - Not all click events are correctly triggered when
-    // listening from the xhtml node, so listen from the xul window and filter
-    addEventListener("click", this, false);
-
     // Specially treat the sponsored icon to prevent regular hover effects
     let sponsored = this._querySelector(".newtab-control-sponsored");
     sponsored.addEventListener("mouseover", () => {
       this.cell.node.setAttribute("ignorehover", "true");
     });
     sponsored.addEventListener("mouseout", () => {
       this.cell.node.removeAttribute("ignorehover");
     });
@@ -213,21 +209,29 @@ Site.prototype = {
       Services.telemetry.getHistogramById("NEWTAB_PAGE_DIRECTORY_TYPE_CLICKED")
                         .add(typeIndex);
     }
   },
 
   /**
    * Handles site click events.
    */
-  _onClick: function Site_onClick(aEvent) {
-    let target = aEvent.target;
+  onClick: function Site_onClick(aEvent) {
+    let {button, target} = aEvent;
     if (target.classList.contains("newtab-link") ||
         target.parentElement.classList.contains("newtab-link")) {
-      this._recordSiteClicked(this.cell.index);
+      // Record for primary and middle clicks
+      if (button == 0 || button == 1) {
+        this._recordSiteClicked(this.cell.index);
+      }
+      return;
+    }
+
+    // Only handle primary clicks for the remaining targets
+    if (button != 0) {
       return;
     }
 
     aEvent.preventDefault();
     if (aEvent.target.classList.contains("newtab-control-block"))
       this.block();
     else if (target.classList.contains("newtab-control-sponsored"))
       gPage.showSponsoredPanel(target);
@@ -237,23 +241,16 @@ Site.prototype = {
       this.pin();
   },
 
   /**
    * Handles all site events.
    */
   handleEvent: function Site_handleEvent(aEvent) {
     switch (aEvent.type) {
-      case "click":
-        // Check the bitmask if the click event is for the site's descendants
-        if (this._node.compareDocumentPosition(aEvent.target) &
-            this._node.DOCUMENT_POSITION_CONTAINED_BY) {
-          this._onClick(aEvent);
-        }
-        break;
       case "mouseover":
         this._node.removeEventListener("mouseover", this, false);
         this._speculativeConnect();
         break;
       case "dragstart":
         gDrag.start(this, aEvent);
         break;
       case "dragend":
--- a/browser/base/content/test/newtab/browser.ini
+++ b/browser/base/content/test/newtab/browser.ini
@@ -12,16 +12,17 @@ skip-if = e10s # Bug ?????? - about:newt
 [browser_newtab_bug734043.js]
 [browser_newtab_bug735987.js]
 skip-if = os == "mac" # Intermittent failures, bug 898317
 [browser_newtab_bug752841.js]
 [browser_newtab_bug765628.js]
 [browser_newtab_bug876313.js]
 [browser_newtab_bug991111.js]
 [browser_newtab_bug991210.js]
+[browser_newtab_bug998387.js]
 [browser_newtab_disable.js]
 [browser_newtab_drag_drop.js]
 [browser_newtab_drag_drop_ext.js]
 [browser_newtab_drop_preview.js]
 [browser_newtab_focus.js]
 [browser_newtab_perwindow_private_browsing.js]
 [browser_newtab_reset.js]
 [browser_newtab_sponsored_icon_click.js]
--- a/browser/base/content/test/newtab/browser_newtab_bug991111.js
+++ b/browser/base/content/test/newtab/browser_newtab_bug991111.js
@@ -3,17 +3,17 @@
 
 function runTests() {
   yield setLinks("0");
   yield addNewTabPageTab();
 
   // Remember if the click handler was triggered
   let cell = getCell(0);
   let clicked = false;
-  cell.site._onClick = e => {
+  cell.site.onClick = e => {
     clicked = true;
     executeSoon(TestRunner.next);
   };
 
   // Send a middle-click and make sure it happened
   yield EventUtils.synthesizeMouseAtCenter(cell.node, {button: 1}, getContentWindow());
   ok(clicked, "middle click triggered click listener");
 }
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/newtab/browser_newtab_bug998387.js
@@ -0,0 +1,23 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+function runTests() {
+  yield setLinks("0");
+  yield addNewTabPageTab();
+
+  // Remember if the click handler was triggered
+  let cell = getCell(0);
+  let clicked = false;
+  cell.site.onClick = e => {
+    clicked = true;
+    executeSoon(TestRunner.next);
+  };
+
+  // Send a middle-click and make sure it happened
+  let block = getContentDocument().querySelector(".newtab-control-block");
+  yield EventUtils.synthesizeMouseAtCenter(block, {button: 1}, getContentWindow());
+  ok(clicked, "middle click triggered click listener");
+
+  // Make sure the cell didn't actually get blocked
+  checkGrid("0");
+}