Comment on attachment 287296 [details] [diff] [review] patch This looks good overall. Two things I noticed, though: >Index: toolkit/content/widgets/tabbox.xml >- const tabs = this.childNodes; >- if (0 <= val && val < tabs.length && !tabs[val].selected) { >- >- for (var i = 0; i < tabs.length; i++) >- if (i != val && tabs[i].selected) >- tabs[i]._selected = false; >+ var tab = this.getItemAtIndex(val); >+ if (tab && !tab.selected) { >+ Array.forEach(this.childNodes, function (tab) { >+ tab._selected = false; >+ }); >+ tab._selected = true; The _selected setter on a tab is non-trivial, is it perhaps worth keeping the optimization that avoids setting the passed-in tab's _selected property twice (the i != val)? Also the one that avoids setting "_selected" if "selected" is already false? >- if (val && !val.selected) { >- const tabs = this.childNodes; >- for (var i = 0; i < tabs.length; i++) >- if (tabs[i] == val) >- this.selectedIndex = i; >- } >+ if (val && !val.selected) >+ this.selectedIndex = this.getIndexOfItem(val); > return val; This will set selectedIndex to -1 if val is a bogus non-null object, whereas the previous code left selectedIndex untouched in that case. I guess that's OK because the selectedIndex setter ignores invalid values, but it might be a good idea to make note of that with a comment.

(In reply to comment #1) > This will set selectedIndex to -1 if val is a bogus non-null object, whereas > the previous code left selectedIndex untouched in that case. I guess that's OK > because the selectedIndex setter ignores invalid values, but it might be a good > idea to make note of that with a comment. I think a comment is better, as the normal case will be that the passed item is a valid one.

Note that selectedIndex setter has changed recently, so you will need to update part of this patch.

Comment on attachment 288807 [details] [diff] [review] synced with bug 386202 Neil, could you please take a look at the selectedIndex setter? The change looks trivial, but I'd like to make sure I didn't miss anything.

Comment on attachment 288807 [details] [diff] [review] synced with bug 386202 Looks good to me

Comment on attachment 288807 [details] [diff] [review] synced with bug 386202 asking for approval for this patch where my focus was on performance and code simplification

Comment on attachment 288807 [details] [diff] [review] synced with bug 386202 a=release drivers.

Checking in toolkit/content/widgets/tabbox.xml; /cvsroot/mozilla/toolkit/content/widgets/tabbox.xml,v <-- tabbox.xml new revision: 1.51; previous revision: 1.50 done