402419 - tabbox cleanup

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

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

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.

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 3

[Dão Gottwald [:dao]]

(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.

Comment 4

[Neil Deakin]

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

Comment 5

[Dão Gottwald [:dao]]

Attachment: synced with bug 386202

Comment 6

[Dão Gottwald [:dao]]

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 7

[Neil Deakin]

Comment on attachment 288807 [details] [diff] [review]
synced with bug 386202

Looks good to me

Comment 8

[Dão Gottwald [:dao]]

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 9

[Damon Sicore (:damons)]

Comment on attachment 288807 [details] [diff] [review]
synced with bug 386202

a=release drivers.

Comment 10

[Reed Loden [:reed]]

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