Bug 942581 - unregisterArea should keep placements by default so registering/unregistering doesn't lose data in Australis, r=jaws
☠☠ backed out by 74fa7672ad76 ☠ ☠
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 26 Nov 2013 20:50:01 +0100
changeset 174073 43b8117acefc07161378b637226c3811330a431b
parent 174072 6cfa46dc9859324e07af8d975cd4d8e2e4c3df37
child 174074 56161051f7fcc5ea80027bfca7b6dbe48a1e408c
push id445
push userffxbld
push dateMon, 10 Mar 2014 22:05:19 +0000
treeherdermozilla-release@dc38b741b04e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs942581
milestone28.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
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
browser/components/customizableui/test/browser.ini
browser/components/customizableui/test/browser_942581_unregisterArea_keeps_placements.js
browser/components/customizableui/test/head.js
--- a/browser/components/customizableui/src/CustomizableUI.jsm
+++ b/browser/components/customizableui/src/CustomizableUI.jsm
@@ -283,33 +283,44 @@ let CustomizableUIInternal = {
       // Guarantee this area exists in gFuturePlacements, to avoid checking it in
       // various places elsewhere.
       gFuturePlacements.set(aName, new Set());
     } else {
       this.restoreStateForArea(aName);
     }
   },
 
-  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");
     }
 
     // Move all the widgets out
     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 {
       this.endBatchUpdate(true);
     }
   },
 
   registerToolbarNode: function(aToolbar, aExistingChildren) {
@@ -1201,22 +1212,25 @@ let CustomizableUIInternal = {
       if (node.id && !this.getPlacementOfWidget(node.id)) {
         widgets.add(node.id);
       }
     }
 
     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 };
       }
     }
 
     return null;
   },
@@ -1251,17 +1265,17 @@ let CustomizableUIInternal = {
       gFuturePlacements.get(aArea).add(aWidgetId);
       return;
     }
 
     if (this.isSpecialWidget(aWidgetId)) {
       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;
     }
 
     // Do nothing if the widget is not allowed to move to the target area.
     if (!this.canWidgetMoveToArea(aWidgetId, aArea)) {
       return;
@@ -1299,17 +1313,17 @@ let CustomizableUIInternal = {
 
     gDirty = true;
     this.saveState();
 
     this.notifyListeners("onWidgetAdded", aWidgetId, aArea, aPosition);
   },
 
   removeWidgetFromArea: function(aWidgetId) {
-    let oldPlacement = this.getPlacementOfWidget(aWidgetId);
+    let oldPlacement = this.getPlacementOfWidget(aWidgetId, false, true);
     if (!oldPlacement) {
       return;
     }
 
     if (!this.isWidgetRemovable(aWidgetId)) {
       return;
     }
 
@@ -2040,18 +2054,18 @@ this.CustomizableUI = {
     CustomizableUIInternal.registerArea(aName, aProperties);
   },
   registerToolbarNode: function(aToolbar, aExistingChildren) {
     CustomizableUIInternal.registerToolbarNode(aToolbar, aExistingChildren);
   },
   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);
   },
   removeWidgetFromArea: function(aWidgetId) {
     CustomizableUIInternal.removeWidgetFromArea(aWidgetId);
   },
   moveWidgetWithinArea: function(aWidgetId, aPosition) {
@@ -2231,17 +2245,21 @@ function WidgetGroupWrapper(aWidget) {
 
   this.__defineGetter__("instances", function() {
     // Can't use gBuildWindows here because some areas load lazily:
     let placement = CustomizableUIInternal.getPlacementOfWidget(aWidget.id);
     if (!placement) {
       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() {
     return gAreas.get(aWidget.currentArea).get("type");
   });
 
   Object.freeze(this);
 }
--- a/browser/components/customizableui/test/browser.ini
+++ b/browser/components/customizableui/test/browser.ini
@@ -35,9 +35,10 @@ skip-if = true
 [browser_934113_menubar_removable.js]
 # Because this test is about the menubar, it can't be run on mac
 skip-if = os == "mac"
 
 [browser_938980_navbar_collapsed.js]
 [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]
new file mode 100644
--- /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);
+}
+
--- a/browser/components/customizableui/test/head.js
+++ b/browser/components/customizableui/test/head.js
@@ -34,22 +34,23 @@ function createToolbarWithPlacements(id,
   let tb = document.createElementNS(kNSXUL, "toolbar");
   tb.id = id;
   tb.setAttribute("customizable", "true");
   CustomizableUI.registerArea(id, {
     type: CustomizableUI.TYPE_TOOLBAR,
     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();
 }
 
 function resetCustomization() {
   return CustomizableUI.reset();
 }
@@ -66,16 +67,20 @@ function isInWin8() {
 function addSwitchToMetroButtonInWindows8(areaPanelPlacements) {
   if (isInWin8()) {
     areaPanelPlacements.push("switch-to-metro-button");
   }
 }
 
 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);
   for (let i = 0; i < minItems; i++) {
     if (typeof expectedPlacements[i] == "string") {
       is(actualPlacements[i], expectedPlacements[i],
          "Item " + i + " in " + areaId + " should match expectations.");
     } else if (expectedPlacements[i] instanceof RegExp) {