author | Drew Willcoxon <adw@mozilla.com> |
Fri, 27 Oct 2017 17:39:47 -0400 | |
changeset 389049 | d887685b52ae3890123648911f884873de91c42a |
parent 389048 | b5b0ac43e2e89c84a670d590862b4d0d22d069d1 |
child 389050 | 9f83f552a6118f1061ee5afd87578d9b5b82b308 |
push id | 32777 |
push user | archaeopteryx@coole-files.de |
push date | Mon, 30 Oct 2017 22:44:45 +0000 |
treeherder | mozilla-central@dd0f265a1300 [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | mikedeboer |
bugs | 1395387 |
milestone | 58.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
|
browser/modules/PageActions.jsm | file | annotate | diff | comparison | revisions | |
browser/modules/test/browser/browser_PageActions.js | file | annotate | diff | comparison | revisions |
--- 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");