Bug 1395387 - Reconcile WebExtension page actions and Photon page actions: Photon page actions changes part 2, purge cache on shutdown. r?mikedeboer draft
authorDrew Willcoxon <adw@mozilla.com>
Fri, 27 Oct 2017 17:39:47 -0400
changeset 687855 ab5e4fa906dd1f4cedcfd5e1e4cd09c414eac31b
parent 687854 7d6c03a99ba1929a2cc2d9815b65d62bb876d625
child 687856 018c7ef91d4c714f67ac60a427ef02d16bccc9da
push id86621
push userdwillcoxon@mozilla.com
push dateFri, 27 Oct 2017 21:40:47 +0000
reviewersmikedeboer
bugs1395387
milestone58.0a1
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");