Bug 1395387 - Reconcile WebExtension page actions and Photon page actions: Photon page actions changes part 2, purge cache on shutdown. r=mikedeboer
authorDrew Willcoxon <adw@mozilla.com>
Fri, 27 Oct 2017 17:39:47 -0400
changeset 389049 d887685b52ae3890123648911f884873de91c42a
parent 389048 b5b0ac43e2e89c84a670d590862b4d0d22d069d1
child 389050 9f83f552a6118f1061ee5afd87578d9b5b82b308
push id32777
push userarchaeopteryx@coole-files.de
push dateMon, 30 Oct 2017 22:44:45 +0000
treeherdermozilla-central@dd0f265a1300 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmikedeboer
bugs1395387
milestone58.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 1395387 - Reconcile WebExtension page actions and Photon page actions: Photon page actions changes part 2, purge cache on shutdown. r=mikedeboer MozReview-Commit-ID: LEMywaJu8xM
browser/modules/PageActions.jsm
browser/modules/test/browser/browser_PageActions.js
--- a/browser/modules/PageActions.jsm
+++ b/browser/modules/PageActions.jsm
@@ -16,16 +16,18 @@ this.EXPORTED_SYMBOLS = [
 
 const { utils: Cu } = Components;
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
   "resource://gre/modules/AppConstants.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown",
+  "resource://gre/modules/AsyncShutdown.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "BinarySearch",
   "resource://gre/modules/BinarySearch.jsm");
 
 
 const ACTION_ID_BOOKMARK = "bookmark";
 const ACTION_ID_BOOKMARK_SEPARATOR = "bookmarkSeparator";
 const ACTION_ID_BUILT_IN_SEPARATOR = "builtInSeparator";
 
@@ -59,16 +61,26 @@ this.PageActions = {
       bpa.placeAllActions();
     }
 
     // These callbacks are deferred until init happens and all built-in actions
     // are added.
     while (callbacks && callbacks.length) {
       callbacks.shift()();
     }
+
+    // Purge removed actions from persisted state on shutdown.  The point is not
+    // to do it on Action.remove().  That way actions that are removed and
+    // re-added while the app is running will have their urlbar placement and
+    // other state remembered and restored.  This happens for upgraded and
+    // downgraded extensions, for example.
+    AsyncShutdown.profileBeforeChange.addBlocker(
+      "PageActions: purging unregistered actions from cache",
+      () => this._purgeUnregisteredPersistedActions(),
+    );
   },
 
   _deferredAddActionCalls: [],
 
   /**
    * The list of Action objects, sorted in the order in which they should be
    * placed in the page action panel.  If there are both built-in and non-built-
    * in actions, then the list will include the separator between the two.  The
@@ -242,16 +254,17 @@ this.PageActions = {
         this._persistedActions.idsInUrlbar.splice(nextIndex, 0, action.id);
       }
     } else if (index >= 0) {
       this._persistedActions.idsInUrlbar.splice(index, 1);
     }
     this._storePersistedActions();
   },
 
+  // These keep track of currently registered actions.
   _builtInActions: [],
   _nonBuiltInActions: [],
   _actionsByID: new Map(),
 
   /**
    * Returns the ID of the action before which the given action should be
    * inserted in the urlbar.
    *
@@ -317,39 +330,29 @@ this.PageActions = {
   /**
    * Call this when an action is removed.
    *
    * @param  action (Action object, required)
    *         The action that was removed.
    */
   onActionRemoved(action) {
     if (!this.actionForID(action.id)) {
-      // The action isn't present.  Not an error.
+      // The action isn't registered (yet).  Not an error.
       return;
     }
 
     this._actionsByID.delete(action.id);
     for (let list of [this._nonBuiltInActions, this._builtInActions]) {
       let index = list.findIndex(a => a.id == action.id);
       if (index >= 0) {
         list.splice(index, 1);
         break;
       }
     }
 
-    // Remove the action from persisted storage.
-    for (let name of ["ids", "idsInUrlbar"]) {
-      let array = this._persistedActions[name];
-      let index = array.indexOf(action.id);
-      if (index >= 0) {
-        array.splice(index, 1);
-      }
-    }
-    this._storePersistedActions();
-
     for (let bpa of allBrowserPageActions()) {
       bpa.removeAction(action);
     }
   },
 
   /**
    * Call this when an action's shownInUrlbar property changes.
    *
@@ -395,16 +398,27 @@ this.PageActions = {
 
   _loadPersistedActions() {
     try {
       let json = Services.prefs.getStringPref(PREF_PERSISTED_ACTIONS);
       this._persistedActions = this._migratePersistedActions(JSON.parse(json));
     } catch (ex) {}
   },
 
+  _purgeUnregisteredPersistedActions() {
+    // Remove all action IDs from persisted state that do not correspond to
+    // currently registered actions.
+    for (let name of ["ids", "idsInUrlbar"]) {
+      this._persistedActions[name] = this._persistedActions[name].filter(id => {
+        return this.actionForID(id);
+      });
+    }
+    this._storePersistedActions();
+  },
+
   _migratePersistedActions(actions) {
     // Start with actions.version and migrate one version at a time, all the way
     // up to the current version.
     for (let version = actions.version || 0;
          version < PERSISTED_ACTIONS_CURRENT_VERSION;
          version++) {
       let methodName = `_migratePersistedActionsTo${version + 1}`;
       actions = this[methodName](actions);
@@ -428,16 +442,19 @@ this.PageActions = {
       actions.idsInUrlbar.push(ACTION_ID_BOOKMARK);
     }
     return {
       ids,
       idsInUrlbar: actions.idsInUrlbar,
     };
   },
 
+  // This keeps track of all actions, even those that are not currently
+  // registered because they have been removed, so long as
+  // _purgeUnregisteredPersistedActions has not been called.
   _persistedActions: {
     version: PERSISTED_ACTIONS_CURRENT_VERSION,
     // action IDs that have ever been seen and not removed, order not important
     ids: [],
     // action IDs ordered by position in urlbar
     idsInUrlbar: [],
   },
 };
@@ -915,22 +932,22 @@ Action.prototype = {
    */
   onShowingInPanel(buttonNode) {
     if (this._onShowingInPanel) {
       this._onShowingInPanel(buttonNode);
     }
   },
 
   /**
-   * Makes PageActions forget about this action and removes its DOM nodes from
-   * all browser windows.  Call this when the user removes your action, like
-   * when your extension is uninstalled.  You probably don't want to call it
-   * simply when your extension is disabled or the app quits, because then
-   * PageActions won't remember it the next time your extension is enabled or
-   * the app starts.
+   * Removes the action's DOM nodes from all browser windows.
+   *
+   * PageActions will remember the action's urlbar placement, if any, after this
+   * method is called until app shutdown.  If the action is not added again
+   * before shutdown, then PageActions will discard the placement, and the next
+   * time the action is added, its placement will be reset.
    */
   remove() {
     PageActions.onActionRemoved(this);
   }
 };
 
 this.PageActions.Action = Action;
 
