308068 - JavaScript strict warning in richlistbox.xml

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Simon Bünzli]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050910 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050910 Firefox/1.4

A strict warning is thrown, whenever the selectedItem field of a richlistbox is
set to null (e.g. in clearSelection). A patch is following.

Reproducible: Always

Steps to Reproduce:
1. Make sure javascript.options.strict and javascript.options.showInConsole are
set to true (in about:config)
2. Open the JavaScript Console and the Extensions Manager
3. Hit [Home] or [End] in the Extensions Manager

Actual Results:  
The following two strict warnings are thrown:

Warning: reference to undefined property children[run]
Source File: chrome://global/content/bindings/richlistbox.xml
Line: 331

Warning: reference to undefined property children[run]
Source File: chrome://global/content/bindings/richlistbox.xml
Line: 335

Expected Results:  
No console cluttering.

Comment 1

[Simon Bünzli]

Attachment: Code simplification, fixes strict warning

The problem can be fixed by reversing the equality and the bounds check in
getIndexOf. By testing for null at the beginning, the problem also goes away.

Since getIndexOf and the selectedIndex getter share much common code, I decided
to unify them and to combine both possible fixes for stability and speed. Hope
the code remains clear enough.

Comment 2

[Doron Rosenberg (IBM)]

Comment on attachment 195681 [details] [diff] [review]
Code simplification, fixes strict warning

code gets somewhat unreadable, but doesn't take too much work to figure it out.

Comment 3

[Simon Bünzli]

Comment on attachment 195681 [details] [diff] [review]
Code simplification, fixes strict warning

Any suggestions on how to make the code better readable?

Comment 4

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

What's wrong with using a for loop? Makes it much more readable IMO...

Comment 5

[Simon Bünzli]

Attachment: Using a loop for improved readability

Comment 6

[Simon Bünzli]

Attachment: using Array.indexOf for trivial solution

Thinking it over, the solution seems to be a one-liner when using the new
Array.indexOf. The null-test remains for performance reasons (the replaced
selectedIndex getter did the same test).

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 195681 [details] [diff] [review]
Code simplification, fixes strict warning

>-            while (children[run] != aElement && run < children.length) {
>-              run++;
>+            while (index < children.length && children[index] != aElement) {
>+              index++;
>             }
> 
>-            if (children[run] == aElement)
>-              index = run;
>-
>-            return index;
>+            return (index < children.length) ? index : -1;

I generally find the following style clearer in the return on match case

while (index < children.length) {
  if (children[index] == aElement)
    return index;

  index++
}

return -1;

r=me with that changed

Comment 8

[Simon Bünzli]

Comment on attachment 195851 [details] [diff] [review]
using Array.indexOf for trivial solution

I'd rather include this patch, anyway. Alternatively you might prefer
attachment 195745 [details] [diff] [review]. Please review one of them before I rewrite the original
patch.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 195851 [details] [diff] [review]
using Array.indexOf for trivial solution

not the time or the place to take this kind of fix. please do not request
approval for this kind of change when we're in lockdown mode. thanks.

Comment 10

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

Trunk:
mozilla/toolkit/content/widgets/richlistbox.xml; new revision: 1.22;

Comment 11

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

mozilla/toolkit/content/widgets/richlistbox.xml 	1.13.2.11