Firefox startup rebuilds toolbars

RESOLVED FIXED

Status

()

defect
P2
normal
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: bzbarsky, Assigned: enndeakin)

Tracking

(Depends on 1 bug, {perf})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts][2010Q1])

Attachments

(1 attachment, 9 obsolete attachments)

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?
Assignee: nobody → gavin.sharp
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → Firefox 3 alpha1
Status: NEW → ASSIGNED
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.
Priority: P2 → P1
Target Milestone: Firefox 3 alpha1 → Firefox 3 M7
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Posted patch WIP (obsolete) — Splinter Review
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.
Posted patch updated (obsolete) — Splinter Review
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.
Attachment #279118 - Attachment is obsolete: true
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Posted patch updated (obsolete) — Splinter Review
I think this is ready for review, just need to double check a few more things.
Attachment #281900 - Attachment is obsolete: true
Posted patch WIP, with tests (obsolete) — Splinter Review
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.
Blocks: 397946, 397947
Posted patch patch (obsolete) — Splinter Review
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).
Attachment #283090 - Attachment is obsolete: true
Attachment #285790 - Attachment is obsolete: true
Attachment #286859 - Flags: review?(enndeakin)
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
Attachment #286859 - Flags: review?(enndeakin) → review-
(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.
Priority: P1 → --
Target Milestone: Firefox 3 beta1 → ---
Whiteboard: [ts][needs new patch]
Assignee: gavin.sharp → nobody
Priority: -- → P2
Moving this up in priority, as Rob's logging on WinCE shows a huge hit here.
Priority: P2 → P1
Priority: P1 → P2
Seems to work and the tests pass. Builds will appear at https://build.mozilla.org/tryserver-builds/neil@mozilla.com-toolbarcurrentset/ and could use additional toolbar customization testing by others.
Assignee: nobody → enndeakin
Attachment #286859 - Attachment is obsolete: true
Attachment #424089 - Flags: review?(dao)
Whiteboard: [ts][needs new patch] → [ts]
Attachment #424089 - Flags: review?(mano)
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.
Attachment #424089 - Flags: review?(dao) → review-
(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
Attachment #424089 - Attachment is obsolete: true
Attachment #425816 - Flags: review?(dao)
Attachment #424089 - Flags: review?(mano)
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.
This is mainly just a performance improvement patch so unlikely.
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 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.
Duplicate of this bug: 545428
(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.
(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.
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?
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.
Do we want the window-controls to be movable as well?
(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 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.
Attachment #425816 - Flags: review?(dao) → review-
Attachment #425816 - Attachment is obsolete: true
Attachment #427118 - Flags: review?(dao)
Attachment #425816 - Flags: review?(mano)
Whiteboard: [ts] → [ts][2010Q1]
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.
(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?
> Fields don't support being readonly though.

They do now, in fact.  See bug 542406.
Posted patch address comments (obsolete) — Splinter Review
Attachment #427118 - Attachment is obsolete: true
Attachment #427645 - Flags: review?(dao)
Attachment #427118 - Flags: review?(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.
Attachment #427645 - Flags: review?(dao) → review-
> 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"
(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.
Attachment #427645 - Attachment is obsolete: true
Attachment #428270 - Flags: review?(dao)
Blocks: 347930
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.
Attachment #428270 - Flags: review?(dao) → review+
http://hg.mozilla.org/mozilla-central/rev/f08bba41b284
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 550205
Does this need to be refactored after tabbar as a toolbar landed?
Depends on: 578032
Depends on: 579151
You need to log in before you can comment on or make changes to this bug.