Bug 354048, allow toolbaritems to be placed directly inside toolbar so that uncustomized toolbars don't need to be rebuilt at startup, r=dao

browser/base/content/browser.xul

@@ -335,10 +335,20 @@
       </toolbaritem>
     </toolbar>
 
-    <toolbarpalette id="BrowserToolbarPalette">
+    <toolbar id="nav-bar" class="toolbar-primary chromeclass-toolbar"
+             toolbarname="&navbarCmd.label;" accesskey="&navbarCmd.accesskey;"
+             fullscreentoolbar="true" mode="icons" customizable="true"
+#ifdef WINCE
+             iconsize="small" defaulticonsize="small"
+             defaultset="unified-back-forward-button,reload-button,stop-button,home-button,urlbar-container,search-container,navigator-throbber,fullscreenflex,window-controls"
+#else
+             iconsize="large"
+             defaultset="unified-back-forward-button,reload-button,stop-button,home-button,urlbar-container,search-container,fullscreenflex,window-controls"
+#endif
+             context="toolbar-context-menu">
 
       <toolbaritem id="unified-back-forward-button" class="chromeclass-toolbar-additional"
-                   context="backForwardMenu">
+                   context="backForwardMenu" removable="true">
         <toolbarbutton id="back-button" class="toolbarbutton-1"
                        label="&backCmd.label;"
                        command="Browser:BackOrBackDuplicate"
@@ -367,18 +377,18 @@
       </toolbaritem>
 
       <toolbarbutton id="reload-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                     label="&reloadCmd.label;"
+                     label="&reloadCmd.label;" removable="true"
                      command="Browser:ReloadOrDuplicate"
                      onclick="checkForMiddleClick(this, event);"
                      tooltiptext="&reloadButton.tooltip;"/>
 
       <toolbarbutton id="stop-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                     label="&stopCmd.label;"
+                     label="&stopCmd.label;" removable="true"
                      command="Browser:Stop"
                      tooltiptext="&stopButton.tooltip;"/>
 
       <toolbarbutton id="home-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                     persist="class"
+                     persist="class" removable="true"
                      label="&homeButton.label;"
                      ondragover="homeButtonObserver.onDragOver(event)"
                      ondragenter="homeButtonObserver.onDragOver(event)"
@@ -387,7 +397,7 @@
                      onclick="BrowserGoHome(event);"/>
 
       <toolbaritem id="urlbar-container" align="center" flex="400" persist="width"
-                   title="&locationItem.title;" class="chromeclass-location">
+                   title="&locationItem.title;" class="chromeclass-location" removable="true">
         <textbox id="urlbar" flex="1"
                  bookmarkhistoryemptytext="&urlbar.bookmarkhistory.emptyText;"
                  bookmarkemptytext="&urlbar.bookmark.emptyText;"
@@ -460,105 +470,10 @@
 
       <toolbaritem id="search-container" title="&searchItem.title;"
                    align="center" class="chromeclass-toolbar-additional"
-                   flex="100" persist="width">
+                   flex="100" persist="width" removable="true">
         <searchbar id="searchbar" flex="1"/>
       </toolbaritem>
 
