237658 - In <pref-fonts.js>, "Warning: redeclaration of var dataEls"

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[timeless]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040315

Warning: redeclaration of var dataEls
Source File: chrome://communicator/content/pref/pref-fonts.js
Line: 373, Column: 17
Source Code:
             var dataEls = selectElement.listElement.getElementsByAttribute(
"value", selectVal );

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <pref-fonts.js>

Bug fix,
plus some cleanup.

(I tried to move the modified lines to the left, as the existing code
permitted...)

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 144073 [details] [diff] [review]
(Av1) <pref-fonts.js>

'r=?':
Could you review and test it ?

Comment 3

[[e-mail]]

Comment on attachment 144073 [details] [diff] [review]
(Av1) <pref-fonts.js>

I think I prefer the if (dataEls.length) break; logic, it's more readable. Also
is it possible to merge the two if (dataEls.length) conditions? Perhaps not.
And I still don't like it when you mess around with the indentation. Also,
seeing as you're changing it, I prefer to replace /^\s+|\s+$/ with "" instead
of that strip whitespace code.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b) <pref-fonts.js>

Av1, with comment 3 suggestion(s),
except the following one which I did not understand :-(

(In reply to comment #3)
> (From update of attachment 144073 [details] [diff] [review])
> seeing as you're changing it, I prefer to replace /^\s+|\s+$/ with "" instead

> of that strip whitespace code.

Comment 5

[timeless]

the code does:
var stripWhitespace = /^\s*(.*)\s*$/;
foo.replace(stripWhitespace, "$1"));

you could do:
foo.replace(/^\s+|\s+$/,"")

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1c) <pref-fonts.js>

Av1b, with comment 5 suggestion(s).

Comment 7

[[e-mail]]

Comment on attachment 145047 [details] [diff] [review]
(Av1c) <pref-fonts.js>

>+     if (dataEls && dataEls.length)
>+       selectedItem = dataEls[0];
bz, dataEls is the return from listElement.getElementsByAttribute so there
aren't exactly a large number of elements to scan, but is there a preferred way
to do this or will this suffice in this case?

Comment 8

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #7)
> >+     if (dataEls && dataEls.length)
> >+       selectedItem = dataEls[0];

if (dataEls && dataEls.item(0))
  selectedItem = dataEls[0];

is what I'd recommend.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av1d) <pref-fonts.js> [Checked in: See comment 11]

Av1c, with comment 8 suggestion(s).

Comment 10

[[e-mail]]

Fix checked in.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 146099 [details] [diff] [review]
(Av1d) <pref-fonts.js>
[Checked in: See comment 11]


(In reply to comment #10)
> Fix checked in.

(for the record)
A modified version was checked in.

Comment 12

[[e-mail]]

(In reply to comment #11)
>A modified version was checked in.
Merge conflicts, don't you just love them ;-)

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 146099 [details] [diff] [review]
(Av1d) <pref-fonts.js>
[Checked in: See comment 11]

'approval1.7=?':
Trivial U.I. code cleanup, no risk.
(already on Trunk)

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 146099 [details] [diff] [review]
(Av1d) <pref-fonts.js>
[Checked in: See comment 11]

warning cleanups can happen on the trunk. there's not much user value, even if
the risk is low -- and warning cleanups have broken features in the past.