Bug 942581 - unregisterArea should keep placements by default so registering/unregistering doesn't lose data in Australis, r=jaws

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() {

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]

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);
+}
+

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