-      <toolbarbutton id="print-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                     label="&printButton.label;" command="cmd_print"
-                     tooltiptext="&printButton.tooltip;"/>
-
-      <toolbaritem id="navigator-throbber" title="&throbberItem.title;" align="center" pack="center"
-                   mousethrough="always">
-        <image/>
-      </toolbaritem>
-
-      <toolbaritem flex="1" id="personal-bookmarks" title="&bookmarksItem.title;">
-         <hbox id="bookmarksBarContent" flex="1"
-               type="places"
-               place="place:folder=TOOLBAR"
-               context="placesContext"
-               onclick="BookmarksEventHandler.onClick(event);"
-               oncommand="BookmarksEventHandler.onCommand(event);"
-               onpopupshowing="BookmarksEventHandler.onPopupShowing(event);"
-               tooltip="bhTooltip" popupsinherittooltip="true"/>
-      </toolbaritem>
-
-        <toolbarbutton id="downloads-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                       observes="Tools:Downloads"
-                       ondrop="DownloadsButtonDNDObserver.onDrop(event)"
-                       ondragover="DownloadsButtonDNDObserver.onDragOver(event)"
-                       ondragenter="DownloadsButtonDNDObserver.onDragOver(event)"
-                       ondragleave="DownloadsButtonDNDObserver.onDragLeave(event)"
-                       label="&downloads.label;"
-                       tooltiptext="&downloads.tooltip;"/>
-
-        <toolbarbutton id="history-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                       observes="viewHistorySidebar" label="&historyButton.label;"
-                       tooltiptext="&historyButton.tooltip;"/>
-
-        <toolbarbutton id="bookmarks-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                       observes="viewBookmarksSidebar"
-                       tooltiptext="&bookmarksButton.tooltip;"
-                       ondrop="bookmarksButtonObserver.onDrop(event)"
-                       ondragover="bookmarksButtonObserver.onDragOver(event)"
-                       ondragenter="bookmarksButtonObserver.onDragOver(event)"
-                       ondragleave="bookmarksButtonObserver.onDragLeave(event)"/>
-
-        <toolbarbutton id="new-tab-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                       label="&tabCmd.label;"
-                       command="cmd_newNavigatorTab"
-                       tooltiptext="&newTabButton.tooltip;"
-                       ondrop="newTabButtonObserver.onDrop(event)"
-                       ondragover="newTabButtonObserver.onDragOver(event)"
-                       ondragenter="newTabButtonObserver.onDragOver(event)"
-                       ondragleave="newTabButtonObserver.onDragLeave(event)"/>
-
-        <toolbarbutton id="new-window-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                       label="&newNavigatorCmd.label;"
-                       command="key_newNavigator"
-                       tooltiptext="&newWindowButton.tooltip;"
-                       ondrop="newWindowButtonObserver.onDrop(event)"
-                       ondragover="newWindowButtonObserver.onDragOver(event)"
-                       ondragenter="newWindowButtonObserver.onDragOver(event)"
-                       ondragleave="newWindowButtonObserver.onDragLeave(event)"/>
-
-        <toolbarbutton id="cut-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                       label="&cutCmd.label;"
-                       command="cmd_cut"
-                       tooltiptext="&cutButton.tooltip;"/>
-
-        <toolbarbutton id="copy-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                       label="&copyCmd.label;"
-                       command="cmd_copy"
-                       tooltiptext="&copyButton.tooltip;"/>
-
-        <toolbarbutton id="paste-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                       label="&pasteCmd.label;"
-                       command="cmd_paste"
-                       tooltiptext="&pasteButton.tooltip;"/>
-        <toolbarbutton id="fullscreen-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
-                       observes="View:FullScreen"
-                       type="checkbox"
-                       label="&fullScreenCmd.label;"
-                       tooltiptext="&fullScreenButton.tooltip;"/>
-    </toolbarpalette>
-
-    <toolbar id="nav-bar" class="toolbar-primary chromeclass-toolbar"
-             toolbarname="&navbarCmd.label;" accesskey="&navbarCmd.accesskey;"
-             fullscreentoolbar="true" mode="icons"
-#ifdef WINCE
-             iconsize="small" defaulticonsize="small"
-#else
-             iconsize="large"
-#endif
-             customizable="true"
-#ifdef WINCE
-             defaultset="unified-back-forward-button,reload-button,stop-button,home-button,urlbar-container,search-container,navigator-throbber,fullscreenflex,window-controls"
-#else
-             defaultset="unified-back-forward-button,reload-button,stop-button,home-button,urlbar-container,search-container,fullscreenflex,window-controls"
-#endif
-             context="toolbar-context-menu">
       <hbox id="fullscreenflex" flex="1" hidden="true" fullscreencontrol="true"/>
       <hbox id="window-controls" hidden="true" fullscreencontrol="true">
         <toolbarbutton id="minimize-button"
@@ -587,7 +502,89 @@
 #ifdef WINCE
              collapsed="true"
 #endif
