From bec9f2f155997c15f5f028cb1ac1d9834ca3a151 Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Tue, 26 Nov 2013 20:50:01 +0100 Subject: [PATCH] Bug 942581 - unregisterArea should keep placements by default so registering/unregistering doesn't lose data in Australis, r=jaws --- .../customizableui/src/CustomizableUI.jsm | 38 ++++-- .../customizableui/test/browser.ini | 1 + ..._942581_unregisterArea_keeps_placements.js | 118 ++++++++++++++++++ .../components/customizableui/test/head.js | 7 +- 4 files changed, 153 insertions(+), 11 deletions(-) create mode 100644 browser/components/customizableui/test/browser_942581_unregisterArea_keeps_placements.js diff --git a/browser/components/customizableui/src/CustomizableUI.jsm b/browser/components/customizableui/src/CustomizableUI.jsm index 916e4348e611..ca49fa39ee29 100644 --- a/browser/components/customizableui/src/CustomizableUI.jsm +++ b/browser/components/customizableui/src/CustomizableUI.jsm @@ -288,11 +288,11 @@ let CustomizableUIInternal = { } }, - unregisterArea: function(aName) { + unregisterArea: function(aName, aDestroyPlacements) { if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName)) { throw new Error("Invalid area name"); } - if (!gAreas.has(aName)) { + if (!gAreas.has(aName) && !gPlacements.has(aName)) { throw new Error("Area not registered"); } @@ -300,11 +300,22 @@ let CustomizableUIInternal = { this.beginBatchUpdate(); try { let placements = gPlacements.get(aName); - placements.forEach(this.removeWidgetFromArea, this); + if (placements) { + // Need to clone this array so removeWidgetFromArea doesn't modify it + placements = [...placements]; + placements.forEach(this.removeWidgetFromArea, this); + } // Delete all remaining traces. gAreas.delete(aName); - gPlacements.delete(aName); + // Only destroy placements when necessary: + if (aDestroyPlacements) { + gPlacements.delete(aName); + } else { + // Otherwise we need to re-set them, as removeFromArea will have emptied + // them out: + gPlacements.set(aName, placements); + } gFuturePlacements.delete(aName); gBuildAreas.delete(aName); } finally { @@ -1206,12 +1217,15 @@ let CustomizableUIInternal = { return [...widgets]; }, - getPlacementOfWidget: function(aWidgetId, aOnlyRegistered) { + getPlacementOfWidget: function(aWidgetId, aOnlyRegistered, aDeadAreas) { if (aOnlyRegistered && !this.widgetExists(aWidgetId)) { return null; } for (let [area, placements] of gPlacements) { + if (!gAreas.has(area) && !aDeadAreas) { + continue; + } let index = placements.indexOf(aWidgetId); if (index != -1) { return { area: area, position: index }; @@ -1256,7 +1270,7 @@ let CustomizableUIInternal = { aWidgetId = this.ensureSpecialWidgetId(aWidgetId); } - let oldPlacement = this.getPlacementOfWidget(aWidgetId); + let oldPlacement = this.getPlacementOfWidget(aWidgetId, false, true); if (oldPlacement && oldPlacement.area == aArea) { this.moveWidgetWithinArea(aWidgetId, aPosition); return; @@ -1304,7 +1318,7 @@ let CustomizableUIInternal = { }, removeWidgetFromArea: function(aWidgetId) { - let oldPlacement = this.getPlacementOfWidget(aWidgetId); + let oldPlacement = this.getPlacementOfWidget(aWidgetId, false, true); if (!oldPlacement) { return; } @@ -2045,8 +2059,8 @@ this.CustomizableUI = { registerMenuPanel: function(aPanel) { CustomizableUIInternal.registerMenuPanel(aPanel); }, - unregisterArea: function(aName) { - CustomizableUIInternal.unregisterArea(aName); + unregisterArea: function(aName, aDestroyPlacements) { + CustomizableUIInternal.unregisterArea(aName, aDestroyPlacements); }, addWidgetToArea: function(aWidgetId, aArea, aPosition) { CustomizableUIInternal.addWidgetToArea(aWidgetId, aArea, aPosition); @@ -2236,7 +2250,11 @@ function WidgetGroupWrapper(aWidget) { return []; } let area = placement.area; - return [this.forWindow(node.ownerDocument.defaultView) for (node of gBuildAreas.get(area))]; + let buildAreas = gBuildAreas.get(area); + if (!buildAreas) { + return []; + } + return [this.forWindow(node.ownerDocument.defaultView) for (node of buildAreas)]; }); this.__defineGetter__("areaType", function() { diff --git a/browser/components/customizableui/test/browser.ini b/browser/components/customizableui/test/browser.ini index 2bcc13f93c27..6ab40f2c040b 100644 --- a/browser/components/customizableui/test/browser.ini +++ b/browser/components/customizableui/test/browser.ini @@ -40,4 +40,5 @@ skip-if = os == "mac" [browser_938995_indefaultstate_nonremovable.js] [browser_940946_removable_from_navbar_customizemode.js] [browser_941083_invalidate_wrapper_cache_createWidget.js] +[browser_942581_unregisterArea_keeps_placements.js] [browser_panel_toggle.js] diff --git a/browser/components/customizableui/test/browser_942581_unregisterArea_keeps_placements.js b/browser/components/customizableui/test/browser_942581_unregisterArea_keeps_placements.js new file mode 100644 index 000000000000..feda11a32523 --- /dev/null +++ b/browser/components/customizableui/test/browser_942581_unregisterArea_keeps_placements.js @@ -0,0 +1,118 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +const kToolbarName = "test-unregisterArea-placements-toolbar"; +const kTestWidgetPfx = "test-widget-for-unregisterArea-placements-"; +const kTestWidgetCount = 3; + +let gTests = [ + { + desc: "unregisterArea should keep placements by default and restore them when re-adding the area", + run: function() { + let widgetIds = [] + for (let i = 0; i < kTestWidgetCount; i++) { + let id = kTestWidgetPfx + i; + widgetIds.push(id); + let spec = {id: id, type: 'button', removable: true, label: "unregisterArea test", tooltiptext: "" + i}; + CustomizableUI.createWidget(spec); + } + for (let i = kTestWidgetCount; i < kTestWidgetCount * 2; i++) { + let id = kTestWidgetPfx + i; + widgetIds.push(id); + createDummyXULButton(id, "unregisterArea XUL test " + i); + } + let toolbarNode = createToolbarWithPlacements(kToolbarName, widgetIds); + checkAbstractAndRealPlacements(toolbarNode, widgetIds); + + // Now move one of them: + CustomizableUI.moveWidgetWithinArea(kTestWidgetPfx + kTestWidgetCount, 0); + // Clone the array so we know this is the modified one: + let modifiedWidgetIds = [...widgetIds]; + let movedWidget = modifiedWidgetIds.splice(kTestWidgetCount, 1)[0]; + modifiedWidgetIds.unshift(movedWidget); + + // Check it: + checkAbstractAndRealPlacements(toolbarNode, modifiedWidgetIds); + + // Then unregister + CustomizableUI.unregisterArea(kToolbarName); + + // Check we tell the outside world no dangerous things: + checkWidgetFates(widgetIds); + // Only then remove the real node + toolbarNode.remove(); + + // Now move one of the items to the palette, and another to the navbar: + let lastWidget = modifiedWidgetIds.pop(); + CustomizableUI.removeWidgetFromArea(lastWidget); + lastWidget = modifiedWidgetIds.pop(); + CustomizableUI.addWidgetToArea(lastWidget, CustomizableUI.AREA_NAVBAR); + + // Recreate ourselves with the default placements being the same: + toolbarNode = createToolbarWithPlacements(kToolbarName, widgetIds); + // Then check that after doing this, our actual placements match + // the modified list, not the default one. + checkAbstractAndRealPlacements(toolbarNode, modifiedWidgetIds); + + // Now remove completely: + CustomizableUI.unregisterArea(kToolbarName, true); + checkWidgetFates(modifiedWidgetIds); + toolbarNode.remove(); + + // One more time: + // Recreate ourselves with the default placements being the same: + toolbarNode = createToolbarWithPlacements(kToolbarName, widgetIds); + // Should now be back to default: + checkAbstractAndRealPlacements(toolbarNode, widgetIds); + CustomizableUI.unregisterArea(kToolbarName, true); + checkWidgetFates(widgetIds); + toolbarNode.remove(); + + //XXXgijs: ensure cleanup function doesn't barf: + gAddedToolbars.delete(kToolbarName); + + // Remove all the XUL widgets, destroy the others: + for (let widget of widgetIds) { + let widgetWrapper = CustomizableUI.getWidget(widget); + if (widgetWrapper.provider == CustomizableUI.PROVIDER_XUL) { + gNavToolbox.palette.querySelector("#" + widget).remove(); + } else { + CustomizableUI.destroyWidget(widget); + } + } + }, + } +]; + +function checkAbstractAndRealPlacements(aNode, aExpectedPlacements) { + assertAreaPlacements(kToolbarName, aExpectedPlacements); + let physicalWidgetIds = [node.id for (node of aNode.childNodes)]; + placementArraysEqual(aNode.id, physicalWidgetIds, aExpectedPlacements); +} + +function checkWidgetFates(aWidgetIds) { + for (let widget of aWidgetIds) { + ok(!CustomizableUI.getPlacementOfWidget(widget), "Widget should be in palette"); + ok(!document.getElementById(widget), "Widget should not be in the DOM"); + let widgetInPalette = !!gNavToolbox.palette.querySelector("#" + widget); + let widgetProvider = CustomizableUI.getWidget(widget).provider; + let widgetIsXULWidget = widgetProvider == CustomizableUI.PROVIDER_XUL; + is(widgetInPalette, widgetIsXULWidget, "Just XUL Widgets should be in the palette"); + } +} + +function asyncCleanup() { + yield resetCustomization(); +} + +function cleanup() { + removeCustomToolbars(); +} + +function test() { + waitForExplicitFinish(); + registerCleanupFunction(cleanup); + runTests(gTests, asyncCleanup); +} + diff --git a/browser/components/customizableui/test/head.js b/browser/components/customizableui/test/head.js index 5e1b7ba8e8eb..1de61e07fc21 100644 --- a/browser/components/customizableui/test/head.js +++ b/browser/components/customizableui/test/head.js @@ -39,12 +39,13 @@ function createToolbarWithPlacements(id, placements) { defaultPlacements: placements }); gNavToolbox.appendChild(tb); + return tb; } function removeCustomToolbars() { CustomizableUI.reset(); for (let toolbarId of gAddedToolbars) { - CustomizableUI.unregisterArea(toolbarId); + CustomizableUI.unregisterArea(toolbarId, true); document.getElementById(toolbarId).remove(); } gAddedToolbars.clear(); @@ -71,6 +72,10 @@ function addSwitchToMetroButtonInWindows8(areaPanelPlacements) { function assertAreaPlacements(areaId, expectedPlacements) { let actualPlacements = getAreaWidgetIds(areaId); + placementArraysEqual(areaId, actualPlacements, expectedPlacements); +} + +function placementArraysEqual(areaId, actualPlacements, expectedPlacements) { is(actualPlacements.length, expectedPlacements.length, "Area " + areaId + " should have " + expectedPlacements.length + " items."); let minItems = Math.min(expectedPlacements.length, actualPlacements.length);