354048 - Firefox startup rebuilds toolbars

Status: RESOLVED FIXED

(Firefox :: General, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

I was profiling startup, and noticed that a few percent (hard to quantify exactly with C++-only profilers, but not more than 10%, and probably more than 1%) was spent in rebuilding the toolbars.  See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/toolbar.xml&rev=1.33#166

Anything we can do about making this code do less DOM munging?

Comment 1

[:Gavin Sharp [email: [e-mail]]]

So the basic design for customizable toolbars is that all available elements are children of the toolbarpalette, and at startup, the toolbar grabs elements from the palette based on the persisted "currentSet" attribute. To fix this, it seems to me like we should make it so that the default set are children of the toolbar, and have the toolbar constructor adjust the default set at startup instead of building the entire list. That way we avoid the DOM munging for default toolbars, and minimize it for almost-default toolbars, which is likely the majority case.

It might be tricky to do this in a backwards-compatible way, though; it seems like the current toolbars treat any children that are present when the binding constructor fires as "permanent" children. Perhaps I should just introduce a new toolbar binding ("toolbar-default", or something?) and make in-tree toolbars use it.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: WIP

Needs more work (toolbar customization is a bit broken, I think), but this should be useful for testing. It avoids moving lots of DOM nodes in the most common situation: a default or almost-default toolbar setup.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated

This takes a different approach, but seems to work better. I tried running it through the Txul test locally but it didn't appear to have any effect. Still need to iron out some kinks, I think.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated

I think this is ready for review, just need to double check a few more things.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: WIP, with tests

This one's a bit more complex, but was required for being compatible with the previous code. Also delayed building the customize palette until the user actually tries to customize the toolbar. Added some tests and code comments. Locally this gives me about a ~7% Txul win, but I need to look into a few possible toolbar customization problems and do a bit of cleanup.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This is ready for review, I think. There are a couple of issues that I also want to look into:
1) Toolbar customization could really use some tests
2) The other forked toolbarCustomization.js files probably also need the change I made to the toolkit one in this patch
3) This doesn't affect my local Txul measurements as much as I thought it would, but that could be because my local machine is too fast. I have some ideas on further performance improvements that are made easier once this patch is in (like not cloning items for the palette until we actually customize).

Comment 7

[Neil Deakin]

Comment on attachment 286859 [details] [diff] [review]
patch

That currentSet setter is way too unweildly. I spent some time coming up with a better one.

        <setter>
          <![CDATA[
            var ids = (val == "__empty") ? [] : val.split(",");

            var c = 0;
            var children = this.childNodes;
            iter:
            for (var i = 0; i < ids.length; i++) {
              var id = ids[i];
              var curnode = children[c] || null;
              if (!curnode) {
                var newitem = this._createToolbarItem(id);
                if (newitem) {
                  this.appendChild(newitem);
                  c++;
                }
              }
              else if (this._idFromNode(curnode) != id) {
                for (var findc = c + 1; findc < children.length; findc++) {
                  var findnode = children[findc];

                  if (this._idFromNode(findnode) == id) {
                    if (findnode.getAttribute("permanent") != "false") {
                      while (c < findc) {
                        if (children[c].getAttribute("permanent") == "false") {
                          this.removeChild(children[c]);
                          findc--;
                        }
                        else c++;
                      }

                      while (++c < children.length &&
                             children[c].getAttribute("permanent") != "false");
                    }
                    else {
                      this.insertBefore(findnode, curnode);
                      c++;
                    }

                    continue iter;
                  }
                }

                var newitem = this._createToolbarItem(id);
                if (newitem) {
                  this.insertBefore(newitem, curnode);
                  c++;
                }
              }
              else if (children[c].getAttribute("permanent") != "false") {
                while (++c < children.length &&
                       children[c].getAttribute("permanent") != "false");
              }
              else {
                c++;
              }
            }
            for (i = children.length - 1; i >= c; i--) {
              if (children[i].getAttribute("permanent") == "false")
                this.removeChild(children[i]);
            }

            return val;
          ]]>
        </setter>