-             customizable="true"/>
+             customizable="true">
+      <toolbaritem flex="1" id="personal-bookmarks" title="&bookmarksItem.title;">
+         <hbox id="bookmarksBarContent" flex="1"
+               type="places"
+               place="place:folder=TOOLBAR"
+               context="placesContext"
+               onclick="BookmarksEventHandler.onClick(event);"
+               oncommand="BookmarksEventHandler.onCommand(event);"
+               onpopupshowing="BookmarksEventHandler.onPopupShowing(event);"
+               tooltip="bhTooltip" popupsinherittooltip="true"/>
+      </toolbaritem>
+    </toolbar>
+
+    <toolbarpalette id="BrowserToolbarPalette">
+
+      <toolbarbutton id="print-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     label="&printButton.label;" command="cmd_print"
+                     tooltiptext="&printButton.tooltip;"/>
+
+      <toolbaritem id="navigator-throbber" title="&throbberItem.title;" align="center" pack="center"
+                   mousethrough="always">
+        <image/>
+      </toolbaritem>
+
+      <toolbarbutton id="downloads-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     observes="Tools:Downloads"
+                     ondrop="DownloadsButtonDNDObserver.onDrop(event)"
+                     ondragover="DownloadsButtonDNDObserver.onDragOver(event)"
+                     ondragenter="DownloadsButtonDNDObserver.onDragOver(event)"
+                     ondragleave="DownloadsButtonDNDObserver.onDragLeave(event)"
+                     label="&downloads.label;"
+                     tooltiptext="&downloads.tooltip;"/>
+
+      <toolbarbutton id="history-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     observes="viewHistorySidebar" label="&historyButton.label;"
+                     tooltiptext="&historyButton.tooltip;"/>
+
+      <toolbarbutton id="bookmarks-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     observes="viewBookmarksSidebar"
+                     tooltiptext="&bookmarksButton.tooltip;"
+                     ondrop="bookmarksButtonObserver.onDrop(event)"
+                     ondragover="bookmarksButtonObserver.onDragOver(event)"
+                     ondragenter="bookmarksButtonObserver.onDragOver(event)"
+                     ondragleave="bookmarksButtonObserver.onDragLeave(event)"/>
+
+      <toolbarbutton id="new-tab-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     label="&tabCmd.label;"
+                     command="cmd_newNavigatorTab"
+                     tooltiptext="&newTabButton.tooltip;"
+                     ondrop="newTabButtonObserver.onDrop(event)"
+                     ondragover="newTabButtonObserver.onDragOver(event)"
+                     ondragenter="newTabButtonObserver.onDragOver(event)"
+                     ondragleave="newTabButtonObserver.onDragLeave(event)"/>
+
+      <toolbarbutton id="new-window-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     label="&newNavigatorCmd.label;"
+                     command="key_newNavigator"
+                     tooltiptext="&newWindowButton.tooltip;"
+                     ondrop="newWindowButtonObserver.onDrop(event)"
+                     ondragover="newWindowButtonObserver.onDragOver(event)"
+                     ondragenter="newWindowButtonObserver.onDragOver(event)"
+                     ondragleave="newWindowButtonObserver.onDragLeave(event)"/>
+
+      <toolbarbutton id="cut-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     label="&cutCmd.label;"
+                     command="cmd_cut"
+                     tooltiptext="&cutButton.tooltip;"/>
+
+      <toolbarbutton id="copy-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     label="&copyCmd.label;"
+                     command="cmd_copy"
+                     tooltiptext="&copyButton.tooltip;"/>
+
+      <toolbarbutton id="paste-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     label="&pasteCmd.label;"
+                     command="cmd_paste"
+                     tooltiptext="&pasteButton.tooltip;"/>
+      <toolbarbutton id="fullscreen-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     observes="View:FullScreen"
+                     type="checkbox"
+                     label="&fullScreenCmd.label;"
+                     tooltiptext="&fullScreenButton.tooltip;"/>
+    </toolbarpalette>
   </toolbox>
 
   <hbox flex="1" id="browser">

toolkit/content/customizeToolbar.js

@@ -806,10 +806,8 @@ function onToolbarDrop(aEvent)
     if (wrapper == gCurrentDragOverItem)
        return;
 
-    // Don't allow static kids (e.g., the menubar) to move.
-    if (wrapper.parentNode.firstPermanentChild && wrapper.parentNode.firstPermanentChild.id == wrapper.firstChild.id)
-      return;
-    if (wrapper.parentNode.lastPermanentChild && wrapper.parentNode.lastPermanentChild.id == wrapper.firstChild.id)
+    // Don't allow non-removable kids (e.g., the menubar) to move.
+    if (wrapper.firstChild.getAttribute("removable") != "true")
       return;
 
     // Remove the item from its place in the toolbar.
