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
--- 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");