It's a bit faster too. I didn't test the toolbar customization though.

The changes to the constructor and the firstPermanent/lastPermanent fields shouldn't be needed anymore, although maybe something is relying on this?

Index: toolkit/content/tests/widgets/test_toolbar.xul
===================================================================

I found that the testcase needed to run during a load handler, otherwise the test runs before the toolbar initializes.

+    function testSet(aTb, aIDs, aResultIDs) {
+      var resultIDs = aResultIDs || aIDs;
+      var currentSet = aIDs.join(",");
+      ok(currentSet, "setting currentSet: " + currentSet);
+      aTb.currentSet = currentSet;
+      checkSet(aTb, resultIDs);

Move the resultsIDs initializer down to just before checkSet. It took me a while to understand this function otherwise.

+    function test_currentSet_permanent() {
+        [["p1", "tb-test3-1", "p2"],
+         ["p1", "tb-test3-1", "tb-test3-2", "tb-test3-3", "p2"]],
+        [["p1", "tb-test3-2", "p2"],
+         ["p1", "p2", "tb-test3-1", "tb-test3-2", "tb-test3-3"]],

This second test is an odd result. I would expect it to place tb-teste-2 in between p1 and p2. With the modified currentSet setter, this is the case.

Maybe also add some tests for when the same id exists twice.

Index: toolkit/content/widgets/toolbar.xml
===================================================================
 
+      <method name="_idFromNode">
+        <parameter name="aNode"/>
+        <body>
+        <![CDATA[
+          if (!aNode.id)
+            return null;

The splitter added in browser.xul doesn't have an id. This code here just be removed and checked in the 'default' case below.

+            case "toolbarsplitter":
+              return "splitter";

there is no 'toolbarsplitter', it's just 'splitter'.

+            case "toolbarbutton":
+            case "toolbaritem":
+            default:
+              return aNode.id;

A bit redundant those first two case labels.

+            function createItem(aType, aUniqueNum, aLocalName) {
+              var XUL_NS = "http://www.mozilla.org/keymaster/" +
+                           "gatekeeper/there.is.only.xul";

const?

+              var item = document.createElementNS(XUL_NS, aLocalName);
+              item.id = aType + Date.now() + aUniqueNum;
+              if (aType == "spring")
+                item.flex = 1;
+              if (aType == "splitter")
+                item.className = "toolbar-splitter";

This part can just be moved down to inside the switch below, so it only checks it for those items.

+              return item;
+            }
+
+            switch (aId) {
+              // Handle special cases
+              case "separator":
+              case "spring":
+              case "spacer":
+                newItem = createItem(aId, this.childNodes.length, "toolbar" + aId);
+                break;
+              case "splitter":
+                newItem = createItem(aId, this.childNodes.length, "splitter");
+                break;
+              default:
+                if (this.toolbox.palette) {
+                  // Attempt to locate an item with a matching ID within
+                  // the palette.
+                  var paletteItem = this.toolbox.palette.firstChild;
+                  while (paletteItem) {
+                    if (paletteItem.id == aId) {
+                      newItem = paletteItem.cloneNode(true);
+                      break;
+                    }
+                    paletteItem = paletteItem.nextSibling;
+                  }
                 }
-                paletteItem = paletteItem.nextSibling;
-              }

Index: browser/base/content/browser.xul
===================================================================
              defaultset="menubar-items"
 #else
              defaultset="menubar-items,spring,throbber-box"
 #endif
              mode="icons" context="toolbar-context-menu">
+

Extra blank line added here

 
+      <!-- Adding this here to match our defaultSet allows the toolbar
+           contructor to take a shortcut and not rebuild toolbars. -->
+      <toolbarsplitter permanent="false"/>

splitter, not toolbarsplitter

+#ifdef XP_MACOSX
+      <toolbaritem id="throbber-box"
+                   title="&throbberItem.title;"
+                   align="center"
+                   pack="center"
+                   permanent="false">
         <button id="navigator-throbber" disabled="true"/>
       </toolbaritem>

There seems to be a missing #endif here

-#ifdef XP_MACOSX
-             defaultset="back-button,forward-button,reload-button,stop-button,home-button,urlbar-container,splitter,search-container,throbber-box"
 #else
-             defaultset="back-button,forward-button,reload-button,stop-button,home-button,urlbar-container,splitter,search-container,fullscreenflex,window-controls"
-#endif
-             context="toolbar-context-menu">
-#ifndef XP_MACOSX

This last line shouldn't be removed

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> (From update of attachment 286859 [details] [diff] [review])
> That currentSet setter is way too unweildly. I spent some time coming up with a
> better one.

> The changes to the constructor and the firstPermanent/lastPermanent fields
> shouldn't be needed anymore, although maybe something is relying on this?

Thanks, I'll look into it. It was never very clear how multiple "permanent children" were supposed to be supported, the current code seems to be mostly a hack to prevent the menubar from being customizable.

> +    function test_currentSet_permanent() {
> +        [["p1", "tb-test3-1", "p2"],
> +         ["p1", "tb-test3-1", "tb-test3-2", "tb-test3-3", "p2"]],
> +        [["p1", "tb-test3-2", "p2"],
> +         ["p1", "p2", "tb-test3-1", "tb-test3-2", "tb-test3-3"]],
> 
> This second test is an odd result. I would expect it to place tb-teste-2 in
> between p1 and p2. With the modified currentSet setter, this is the case.

Yeah, that's a result of http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/toolbar.xml&rev=1.38&mark=186#182  - it checks whether it's the first or the last, but it doesn't keep track of any "middle" permanent children. You're probably right that that behavior isn't worth keeping.

> Index: toolkit/content/widgets/toolbar.xml
> ===================================================================
> 
> +      <method name="_idFromNode">
> +        <parameter name="aNode"/>
> +        <body>
> +        <![CDATA[
> +          if (!aNode.id)
> +            return null;
> 
> The splitter added in browser.xul doesn't have an id. This code here just be
> removed and checked in the 'default' case below.

Another late change to the code that I didn't give too much thought to before I attached the patch :(

> +            case "toolbarsplitter":
> +              return "splitter";
> 
> there is no 'toolbarsplitter', it's just 'splitter'.

Yeah, this toolbarsplitter junk is the result of a mistake I made when I was simplifying getItem. I noticed it later when the splitter was not appearing in the toolbar, but I only partially fixed it (the "aLocalName" param to createItem() was the partial fix, at the time I was mostly just trying to rid myself of the patch and get your eyes on it).

> 
> +            case "toolbarbutton":
> +            case "toolbaritem":
> +            default:
> +              return aNode.id;
> 
> A bit redundant those first two case labels.

Sure, I left it that way because it was clearer what this function was expected to deal with, but that's probably silly.

> Index: browser/base/content/browser.xul

> +#ifdef XP_MACOSX
> +      <toolbaritem id="throbber-box"
> +                   title="&throbberItem.title;"
> +                   align="center"
> +                   pack="center"
> +                   permanent="false">
>          <button id="navigator-throbber" disabled="true"/>
>        </toolbaritem>
> 
> There seems to be a missing #endif here

No, it's just farther down in the diff because of all the removed lines.

> -#ifdef XP_MACOSX
> -            
> defaultset="back-button,forward-button,reload-button,stop-button,home-button,urlbar-container,splitter,search-container,throbber-box"
>  #else
> -            
> defaultset="back-button,forward-button,reload-button,stop-button,home-button,urlbar-container,splitter,search-container,fullscreenflex,window-controls"
> -#endif
> -             context="toolbar-context-menu">
> -#ifndef XP_MACOSX
> 
> This last line shouldn't be removed

Things make sense once you combine this comment with the previous one :) This code is now in the #else part of the #ifdef.

Comment 9

[Dietrich Ayala (:dietrich)]

Moving this up in priority, as Rob's logging on WinCE shows a huge hit here.

Comment 10

[Neil Deakin]

Attachment: improved toolbar initialization

Seems to work and the tests pass. Builds will appear at https://build.mozilla.org/tryserver-builds/[e-mail]-toolbarcurrentset/ and could use additional toolbar customization testing by others.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 424089 [details] [diff] [review]
improved toolbar initialization

This rips the back and forward buttons out of their container item, i.e. displays this:

#nav-bar
  #unified-back-forward-button
    #back-forward-dropmarker
  #back-button
  #forward-button

instead of:

#nav-bar
  #unified-back-forward-button
    #back-button
    #forward-button
    #back-forward-dropmarker

That's because of this: http://hg.mozilla.org/mozilla-central/annotate/bc6f2b598ff9/browser/base/content/browser.js#l3388
We can remove this code, but we'd still need to deal with "back-button,forward-button" being in the current set from previous releases.

Also, I seem to be unable to remove items from the toolbars while customizing -- I haven't yet tried to figure out why.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> That's because of this:
> http://hg.mozilla.org/mozilla-central/annotate/bc6f2b598ff9/browser/base/content/browser.js#l3388
> We can remove this code

filed bug 544659 on that

Comment 13

[Neil Deakin]

Attachment: fix back and forward handling, and dragging items between toolbars

Comment 14

[Philip Chee]

Sorry for the bugspam, but are there any plans to backport this to 1.9.2? I ask this because SeaMonkey/comm-central hasn't branched yet and AFAIK Thunderbird 3.1 will be comm-central+mozilla-1.9.2.

Comment 15

[Neil Deakin]

This is mainly just a performance improvement patch so unlikely.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 425816 [details] [diff] [review]
fix back and forward handling, and dragging items between toolbars 

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

You're actually allowing the menubar to move indirectly when other items are dropped before it, which used to be forbidden.
I think I'd prefer to keep it this way, although I also like the idea of getting rid of "firstpermanentchild"/"lastpermanentchild"...

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 425816 [details] [diff] [review]
fix back and forward handling, and dragging items between toolbars 

>+      <property name="toolbox" readonly="true">
>+        <getter><![CDATA[
>+          return this.parentNode;
>+        ]]></getter>
>+      </property>

Toolbars aren't required to be inside of toolboxes, so it seems like this should be null if the parentNode isn't a toolbox.

Comment 19

[Neil Deakin]

(In reply to comment #16)
> You're actually allowing the menubar to move indirectly when other items are
> dropped before it, which used to be forbidden.

What do you mean here? You can currently drop items before the menubar as well.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> (In reply to comment #16)
> > You're actually allowing the menubar to move indirectly when other items are
> > dropped before it, which used to be forbidden.
> 
> What do you mean here? You can currently drop items before the menubar as well.

Ok, I thought you couldn't.

Comment 21

[Dão Gottwald [:dao]]

The items in the palette all lack removable="true". Maybe this should be the default for all items? Or maybe it should be added automatically on items dragged from the palette to a toolbar?

Comment 22

[Neil Deakin]

The draggable attribute is set when an item is added to the toolbar (in the currentSet setter).

I can add them to the existing palette items as well if desired.

Comment 23

[Philip Chee]

Do we want the window-controls to be movable as well?

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #22)
> The draggable attribute is set when an item is added to the toolbar (in the
> currentSet setter).

That didn't work as expected then. I was unable to remove the Bookmarks and Downloads buttons.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 425816 [details] [diff] [review]
fix back and forward handling, and dragging items between toolbars 

(In reply to comment #24)
> That didn't work as expected then. I was unable to remove the Bookmarks and
> Downloads buttons.

I tested this again and it can be reproduced quite easily -- just move an item from the palette to a toolbar and try to remove it again.

Comment 26

[Neil Deakin]

Attachment: address comments, fix dragging around on toolbars and palette

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 427118 [details] [diff] [review]
address comments, fix dragging around on toolbars and palette 

>+    <toolbarpalette id="BrowserToolbarPalette">

This seems to be positioned arbitrarily between the toolbars. Can you make it the last child of the toolbox?

>+      <property name="toolbox" readonly="true">
>+        <getter><![CDATA[
>+          var parent = this.parentNode;
>+          return parent.localName == "toolbox" ? parent : null;
>+        ]]></getter>
>+      </property>

This can be a readonly field. The parent node isn't going to change without the binding being deconstructed.

>+            // build a cache of items in the toolbarpalette
>+            var paletteChildren = palette ? palette.childNodes : [];
>+            for (c = 0; c < paletteChildren.length; c++) {

c is declared in another for-loop below, which is slightly confusing. You should just use 'let' in such cases.

I'm trying to figure out how my nav-bar lost its fullscreenflex and window-controls. Could one of the previous patches have done this? Restoring the default set brought them back.

Comment 28

[Neil Deakin]

(In reply to comment #27)
> This can be a readonly field. The parent node isn't going to change without the
> binding being deconstructed.

Fields don't support being readonly though.


> I'm trying to figure out how my nav-bar lost its fullscreenflex and
> window-controls. Could one of the previous patches have done this? Restoring
> the default set brought them back.

Possibly. I can't find a way to reproduce this problem.

With these changes, is everything else ok with this patch?

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Fields don't support being readonly though.

They do now, in fact.  See bug 542406.

Comment 30

[Neil Deakin]

Attachment: address comments

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 427645 [details] [diff] [review]
address comments

>-      <hbox id="fullscreenflex" flex="1" hidden="true" fullscreencontrol="true"/>
>-      <hbox id="window-controls" hidden="true" fullscreencontrol="true">
>+      <hbox id="fullscreenflex" flex="1" hidden="true"
>+            fullscreencontrol="true" removable="true"/>
>+      <hbox id="window-controls" hidden="true"
>+            fullscreencontrol="true" removable="true">

These shouldn't be removable.

>       <constructor>

>           // Searching for the toolbox palette in the toolbar binding because
>           // toolbars are constructed first.
>-          var toolbox = this.parentNode;
>-          
>-          if (!toolbox.palette) {
>+          var toolbox = this.toolbox;
>+          if (toolbox && !toolbox.palette) {
>             // Look to see if there is a toolbarpalette.
>             var node = toolbox.firstChild;
>             while (node) {
>               if (node.localName == "toolbarpalette")
>                 break;
>               node = node.nextSibling;
>             }
>             
>             if (!node)
>               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)
>             currentSet = this.getAttribute("defaultset");
>           if (currentSet)
>             this.currentSet = currentSet;

This returns early if it doesn't find a palette. Can you similarly return if there's no toolbox?

>       <property name="currentSet">
>         <getter>
>           <![CDATA[
>+            var node = this.firstChild;
>             var currentSet = "";

>+            while (node) {
>+              var id = this._idFromNode(node);
>+              if (id) {
>+                if (currentSet)
>+                  currentSet += "," + id;
>+                else
>+                 currentSet += id;
>+              }
>+              node = node.nextSibling;
>+            }

>+            return currentSet || "__empty";

nit: missing space before currentSet += id;. You could make make currentSet an array and return currentSet.join(",") || "__empty"

>+            for (let c = 0; c < paletteChildren.length; c++) {
>+              curNode = paletteChildren[c];
>+              paletteItems[curNode.id] = curNode;
>+            }

let curNode? I don't think this is the curNode you want to use below.

>+            // build a cache of fixed (non-removable) items on the toolbar
>+            var children = this.childNodes;
>+            for (let c = 0; c < children.length; c++) {
>+              curNode = children[c];
>+              var curNodeId = this._idFromNode(curNode);
>+              // the last condition here skips over spacers, splitters, etc
>+              if ((!curNodeId || (curNode.getAttribute("removable") != "true")) &&
>+                  !(curNodeId in paletteItems) && curNodeId == curNode.id) {
>+                fixed[curNodeId] = true;
>               }
>             }

Same here.

>+            iter:
>+            // iterate over the ids to use on the toolbar
>+            for (let i = 0; i < ids.length; i++) {
>+              var id = ids[i];
>+              // if one of the fixed nodes was encountered, add all of them in sequence

So if there were A,B,C with A and C being fixed, this would result in A,C,B... r- because if this.

>+              if (id in fixed && !(id in added)) {
>+                for (var p = nodeidx; p < children.length; p++) {
>+                  if (children[p].id in fixed) {
>+                    added[curNode.id] = true;
>+                    this.insertBefore(children[p], children[nodeidx++]);
>+                  }
>+                }

Here I think you want let curNode = children[p]?
This is probably where fullscreenflex and window-controls got lost.

>+      <method name="_getToolbarItem">

>+              default:
>+                if (this.parentNode.localName == "toolbox") {
>+                  // 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.parentNode == this.parentNode)
>+                    return item;
>+                }
>+                if (this.toolbox && this.toolbox.palette) {
>+                  // Attempt to locate an item with a matching ID within
>+                  // the palette.
>+                  var paletteItem = this.toolbox.palette.firstChild;
>+                  while (paletteItem) {
>+                    if (paletteItem.id == aId) {
>+                      newItem = paletteItem;
>+                      break;
>+                    }
>+                    paletteItem = paletteItem.nextSibling;
>+                  }
>+                }
>+                break;

Looks like you could use this.toolbox instead of this this.parentNode and break early if it's null.

Comment 32

[Neil Deakin]

> So if there were A,B,C with A and C being fixed, this would result in A,C,B...
> r- because if this.

The primary issue is that not doing this creates unpredictable reasults. Not a big deal is practise, but tests will result in inconsistent results even for the same value of currentset.

For example if there are three fixed buttons (Fixed1, Fixed2, Fixed3):

1. setting currentset="__empty" will result in:
     "Fixed1 Fixed2 Fixed3"

2. whereas first setting currentset="B1 Fixed2 B2 B3" will result in:
     "B1 Fixed2 B2 B3 Fixed1 Fixed3"
   then setting currentset="__empty" will result in:
     "Fixed2 Fixed1 Fixed3"

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> > So if there were A,B,C with A and C being fixed, this would result in A,C,B...
> > r- because if this.
> 
> The primary issue is that not doing this creates unpredictable reasults. Not a
> big deal is practise, but tests will result in inconsistent results even for
> the same value of currentset.
> 
> For example if there are three fixed buttons (Fixed1, Fixed2, Fixed3):
> 
> 1. setting currentset="__empty" will result in:
>      "Fixed1 Fixed2 Fixed3"
> 
> 2. whereas first setting currentset="B1 Fixed2 B2 B3" will result in:
>      "B1 Fixed2 B2 B3 Fixed1 Fixed3"
>    then setting currentset="__empty" will result in:
>      "Fixed2 Fixed1 Fixed3"

Sounds ok to me. Presumably the consumer doesn't care about the position of those elements, otherwise they should be included in the currentset.

Comment 34

[Neil Deakin]

Attachment: update for comments

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 428270 [details] [diff] [review]
update for comments

>+            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)
>+                  return null;
>+
>+                // 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)
>+                  return item;

Can you decide for and stick to either 'break;' and 'newItem = foo; break;' or 'return null;' and 'return foo;'?

>+                if (toolbox.palette) {
>+                  // Attempt to locate an item with a matching ID within
>+                  // the palette.
>+                  var paletteItem = this.toolbox.palette.firstChild;
>+                  while (paletteItem) {
>+                    if (paletteItem.id == aId) {
>+                      newItem = paletteItem;
>+                      break;
>+                    }
>+                    paletteItem = paletteItem.nextSibling;
>+                  }
>+                }

I would generally appreciate the use of 'let' in such situations, in order to prevent bugs like in the previous revision... unless of course the variable is intended to be used outside of the block.

Comment 36

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/f08bba41b284

Comment 37

[[not reading bugmail]]

Does this need to be refactored after tabbar as a toolbar landed?