@@ -905,10 +903,8 @@ function onPaletteDrop(aEvent)
 
   var wrapper = gToolboxDocument.getElementById("wrapper-"+itemId);
   if (wrapper) {
-    // Don't allow static kids (e.g., the menubar) to move.
-    if (wrapper.parentNode.firstPermanentChild && wrapper.parentNode.firstPermanentChild.id == wrapper.firstChild.id)
-      return;
-    if (wrapper.parentNode.lastPermanentChild && wrapper.parentNode.lastPermanentChild.id == wrapper.firstChild.id)
+    // Don't allow non-removable kids (e.g., the menubar) to move.
+    if (wrapper.firstChild.getAttribute("removable") != "true")
       return;
 
     var wrapperType = wrapper.getAttribute("type");

toolkit/content/tests/widgets/Makefile.in

@@ -88,6 +88,7 @@ _TEST_FILES = 	test_bug360220.xul \
 		test_textbox_emptytext.xul \
 		test_textbox_number.xul \
 		test_textbox_search.xul \
+		test_toolbar.xul \
 		xul_selectcontrol.js \
 		test_popupincontent.xul \
 		test_panelfrommenu.xul \

toolkit/content/tests/widgets/test_toolbar.xul

@@ -0,0 +1,226 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
+<?xml-stylesheet href="/tests/SimpleTest/test.css" type="text/css"?>
+
+<window title="Toolbar" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+
+  <toolbox id="toolbox">
+    <toolbarpalette>
+      <toolbarbutton id="p1" label="p1"/>
+      <toolbarbutton id="p2" label="p2"/>
+      <toolbarbutton id="p3" label="p3"/>
+      <toolbarbutton id="p4" label="p4"/>
+      <toolbarbutton id="p5" label="p5"/>
+      <toolbarbutton id="p6" label="p6"/>
+      <toolbarbutton id="p7" label="p7"/>
+      <toolbarbutton id="p8" label="p8"/>
+      <toolbarbutton id="p9" label="p9"/>
+      <toolbarbutton id="p10" label="p10"/>
+      <toolbarbutton id="p11" label="p11"/>
+      <toolbarbutton id="p12" label="p12"/>
+    </toolbarpalette>
+
+    <toolbar id="tb1" defaultset="p1,p2"/>
+    <toolbar id="tb2" defaultset="p4,p3"/>
+    <toolbar id="tb3" defaultset="p5,p6,t31">
+      <toolbarbutton id="t31" label="t31" removable="true"/>
+    </toolbar>
+    <toolbar id="tb4" defaultset="t41,p7,p8">
+      <toolbarbutton id="t41" label="t41" removable="true"/>
+    </toolbar>
+    <toolbar id="tb5" defaultset="p9,t51,p10">
+      <toolbarbutton id="t51" label="t51" removable="true"/>
+    </toolbar>
+
+    <toolbar id="tb-test" defaultset="p11,p12"/>
+    <toolbar id="tb-test2" defaultset=""/>
+    <!-- fixed toolbarbuttons always have 'fixed' in their id -->
+    <toolbar id="tb-test3" defaultset="">
+      <toolbarbutton id="tb-fixed-1" label="tb-test3-1"/>
+      <toolbarbutton id="tb-fixed-2" label="tb-test3-2" removable="false"/>
+      <toolbarbutton id="tb-fixed-3" label="tb-test3-3"/>
+    </toolbar>
+  </toolbox>
+
+  <toolbar id="notoolbox"/>
+
+  <!-- test resuls are displayed in the html:body -->
+  <body xmlns="http://www.w3.org/1999/xhtml"
+        style="height: 300px; overflow: auto;"/>
+  
+  <!-- test code goes here -->
+  <script type="text/javascript"><![CDATA[
+    const SPACER = /^spacer\d+/;
+    const SEPARATOR = /^separator\d+/;
+    const SPLITTER = /^splitter\d+/;
+    const SPRING = /^spring\d+/;
+
+    function testSet(aTb, aIDs, aResultIDs, aUseFixed) {
+      // build a list of the fixed items in the order they appear on the toolbar
+      var fixedSet = [];
+      if (aUseFixed) {
+        for (let i = 0; i < aTb.childNodes.length; i++) {
+          var id = aTb.childNodes[i].id;
+          if (id.indexOf("fixed") >= 0)
+            fixedSet.push(id);
+        }
+      }
+
+      var currentSet = aIDs.join(",");
+      ok(currentSet, "setting currentSet: " + currentSet);
+      aTb.currentSet = currentSet;
+      var resultIDs = aResultIDs || aIDs;
+      checkSet(aTb, resultIDs, fixedSet);
+    }
+
+    var checkSetCount = 0;
+    function checkSet(aTb, aResultIDs, aFixedSet) {
+      checkSetCount++;
+      var testID = "checkSet(" + checkSetCount + ") ";
+
+      for (let i = 0; i < aTb.childNodes.length; i++) {
+        let id = aTb.childNodes[i].id;
+        if (aResultIDs[i] instanceof RegExp) {
+          ok(aResultIDs[i].test(id),
+             testID + "correct ID " + aResultIDs[i] + " for toolbar " + aTb.id + "; got: " + id);
+        }
+        else if (aResultIDs[i] == "*") {
+          is(id, aFixedSet.shift(), testID + "is fixed with ID " + id + " for toolbar " + aTb.id);
+        }
+        else {
+          is(id, aResultIDs[i],
+             testID + "correct ID " + aResultIDs[i] + " for toolbar " + aTb.id + 
+             "****" + aResultIDs + "," + i + ",");
+          // remove the item from the fixed set once found
+          if (aFixedSet && id.indexOf("fixed") >= 0)
+            aFixedSet.splice(aFixedSet.indexOf(id), 1);
+        }
+      }
+
+      if (aFixedSet)
+        is(aFixedSet.length, 0, testID + "extra fixed items for " + aTb.id);
+      is(aTb.childNodes.length, aResultIDs.length,
+         testID + "correct number of children for " + aTb.id);
+    }
+
+    function test_defaultSet() {
+      checkSet($("tb1"), ["p1", "p2"]);
+      checkSet($("tb2"), ["p4", "p3"]);
+      checkSet($("tb3"), ["p5", "p6", "t31"]);
+      checkSet($("tb4"), ["t41", "p7", "p8"]);
+      checkSet($("tb5"), ["p9", "t51", "p10"]);
+    }
+
+    function test_currentSet(aTb) {
+      ok(aTb, "have toolbar");
+      var defaultSet = aTb.getAttribute("defaultset");
+      var setLength = (defaultSet && defaultSet.split(",").length) || 0;
+      is(setLength, aTb.childNodes.length, "correct # of children initially");
+
+      var emptySet = [["__empty"], []];
+      var testSets = [
+         emptySet,
+         [["p11"]],
+         [["p11","p12"]],
+         [["p11","p12","bogus"], ["p11","p12"]],
+         [["p11"]],
+         emptySet,
+         [["spacer"],    [SPACER]],
+         [["spring"],    [SPRING]],
+         [["separator"], [SEPARATOR]],
+         [["splitter"],  [SPLITTER]],
+         [["p11", "p11", "p12", "spacer", "p11"], ["p11", "p12", SPACER]],
+         [["splitter", "separator", "p11", "spring", "spacer"],
+          [SPLITTER, SEPARATOR, "p11", SPRING, SPACER]],
+         [["splitter", "spacer", "splitter", "p11", "spring", "spacer", "p12", "spring"],
+          [SPLITTER, SPACER, SPLITTER, "p11", SPRING, SPACER, "p12", SPRING]],
+         emptySet
+      ];
+
+      cycleSets(aTb, testSets, emptySet, false);
+    }
+
+    function test_currentSet_nonremovable() {
+      var tb = $("tb-test3");
+      ok(tb, "have tb-test-3");
+
+      // the * used in the tests below means that any fixed item can appear in that position
+      var emptySet = [["__empty"], ["*", "*", "*"]];
+      var testSets = [
+        [["p1", "tb-fixed-1", "p2"],
+         ["p1", "tb-fixed-1", "p2", "*", "*"]],
+        [["p1", "tb-fixed-2", "p2"],
+         ["p1", "tb-fixed-2", "p2", "*", "*"]],
+        [["p1", "tb-fixed-3", "p2"],
+         ["p1", "tb-fixed-3", "p2", "*", "*"]],
+        emptySet,
+
+        [["tb-fixed-1", "tb-fixed-2", "tb-fixed-3"],
+         ["tb-fixed-1", "tb-fixed-2", "tb-fixed-3"]],
+        [["tb-fixed-3", "tb-fixed-2", "tb-fixed-1"],
+         ["tb-fixed-3", "tb-fixed-2", "tb-fixed-1"]],
+
+        [["tb-fixed-1", "tb-fixed-2", "tb-fixed-3", "p1", "p2"],
+         ["tb-fixed-1", "tb-fixed-2", "tb-fixed-3", "p1", "p2"]],
+
+        [["tb-fixed-1", "p2", "p1"],
+         ["tb-fixed-1", "p2", "p1", "*", "*"]],
+
+        [["tb-fixed-1", "p2"],
+         ["tb-fixed-1", "p2", "*", "*"]],
+
+        [["p1", "p2"], ["p1", "p2", "*", "*", "*"]],
+        [["p2", "p1"], ["p2", "p1", "*", "*", "*"]],
+
+        [["tb-fixed-3", "spacer", "p1"],
+         ["tb-fixed-3", SPACER, "p1", "*", "*"]]
+      ];
+
+      cycleSets(tb, testSets, emptySet, true);
+    }
+
+    function cycleSets(aTb, aSets, aEmptySet, aUseFixed) {
+      // Since a lot of the tricky cases handled in the currentSet setter
+      // depend on going from one state to another, run through the test set
+      // multiple times in different orders.
+      var length = aSets.length;
+
+      for (var i = 0; i < length; i++) {
+        testSet(aTb, aSets[i][0], aSets[i][1], aUseFixed);
+      }
+      for (var i = length - 1; i >= 0; i--) {
+        testSet(aTb, aSets[i][0], aSets[i][1], aUseFixed);
+      }
+      for (var i = 0; i < length; i++) {
+        testSet(aTb, aSets[i][0], aSets[i][1], aUseFixed);
+        testSet(aTb, aSets[length - i - 1][0], aSets[length - i - 1][1], aUseFixed);
+        testSet(aTb, aSets[i][0], aSets[i][1], aUseFixed);
+        testSet(aTb, aSets[i][0], aSets[i][1], aUseFixed);
+      }
+      for (var i = 0; i < length; i++) {
+        testSet(aTb, aEmptySet[0], aEmptySet[1], aUseFixed);
+        testSet(aTb, aSets[i][0], aSets[i][1], aUseFixed);
+      }
+    }
+
+    test_defaultSet();
+    test_currentSet($("tb-test"));
+    test_currentSet($("tb-test2"));
+    test_currentSet_nonremovable();
+
+    var toolbox = $("toolbox");
+    var toolbars = document.getElementsByTagName("toolbar");
+    for (var t = 0; t < toolbars.length; t++) {
+      var toolbar = toolbars[t];
+      is(toolbar.toolbox, toolbar.id == "notoolbox" ? null : toolbox,
+         "toolbar " + toolbar.id + " has correct toolbox");
+    }
+
+    $("tb1").toolbox = document.documentElement;
+    is($("tb1").toolbox, toolbox, "toolbox still correct after set");
+
+  ]]></script>
+</window>

toolkit/content/widgets/toolbar.xml

@@ -90,40 +90,25 @@
           return Components.interfaces.nsIAccessibleProvider.XULToolbar;
         </getter>
       </property>
-      <field name="firstPermanentChild">
-        null
-      </field>
-      <field name="lastPermanentChild">
-        null
-      </field>
       
       <property name="toolbarName"
                 onget="return this.getAttribute('toolbarname');"
                 onset="this.setAttribute('toolbarname', val); return val;"/>
-      
+
+      <field name="toolbox" readonly="true">
+      <![CDATA[
+        this.parentNode && this.parentNode.localName == "toolbox" ? this.parentNode : null;
+      ]]>
+      </field>
+
       <constructor>
         <![CDATA[
-          if (this.hasAttribute("firstpermanentchild")) {
-            // The toolbar is being reconstructed. The original first and last
-            // permanent child nodes need to be restored, the rest of the
-            // constructor can be skipped.
-
-            this.firstPermanentChild =
-              this.childNodes.item(this.getAttribute("firstpermanentchild"));
-            this.lastPermanentChild =
-              this.childNodes.item(this.getAttribute("lastpermanentchild"));
-            return;
-          }
-
-          this.firstPermanentChild = this.firstChild;
-          this.lastPermanentChild = this.lastChild;
-          this.setAttribute("firstpermanentchild", this.firstChild ? 0 : -1);
-          this.setAttribute("lastpermanentchild", this.childNodes.length -1);
-
           // Searching for the toolbox palette in the toolbar binding because
           // toolbars are constructed first.
-          var toolbox = this.parentNode;
-          
+          var toolbox = this.toolbox;
+          if (!toolbox)
+            return;
+
           if (!toolbox.palette) {
             // Look to see if there is a toolbarpalette.
             var node = toolbox.firstChild;
@@ -137,10 +122,10 @@
               return;
 
             // Hold on to the palette but remove it from the document.
-            toolbox.palette = node;       
+            toolbox.palette = node;
             toolbox.removeChild(node);
           }
-          
+
           // Build up our contents from the palette.
           var currentSet = this.getAttribute("currentset");
           if (!currentSet)
@@ -150,145 +135,182 @@
         ]]>
       </constructor>
 
+      <method name="_idFromNode">
+        <parameter name="aNode"/>
+        <body>
+        <![CDATA[
+          switch (aNode.localName) {
+            case "toolbarseparator":
+              return "separator";
+            case "toolbarspring":
+              return "spring";
+            case "toolbarspacer":
+              return "spacer";
+            case "splitter":
+              return "splitter";
+            default:
+              return aNode.id;
+          }
+        ]]>
+        </body>
+      </method>
+
       <property name="currentSet">
         <getter>
           <![CDATA[
-            var currentSet = "";
-            for (let i = 0, node; node = this.childNodes.item(i); i++) {
-              if (node == this.firstPermanentChild)
-                this.setAttribute("firstpermanentchild", i);
-              if (node == this.lastPermanentChild)
-                this.setAttribute("lastpermanentchild", i);
-
-              if (node.id &&
-                  node.localName == "toolbaritem" || 
-                  node.localName == "toolbarbutton" ||
-                  node.localName == "toolbarseparator" ||
-                  node.localName == "toolbarspring" ||
-                  node.localName == "toolbarspacer")
-              {
-                if (currentSet)
-                  currentSet += ",";
-
-                if (node.localName == "toolbarseparator")
-                  currentSet += "separator";
-                else if (node.localName == "toolbarspring")
-                  currentSet += "spring";
-                else if (node.localName == "toolbarspacer")
-                  currentSet += "spacer";
-                else
-                  currentSet += node.id;
+            var node = this.firstChild;
+            var currentSet = [];
+            while (node) {
+              var id = this._idFromNode(node);
+              if (id) {
+                currentSet.push(id);
               }
+              node = node.nextSibling;
             }
 
-            if ("currentSetChanged" in this)
-              this.currentSetChanged(currentSet);
-
-            return currentSet ? currentSet : "__empty";
+            return currentSet.join(",") || "__empty";
           ]]>
         </getter>
-        
+
         <setter>
           <![CDATA[
-            var palette = this.parentNode.palette;
-            // Remove all items before the first permanent child and after the last permanent child.
-            while (this.lastChild != this.lastPermanentChild) {
-              switch (this.lastChild.localName) {
-                case "toolbarseparator":
-                case "toolbarspring":
-                case "toolbarspacer":
-                  this.removeChild(this.lastChild);
-                  break;
-                default:
-                  palette.appendChild(this.lastChild);
+            var ids = (val == "__empty") ? [] : val.split(",");
+
+            var nodeidx = 0;
+            var paletteItems = { }, added = { };
+
+            var palette = this.toolbox ? this.toolbox.palette : null;
+
+            // build a cache of items in the toolbarpalette
+            var paletteChildren = palette ? palette.childNodes : [];
+            for (let c = 0; c < paletteChildren.length; c++) {
+              let curNode = paletteChildren[c];
+              paletteItems[curNode.id] = curNode;
+            }
+
+            var children = this.childNodes;
+
+            iter:
+            // iterate over the ids to use on the toolbar
+            for (let i = 0; i < ids.length; i++) {
+              let  id = ids[i];
+
+              // iterate over the existing nodes on the toolbar. nodeidx is the
+              // spot where we want to insert items.
+              for (let c = nodeidx; c < children.length; c++) {
+                let curNode = children[c];
+                if (this._idFromNode(curNode) == id) {
+                  // the node already exists. If c equals nodeidx, we haven't
+                  // iterated yet, so the item is already in the right position.
+                  // Otherwise, insert it here.
+                  if (c != nodeidx) {
+                    this.insertBefore(curNode, children[nodeidx]);
+                  }
+
+                  added[curNode.id] = true;
+                  nodeidx++;
+                  continue iter; // move on to the next id
+                }
+              }
+
+              // the node isn't already on the toolbar, so add a new one.
+              var nodeToAdd = paletteItems[id] || this._getToolbarItem(id);
+              if (nodeToAdd && !(nodeToAdd.id in added)) {
+                added[nodeToAdd.id] = true;
+                this.insertBefore(nodeToAdd, children[nodeidx] || null);
+                nodeToAdd.setAttribute("removable", "true");
+                nodeidx++;
               }
             }
 
-            while (this.firstChild != this.firstPermanentChild) {
-              switch (this.firstChild.localName) {
-                case "toolbarseparator":
-                case "toolbarspring":
-                case "toolbarspacer":
-                  this.removeChild(this.firstChild);
-                  break;
-                default:
-                  palette.appendChild(this.firstChild);
-              }
-            }
+            // remove any leftover removable nodes
+            for (let i = children.length - 1; i >= nodeidx; i--) {
+              let curNode = children[i];
 
-            var firstChildID = this.firstPermanentChild ? this.firstPermanentChild.id : "";
-            var lastChildID = this.lastPermanentChild ? this.lastPermanentChild.id : "";
-
-            if (val && val != "__empty") {
-              var itemIds = val.split(",");
-              var before = true;
-              for (var i = 0; i < itemIds.length; i++) {
-                if (itemIds[i] == firstChildID || itemIds[i] == lastChildID)
-                  before = false;
+              let curNodeId = this._idFromNode(curNode);
+              // skip over fixed items
+              if (curNodeId && curNode.getAttribute("removable") == "true") {
+                if (palette)
+                  palette.appendChild(curNode);
                 else
-                  this.insertItem(itemIds[i], null, null, before);
+                  this.removeChild(curNode);
               }
             }
 
-            if (this.firstPermanentChild) {
-              this.setAttribute("firstpermanentchild",
-                                Array.indexOf(this.childNodes, this.firstPermanentChild));
-              this.setAttribute("lastpermanentchild",
-                                Array.indexOf(this.childNodes, this.lastPermanentChild));
-            }
-
-            if ("currentSetChanged" in this)
-              this.currentSetChanged(val);
-
             return val;
           ]]>
         </setter>
       </property>
-      
+
+      <method name="_getToolbarItem">
+        <parameter name="aId"/>
+        <body>
+          <![CDATA[
+            const XUL_NS = "http://www.mozilla.org/keymaster/" +
+                           "gatekeeper/there.is.only.xul";
+
+            var newItem = null;
+            switch (aId) {
+              // Handle special cases
+              case "separator":
+              case "spring":
+              case "spacer":
+                newItem = document.createElementNS(XUL_NS, "toolbar" + aId);
+                newItem.id = aId + Date.now() + this.childNodes.length;
+                if (aId == "spring")
+                  newItem.flex = 1;
+                break;
+              case "splitter":
+                newItem = document.createElementNS(XUL_NS, "splitter");
+                newItem.id = aId + Date.now() + this.childNodes.length;
+                newItem.className = "toolbar-splitter";
+                break;
+              default:
+                var toolbox = this.toolbox;
+                if (!toolbox)
+                  break;
+
+                // look for an item with the same id, as the item may be
+                // in a different toolbar.
+                var item = document.getElementById(aId);
+                if (item && item.parentNode && item.parentNode.parentNode == toolbox) {
+                  newItem = item;
+                  break;
+                }
+
+                if (toolbox.palette) {
+                  // Attempt to locate an item with a matching ID within
+                  // the palette.
+                  let paletteItem = this.toolbox.palette.firstChild;
+                  while (paletteItem) {
+                    if (paletteItem.id == aId) {
+                      newItem = paletteItem;
+                      break;
+                    }
+                    paletteItem = paletteItem.nextSibling;
+                  }
+                }
+                break;
+            }
+
+            return newItem;
+          ]]>
+        </body>
+      </method>
+
       <method name="insertItem">
         <parameter name="aId"/>
         <parameter name="aBeforeElt"/>
         <parameter name="aWrapper"/>
-        <parameter name="aBeforePermanent"/>
         <body>
           <![CDATA[
-            var newItem = null;
-            
-            // Create special cases of palette items.
-            var uniqueId;
-            if (aId == "separator") {
-              newItem = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
-                                                 "toolbarseparator");
-              uniqueId = (new Date()).getTime()+this.childNodes.length;
-              newItem.id = "separator" + uniqueId;
-              newItem.className = "chromeclass-toolbar-additional";
-            } else if (aId == "spring") {
-              newItem = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
-                                                 "toolbarspring");
-              uniqueId = (new Date()).getTime()+this.childNodes.length;
-              newItem.flex = 1;
-              newItem.id = "spring" + uniqueId;
-              newItem.className = "chromeclass-toolbar-additional";
-            } else if (aId == "spacer") {
-              newItem = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
-                                                 "toolbarspacer");
-              uniqueId = (new Date()).getTime()+this.childNodes.length;
-              newItem.id = "spacer" + uniqueId;
-              newItem.className = "chromeclass-toolbar-additional";
-            } else if (this.parentNode.localName == "toolbox") {
-              // Attempt to locate an item with a matching id within document.
-              newItem = document.getElementById(aId);
-              if (!newItem || newItem.parentNode.parentNode != this.parentNode) {
-                // Attempt to locate an item with a matching id within palette.
-                // The palette does not live in the document by default.
-                newItem = this.parentNode.palette
-                              .getElementsByAttribute("id", aId).item(0);
-                if (!newItem || newItem.parentNode != this.parentNode.palette)
-                  return false;
-              }
-            }
+            var newItem = this._getToolbarItem(aId);
+            if (!newItem)
+              return null;
 
             var insertItem = newItem;
+            // make sure added items are removable
+            newItem.setAttribute("removable", "true");
             
             // Wrap the item in another node if so inclined.
             if (aWrapper) {
@@ -299,8 +321,6 @@
             // Insert the palette item into the toolbar.
             if (aBeforeElt)
               this.insertBefore(insertItem, aBeforeElt);
-            else if (aBeforePermanent && this.firstPermanentChild)
-              this.insertBefore(insertItem, this.firstPermanentChild);
             else
               this.appendChild(insertItem);