--- a/browser/modules/test/browser/browser_PageActions.js
+++ b/browser/modules/test/browser/browser_PageActions.js
@@ -209,35 +209,38 @@ add_task(async function simple() {
 
   // Remove the action.
   action.remove();
   panelButtonNode = document.getElementById(panelButtonID);
   Assert.equal(panelButtonNode, null, "panelButtonNode");
   urlbarButtonNode = document.getElementById(urlbarButtonID);
   Assert.equal(urlbarButtonNode, null, "urlbarButtonNode");
 
-  Assert.deepEqual(PageActions.actions, initialActions,
-                   "Actions should go back to initial");
-  Assert.equal(PageActions.actionForID(action.id), null,
-               "actionForID should be null");
-
-  Assert.ok(!PageActions._persistedActions.ids.includes(action.id),
-            "PageActions should remove action from its list of seen actions");
-
   // The separator between the built-in actions and non-built-in actions should
   // be gone now, too.
   let separatorNode = document.getElementById(
     BrowserPageActions.panelButtonNodeIDForActionID(
       PageActions.ACTION_ID_BUILT_IN_SEPARATOR
     )
   );
   Assert.equal(separatorNode, null, "No separator");
   Assert.ok(!BrowserPageActions.mainViewBodyNode
             .lastChild.localName.includes("separator"),
             "Last child should not be separator");
+
+  Assert.deepEqual(PageActions.actions, initialActions,
+                   "Actions should go back to initial");
+  Assert.equal(PageActions.actionForID(action.id), null,
+               "actionForID should be null");
+
+  Assert.ok(PageActions._persistedActions.ids.includes(action.id),
+            "Action ID should remain in cache until purged");
+  PageActions._purgeUnregisteredPersistedActions();
+  Assert.ok(!PageActions._persistedActions.ids.includes(action.id),
+            "Action ID should be removed from cache after being purged");
 });
 
 
 // Tests a non-built-in action with a subview.
 add_task(async function withSubview() {
   let id = "test-subview";
 
   let onActionCommandCallCount = 0;
@@ -867,16 +870,22 @@ add_task(async function urlbarOrderNewWi
 
   // 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"
   );
 
+  // _persistedActions will contain the IDs of test actions added and removed
+  // above (unless PageActions._purgeUnregisteredPersistedActions() was called
+  // for all of them, which it wasn't).  Filter them out because they should
+  // not appear in the new window (or any window at this point).
+  ids = ids.filter(id => PageActions.actionForID(id));
+
   // 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) {
@@ -968,17 +977,17 @@ add_task(async function migrate1() {
     actualUrlbarNodeIDs,
     orderedIDs.map(id => win.BrowserPageActions.urlbarButtonNodeIDForActionID(id)),
     "Expected actions in new window's urlbar"
   );
 
   // Done, clean up.
   await BrowserTestUtils.closeWindow(win);
   Services.prefs.clearUserPref(PageActions.PREF_PERSISTED_ACTIONS);
-  PageActions.actionForID("copyURL")._shownInUrlbar = false;
+  PageActions.actionForID("copyURL").shownInUrlbar = false;
 });
 
 
 // Opens a new browser window and makes sure per-window state works right.
 add_task(async function perWindowState() {
   // Add a test action.
   let title = "Test perWindowState";
   let action = PageActions.addAction(new PageActions.Action({
@@ -1035,16 +1044,121 @@ add_task(async function perWindowState()
                "Panel button label in new window");
 
   // Done, clean up.
   await BrowserTestUtils.closeWindow(newWindow);
   action.remove();
 });
 
 
+// Adds an action, changes its placement in the urlbar to something non-default,
+// removes the action, and then adds it back.  Since the action was removed and
+// re-added without restarting the app (or more accurately without calling
+// PageActions._purgeUnregisteredPersistedActions), the action should remain in
+// persisted state and retain its last placement in the urlbar.
+add_task(async function removeRetainState() {
+  // Get the list of actions initially in the urlbar.
+  let initialActionsInUrlbar = PageActions.actionsInUrlbar;
+  Assert.ok(initialActionsInUrlbar.length > 0,
+            "This test expects there to be at least one action in the urlbar initially (like the bookmark star)");
+
+  // Add a test action.
+  let id = "test-removeRetainState";
+  let testAction = PageActions.addAction(new PageActions.Action({
+    id,
+    title: "Test removeRetainState",
+  }));
+
+  // Show its button in the urlbar.
+  testAction.shownInUrlbar = true;
+
+  // "Move" the test action to the front of the urlbar by toggling shownInUrlbar
+  // for all the other actions in the urlbar.
+  for (let action of initialActionsInUrlbar) {
+    action.shownInUrlbar = false;
+    action.shownInUrlbar = true;
+  }
+
+  // Check the actions in PageActions.actionsInUrlbar.
+  Assert.deepEqual(
+    PageActions.actionsInUrlbar.map(a => a.id),
+    [testAction].concat(initialActionsInUrlbar).map(a => a.id),
+    "PageActions.actionsInUrlbar should be in expected order: testAction followed by all initial actions"
+  );
+
+  // Check the nodes in the urlbar.
+  let actualUrlbarNodeIDs = [];
+  for (let node = BrowserPageActions.mainButtonNode.nextSibling;
+       node;
+       node = node.nextSibling) {
+    actualUrlbarNodeIDs.push(node.id);
+  }
+  Assert.deepEqual(
+    actualUrlbarNodeIDs,
+    [testAction].concat(initialActionsInUrlbar).map(a => BrowserPageActions.urlbarButtonNodeIDForActionID(a.id)),
+    "urlbar nodes should be in expected order: testAction followed by all initial actions"
+  );
+
+  // Remove the test action.
+  testAction.remove();
+
+  // Check the actions in PageActions.actionsInUrlbar.
+  Assert.deepEqual(
+    PageActions.actionsInUrlbar.map(a => a.id),
+    initialActionsInUrlbar.map(a => a.id),
+    "PageActions.actionsInUrlbar should be in expected order after removing test action: all initial actions"
+  );
+
+  // Check the nodes in the urlbar.
+  actualUrlbarNodeIDs = [];
+  for (let node = BrowserPageActions.mainButtonNode.nextSibling;
+       node;
+       node = node.nextSibling) {
+    actualUrlbarNodeIDs.push(node.id);
+  }
+  Assert.deepEqual(
+    actualUrlbarNodeIDs,
+    initialActionsInUrlbar.map(a => BrowserPageActions.urlbarButtonNodeIDForActionID(a.id)),
+    "urlbar nodes should be in expected order after removing test action: all initial actions"
+  );
+
+  // Add the test action again.
+  testAction = PageActions.addAction(new PageActions.Action({
+    id,
+    title: "Test removeRetainState",
+  }));
+
+  // Show its button in the urlbar again.
+  testAction.shownInUrlbar = true;
+
+  // Check the actions in PageActions.actionsInUrlbar.
+  Assert.deepEqual(
+    PageActions.actionsInUrlbar.map(a => a.id),
+    [testAction].concat(initialActionsInUrlbar).map(a => a.id),
+    "PageActions.actionsInUrlbar should be in expected order after re-adding test action: testAction followed by all initial actions"
+  );
+
+  // Check the nodes in the urlbar.
+  actualUrlbarNodeIDs = [];
+  for (let node = BrowserPageActions.mainButtonNode.nextSibling;
+       node;
+       node = node.nextSibling) {
+    actualUrlbarNodeIDs.push(node.id);
+  }
+  Assert.deepEqual(
+    actualUrlbarNodeIDs,
+    [testAction].concat(initialActionsInUrlbar).map(a => BrowserPageActions.urlbarButtonNodeIDForActionID(a.id)),
+    "urlbar nodes should be in expected order after re-adding test action: testAction followed by all initial actions"
+  );
+
+  // Done, clean up.
+  testAction.remove();
+});
+
+
 function promisePageActionPanelOpen() {
   let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
                   .getInterface(Ci.nsIDOMWindowUtils);
   return BrowserTestUtils.waitForCondition(() => {
     // Wait for the main page action button to become visible.  It's hidden for
     // some URIs, so depending on when this is called, it may not yet be quite
     // visible.  It's up to the caller to make sure it will be visible.
     info("Waiting for main page action button to have non-0 size");