Bug 1396053 - Page action urlbar button ordering can get messed up for new windows. r=Gijs
authorDrew Willcoxon <adw@mozilla.com>
Mon, 04 Sep 2017 23:58:27 -0700
changeset 378850 2efc7d0accca
parent 378849 eb3853161334
child 378851 c884cca7be4a
push id50412
push userdwillcoxon@mozilla.com
push dateTue, 05 Sep 2017 07:00:58 +0000
treeherderautoland@2efc7d0accca [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1396053
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1396053 - Page action urlbar button ordering can get messed up for new windows. r=Gijs MozReview-Commit-ID: 1KwrS6tsnhO
browser/base/content/browser-pageActions.js
browser/modules/PageActions.jsm
browser/modules/test/browser/browser_PageActions.js
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -42,51 +42,64 @@ var BrowserPageActions = {
     delete this.mainViewBodyNode;
     return this.mainViewBodyNode = this.mainViewNode.querySelector(".panel-subview-body");
   },
 
   /**
    * Inits.  Call to init.
    */
   init() {
+    this.placeAllActions();
+  },
+
+  /**
+   * Places all registered actions.
+   */
+  placeAllActions() {
+    // Place actions in the panel.  Notify of onBeforePlacedInWindow too.
     for (let action of PageActions.actions) {
-      this.placeAction(action,
-                       PageActions.insertBeforeActionIDInPanel(action),
-                       PageActions.insertBeforeActionIDInUrlbar(action));
+      action.onBeforePlacedInWindow(window);
+      this.placeActionInPanel(action);
+    }
+
+    // Place actions in the urlbar.  Do this in reverse order.  The reason is
+    // subtle.  If there were no urlbar nodes already in markup (like the
+    // bookmark star button), then doing this in forward order would be fine.
+    // Forward order means that the insert-before relationship is always broken:
+    // there's never a next-sibling node before which to insert a new node, so
+    // node.insertBefore() is always passed null, and nodes are always appended.
+    // That will break the position of nodes that should be inserted before
+    // nodes that are in markup, which in turn can break other nodes.
+    let actionsInUrlbar = PageActions.actionsInUrlbar;
+    for (let i = actionsInUrlbar.length - 1; i >= 0; i--) {
+      let action = actionsInUrlbar[i];
+      this.placeActionInUrlbar(action);
     }
   },
 
   /**
    * Adds or removes as necessary DOM nodes for the given action.
    *
    * @param  action (PageActions.Action, required)
    *         The action to place.
-   * @param  panelInsertBeforeID (string, required)
-   *         The ID of the action in the panel before which the given action
-   *         action should be inserted.
-   * @param  urlbarInsertBeforeID (string, required)
-   *         If the action is shown in the urlbar, then this is ID of the action
-   *         in the urlbar before which the given action should be inserted.
    */
-  placeAction(action, panelInsertBeforeID, urlbarInsertBeforeID) {
+  placeAction(action) {
     action.onBeforePlacedInWindow(window);
-    this.placeActionInPanel(action, panelInsertBeforeID);
-    this.placeActionInUrlbar(action, urlbarInsertBeforeID);
+    this.placeActionInPanel(action);
+    this.placeActionInUrlbar(action);
   },
 
   /**
    * Adds or removes as necessary DOM nodes for the action in the panel.
    *
    * @param  action (PageActions.Action, required)
    *         The action to place.
-   * @param  insertBeforeID (string, required)
-   *         The ID of the action in the panel before which the given action
-   *         action should be inserted.  Pass null to append.
    */
-  placeActionInPanel(action, insertBeforeID) {
+  placeActionInPanel(action) {
+    let insertBeforeID = PageActions.nextActionID(action, PageActions.actions);
     let id = this._panelButtonNodeIDForActionID(action.id);
     let node = document.getElementById(id);
     if (!node) {
       let panelViewNode;
       [node, panelViewNode] = this._makePanelButtonNodeForAction(action);
       node.id = id;
       let insertBeforeNode = null;
       if (insertBeforeID) {
@@ -280,21 +293,20 @@ var BrowserPageActions = {
     return "pageActionActivatedActionPanel";
   },
 
   /**
    * Adds or removes as necessary a DOM node for the given action in the urlbar.
    *
    * @param  action (PageActions.Action, required)
    *         The action to place.
-   * @param  insertBeforeID (string, required)
-   *         If the action is shown in the urlbar, then this is ID of the action
-   *         in the urlbar before which the given action should be inserted.
    */
-  placeActionInUrlbar(action, insertBeforeID) {
+  placeActionInUrlbar(action) {
+    let insertBeforeID =
+      PageActions.nextActionID(action, PageActions.actionsInUrlbar);
     let id = this._urlbarButtonNodeIDForActionID(action.id);
     let node = document.getElementById(id);
 
     if (!action.shownInUrlbar) {
       if (node) {
         if (action.__urlbarNodeInMarkup) {
           node.hidden = true;
         } else {
--- a/browser/modules/PageActions.jsm
+++ b/browser/modules/PageActions.jsm
@@ -35,23 +35,32 @@ this.PageActions = {
    * Inits.  Call to init.
    */
   init() {
     let callbacks = this._deferredAddActionCalls;
     delete this._deferredAddActionCalls;
 
     this._loadPersistedActions();
 
-    // Add the built-in actions, which are defined below in this file.
+    // Register the built-in actions, which are defined below in this file.
     for (let options of gBuiltInActions) {
       if (!this.actionForID(options.id)) {
-        this.addAction(new Action(options));
+        this._registerAction(new Action(options));
       }
     }
 
+    // Now place them all in each window.  Instead of splitting the register and
+    // place steps, we could simply call addAction, which does both, but doing
+    // it this way means that all windows initially place their actions the same
+    // way -- placeAllActions -- regardless of whether they're open when this
+    // method is called or opened later.
+    for (let bpa of allBrowserPageActions()) {
+      bpa.placeAllActions();
+    }
+
     // These callbacks are deferred until init happens and all built-in actions
     // are added.
     while (callbacks && callbacks.length) {
       callbacks.shift()();
     }
   },
 
   _deferredAddActionCalls: [],
@@ -90,16 +99,37 @@ this.PageActions = {
   /**
    * The list of non-built-in actions.  Not live.  (array of Action objects)
    */
   get nonBuiltInActions() {
     return this._nonBuiltInActions.slice();
   },
 
   /**
+   * The list of actions in the urlbar, sorted in the order in which they should
+   * appear there.  Not live.  (array of Action objects)
+   */
+  get actionsInUrlbar() {
+    if (!this._persistedActions) {
+      // This is the case before init is called.  No one should be calling us
+      // then, but return something sensible.
+      return [];
+    }
+    // Remember that IDs in idsInUrlbar may belong to actions that aren't
+    // currently registered.
+    return this._persistedActions.idsInUrlbar.reduce((actions, id) => {
+      let action = this.actionForID(id);
+      if (action) {
+        actions.push(action);
+      }
+      return actions;
+    }, []);
+  },
+
+  /**
    * Gets an action.
    *
    * @param  id (string, required)
    *         The ID of the action to get.
    * @return The Action object, or null if none.
    */
   actionForID(id) {
     return this._actionsByID.get(id);
@@ -123,69 +153,72 @@ this.PageActions = {
   addAction(action) {
     if (this._deferredAddActionCalls) {
       // init() hasn't been called yet.  Defer all additions until it's called,
       // at which time _deferredAddActionCalls will be deleted.
       this._deferredAddActionCalls.push(() => this.addAction(action));
       return action;
     }
 
+    let hadSep = this.actions.some(a => a.id == ACTION_ID_BUILT_IN_SEPARATOR);
+
+    this._registerAction(action);
+
+    let sep = null;
+    if (!hadSep) {
+      sep = this.actions.find(a => a.id == ACTION_ID_BUILT_IN_SEPARATOR);
+    }
+
+    for (let bpa of allBrowserPageActions()) {
+      if (sep) {
+        // There are now both built-in and non-built-in actions, so place the
+        // separator between the two groups.
+        bpa.placeAction(sep);
+      }
+      bpa.placeAction(action);
+    }
+
+    return action;
+  },
+
+  _registerAction(action) {
     if (this.actionForID(action.id)) {
       throw new Error(`Action with ID '${action.id}' already added`);
     }
     this._actionsByID.set(action.id, action);
 
-    // The IDs of the actions in the panel and urlbar before which the new
-    // action shoud be inserted.  null means at the end, or it's irrelevant.
-    let panelInsertBeforeID = null;
-    let urlbarInsertBeforeID = null;
-
-    let oldBuiltInCount = this._builtInActions.length;
-    let oldNonBuiltInCount = this._nonBuiltInActions.length;
-
     // Insert the action into the appropriate list, either _builtInActions or
-    // _nonBuiltInActions, and find panelInsertBeforeID.
+    // _nonBuiltInActions.
 
     // Keep in mind that _insertBeforeActionID may be present but null, which
     // means the action should be appended to the built-ins.
     if ("__insertBeforeActionID" in action) {
       // A "semi-built-in" action, probably an action from an extension
       // bundled with the browser.  Right now we simply assume that no other
       // consumers will use _insertBeforeActionID.
       let index =
         !action.__insertBeforeActionID ? -1 :
         this._builtInActions.findIndex(a => {
           return a.id == action.__insertBeforeActionID;
         });
       if (index < 0) {
         // Append the action.
         index = this._builtInActions.length;
-        if (this._nonBuiltInActions.length) {
-          panelInsertBeforeID = ACTION_ID_BUILT_IN_SEPARATOR;
-        }
-      } else {
-        panelInsertBeforeID = this._builtInActions[index].id;
       }
       this._builtInActions.splice(index, 0, action);
     } else if (gBuiltInActions.find(a => a.id == action.id)) {
       // A built-in action.  These are always added on init before all other
       // actions, one after the other, so just push onto the array.
       this._builtInActions.push(action);
-      if (this._nonBuiltInActions.length) {
-        panelInsertBeforeID = ACTION_ID_BUILT_IN_SEPARATOR;
-      }
     } else {
       // A non-built-in action, like a non-bundled extension potentially.
       // Keep this list sorted by title.
       let index = BinarySearch.insertionIndexOf((a1, a2) => {
         return a1.title.localeCompare(a2.title);
       }, this._nonBuiltInActions, action);
-      if (index < this._nonBuiltInActions.length) {
-        panelInsertBeforeID = this._nonBuiltInActions[index].id;
-      }
       this._nonBuiltInActions.splice(index, 0, action);
     }
 
     if (this._persistedActions.ids[action.id]) {
       // The action has been seen before.  Override its shownInUrlbar value
       // with the persisted value.  Set the private version of that property
       // so that onActionToggledShownInUrlbar isn't called, which happens when
       // the public version is set.
@@ -194,106 +227,51 @@ this.PageActions = {
     } else {
       // The action is new.  Store it in the persisted actions.
       this._persistedActions.ids[action.id] = true;
       if (action.shownInUrlbar) {
         this._persistedActions.idsInUrlbar.push(action.id);
       }
       this._storePersistedActions();
     }
-
-    if (action.shownInUrlbar) {
-      urlbarInsertBeforeID = this.insertBeforeActionIDInUrlbar(action);
-    }
-
-    // If there are now both built-in and non-built-in actions, add a separator
-    // in the panel between the two groups.
-    let placeBuiltInSeparator =
-      (oldNonBuiltInCount == 0 &&
-       this._nonBuiltInActions.length &&
-       this._builtInActions.length) ||
-      (oldBuiltInCount == 0 &&
-       this._builtInActions.length &&
-       this._nonBuiltInActions.length);
-
-    for (let win of browserWindows()) {
-      if (placeBuiltInSeparator) {
-        let sep = new Action({
-          id: ACTION_ID_BUILT_IN_SEPARATOR,
-          _isSeparator: true,
-        });
-        let sepPanelInsertBeforeID =
-          this._nonBuiltInActions.length ? this._nonBuiltInActions[0].id : null;
-        browserPageActions(win).placeAction(sep, sepPanelInsertBeforeID, null);
-      }
-      browserPageActions(win).placeAction(action, panelInsertBeforeID,
-                                          urlbarInsertBeforeID);
-    }
-
-    return action;
   },
 
   _builtInActions: [],
   _nonBuiltInActions: [],
   _actionsByID: new Map(),
 
   /**
-   * Returns the ID of the action among the actions in the panel before which
-   * the given action should be inserted.
+   * The DOM nodes of actions should be ordered properly, both in the panel and
+   * the urlbar.  This method returns the ID of the action that comes after the
+   * given action in the given array.  You can use the returned ID to get a DOM
+   * node ID to pass to node.insertBefore().
    *
+   * Pass PageActions.actions to get the ID of the next action in the panel.
+   * Pass PageActions.actionsInUrlbar to get the ID of the next action in the
+   * urlbar.
+   *
+   * @param  action
+   *         The action whose node you want to insert into your DOM.
+   * @param  actionArray
+   *         The relevant array of actions, either PageActions.actions or
+   *         actionsInUrlbar.
    * @return The ID of the action before which the given action should be
    *         inserted.  If the given action should be inserted last, returns
    *         null.
    */
-  insertBeforeActionIDInPanel(action) {
-    let index = this._builtInActions.findIndex(a => {
-      return a.id == action.id;
-    });
-    if (index >= 0) {
-      return this._builtInActions[index + 1] ||
-             this._nonBuiltInActions.length ? ACTION_ID_BUILT_IN_SEPARATOR
-                                            : null;
-    }
-
-    index = this._nonBuiltInActions.findIndex(a => {
-      return a.id == action.id;
-    });
-    if (index >= 0) {
-      return this._nonBuiltInActions[index + 1] || null;
-    }
-
-    return null;
-  },
-
-  /**
-   * Returns the ID of the action among the current registered actions in the
-   * urlbar before which the given action should be inserted, ignoring whether
-   * the given action's shownInUrlbar is true or false.
-   *
-   * @return The ID of the action before which the given action should be
-   *         inserted.  If the given action should be inserted last or it should
-   *         not be inserted at all, returns null.
-   */
-  insertBeforeActionIDInUrlbar(action) {
-    // First, find the index of the given action.
-    let idsInUrlbar = this._persistedActions.idsInUrlbar;
-    let index = idsInUrlbar.indexOf(action.id);
+  nextActionID(action, actionArray) {
+    let index = actionArray.findIndex(a => a.id == action.id);
     if (index < 0) {
       return null;
     }
-    // Now start at the next index and find the ID of the first action that's
-    // currently registered.  Remember that IDs in idsInUrlbar may belong to
-    // actions that aren't currently registered.
-    for (let i = index + 1; i < idsInUrlbar.length; i++) {
-      let id = idsInUrlbar[i];
-      if (this.actionForID(id)) {
-        return id;
-      }
+    let nextAction = actionArray[index + 1];
+    if (!nextAction) {
+      return null;
     }
-    return null;
+    return nextAction.id;
   },
 
   /**
    * Call this when an action is removed.
    *
    * @param  action (Action object, required)
    *         The action that was removed.
    */
@@ -315,50 +293,50 @@ this.PageActions = {
     // Remove the action from persisted storage.
     delete this._persistedActions.ids[action.id];
     let index = this._persistedActions.idsInUrlbar.indexOf(action.id);
     if (index >= 0) {
       this._persistedActions.idsInUrlbar.splice(index, 1);
     }
     this._storePersistedActions();
 
-    for (let win of browserWindows()) {
-      browserPageActions(win).removeAction(action);
+    for (let bpa of allBrowserPageActions()) {
+      bpa.removeAction(action);
     }
   },
 
   /**
    * Call this when an action's iconURL changes.
    *
    * @param  action (Action object, required)
    *         The action whose iconURL property changed.
    */
   onActionSetIconURL(action) {
     if (!this.actionForID(action.id)) {
       // This may be called before the action has been added.
       return;
     }
-    for (let win of browserWindows()) {
-      browserPageActions(win).updateActionIconURL(action);
+    for (let bpa of allBrowserPageActions()) {
+      bpa.updateActionIconURL(action);
     }
   },
 
   /**
    * Call this when an action's title changes.
    *
    * @param  action (Action object, required)
    *         The action whose title property changed.
    */
   onActionSetTitle(action) {
     if (!this.actionForID(action.id)) {
       // This may be called before the action has been added.
       return;
     }
-    for (let win of browserWindows()) {
-      browserPageActions(win).updateActionTitle(action);
+    for (let bpa of allBrowserPageActions()) {
+      bpa.updateActionTitle(action);
     }
   },
 
   /**
    * Call this when an action's shownInUrlbar property changes.
    *
    * @param  action (Action object, required)
    *         The action whose shownInUrlbar property changed.
@@ -375,19 +353,18 @@ this.PageActions = {
       if (index < 0) {
         this._persistedActions.idsInUrlbar.push(action.id);
       }
     } else if (index >= 0) {
       this._persistedActions.idsInUrlbar.splice(index, 1);
     }
     this._storePersistedActions();
 
-    let insertBeforeID = this.insertBeforeActionIDInUrlbar(action);
-    for (let win of browserWindows()) {
-      browserPageActions(win).placeActionInUrlbar(action, insertBeforeID);
+    for (let bpa of allBrowserPageActions()) {
+      bpa.placeActionInUrlbar(action);
     }
   },
 
   _storePersistedActions() {
     let json = JSON.stringify(this._persistedActions);
     Services.prefs.setStringPref(PREF_PERSISTED_ACTIONS, json);
   },
 
@@ -1005,23 +982,29 @@ function browserPageActions(obj) {
     return obj.BrowserPageActions;
   }
   return obj.ownerGlobal.BrowserPageActions;
 }
 
 /**
  * A generator function for all open browser windows.
  */
-function* browserWindows() {
+function* allBrowserWindows() {
   let windows = Services.wm.getEnumerator("navigator:browser");
   while (windows.hasMoreElements()) {
     yield windows.getNext();
   }
 }
 
+function* allBrowserPageActions() {
+  for (let win of allBrowserWindows()) {
+    yield browserPageActions(win);
+  }
+}
+
 /**
  * A simple function that sets properties on a given object while doing basic
  * required-properties checking.  If a required property isn't specified in the
  * given options object, or if the options object has properties that aren't in
  * the given schema, then an error is thrown.
  *
  * @param  obj
  *         The object to set properties on.
--- a/browser/modules/test/browser/browser_PageActions.js
+++ b/browser/modules/test/browser/browser_PageActions.js
@@ -757,16 +757,93 @@ add_task(async function nonBuiltFirst() 
   Assert.deepEqual(
     Array.map(BrowserPageActions.mainViewBodyNode.childNodes, n => n.id),
     initialActions.map(a => BrowserPageActions._panelButtonNodeIDForActionID(a.id)),
     "Action should no longer be in panel"
   );
 });
 
 
+// Makes sure that urlbar nodes appear in the correct order in a new window.
+add_task(async function urlbarOrderNewWindow() {
+  // Make some new actions.
+  let actions = [0, 1, 2].map(i => {
+    return PageActions.addAction(new PageActions.Action({
+      id: `test-urlbarOrderNewWindow-${i}`,
+      title: `Test urlbarOrderNewWindow ${i}`,
+      shownInUrlbar: true,
+    }));
+  });
+
+  // Make sure PageActions knows they're appended to the urlbar actions.
+  Assert.deepEqual(
+    PageActions._persistedActions.idsInUrlbar.slice(
+      PageActions._persistedActions.idsInUrlbar.length - actions.length
+    ),
+    actions.map(a => a.id),
+    "PageActions._persistedActions.idsInUrlbar has new actions appended"
+  );
+  Assert.deepEqual(
+    PageActions.actionsInUrlbar.slice(
+      PageActions.actionsInUrlbar.length - actions.length
+    ).map(a => a.id),
+    actions.map(a => a.id),
+    "PageActions.actionsInUrlbar has new actions appended"
+  );
+
+  // Reach into _persistedActions to move the new actions to the front of the
+  // urlbar, same as if the user moved them.  That way we can test that insert-
+  // before IDs are correctly non-null when the urlbar nodes are inserted in the
+  // new window below.
+  PageActions._persistedActions.idsInUrlbar.splice(
+    PageActions._persistedActions.idsInUrlbar.length - actions.length,
+    actions.length
+  );
+  for (let i = 0; i < actions.length; i++) {
+    PageActions._persistedActions.idsInUrlbar.splice(i, 0, actions[i].id);
+  }
+
+  // Save the right-ordered IDs to use below, just in case they somehow get
+  // changed when the new window opens, which shouldn't happen, but maybe
+  // there's bugs.
+  let ids = PageActions._persistedActions.idsInUrlbar.slice();
+
+  // Make sure that worked.
+  Assert.deepEqual(
+    ids.slice(0, actions.length),
+    actions.map(a => a.id),
+    "PageActions._persistedActions.idsInUrlbar now has new actions at front"
+  );
+
+  // Open the new window.
+  let win = await BrowserTestUtils.openNewBrowserWindow();
+
+  // Collect its urlbar nodes.
+  let actualUrlbarNodeIDs = [];
+  for (let node = win.BrowserPageActions.mainButtonNode.nextSibling;
+       node;
+       node = node.nextSibling) {
+    actualUrlbarNodeIDs.push(node.id);
+  }
+
+  // Now check that they're in the right order.
+  Assert.deepEqual(
+    actualUrlbarNodeIDs,
+    ids.map(id => win.BrowserPageActions._urlbarButtonNodeIDForActionID(id)),
+    "Expected actions in new window's urlbar"
+  );
+
+  // Done, clean up.
+  await BrowserTestUtils.closeWindow(win);
+  for (let action of actions) {
+    action.remove();
+  }
+});
+
+
 function promisePageActionPanelOpen() {
   let button = document.getElementById("pageActionButton");
   // The main page action button is hidden for some URIs, so make sure it's
   // visible before trying to click it.
   let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
                   .getInterface(Ci.nsIDOMWindowUtils);
   return BrowserTestUtils.waitForCondition(() => {
     info("Waiting for main page action button to have non-0 size");