285596 - In <msgHdrViewOverlay.js>, "Warning: variable emailAddress hides argument"

Status: RESOLVED WORKSFORME

(Thunderbird :: Mail Window Front End, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE)

{{
Warning: variable emailAddress hides argument
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 889, Column: 6
Source Code:
  var emailAddress = addressNode.getTextAttribute("emailAddress");
}}

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <msgHdrViewOverlay.js> ++

[Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE)

Fixing by removing the useless parameter;
plus a little nit.

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 177017 [details] [diff] [review]
(Av1) <msgHdrViewOverlay.js> ++


Could you (super-)review/check in this patch ? Thanks.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) <msgHdrViewOverlay.js> ++

Av1, plus fixing another redeclaration from bug 285134.

[Mozilla Thunderbird, version 1.0+ (20050319)] (nightly) (W98SE)

Could you review/check in this patch ? Thanks.

Comment 4

[Paul Tomlin]

This is still evident in 1.5b2, plus some extra ones.
Personally I'd like to see this as major+ targetted at 1.5, but that's just me
hoping a package shipped will not barf on itself. :)

Warning: variable displayName hides argument
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 436, Column: 10
Source Code:
      var displayName = displayName.replace(/ +/g, " ");


Warning: variable emailAddress hides argument
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 960, Column: 6
Source Code:
  var emailAddress = addressNode.getTextAttribute("emailAddress");


Warning: variable folder hides argument
Source File: chrome://messenger/content/mailWindowOverlay.js
Line: 2097, Column: 14
Source Code:
          var folder = GetLoadedMsgFolder();

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)

> Warning: variable displayName hides argument
> Source File: chrome://messenger/content/msgHdrViewOverlay.js
> Line: 436, Column: 10
> Source Code:
>       var displayName = displayName.replace(/ +/g, " ");

This seems to have been introduced by bug 300246, which has restricted access.

> Warning: variable folder hides argument
> Source File: chrome://messenger/content/mailWindowOverlay.js
> Line: 2097, Column: 14
> Source Code:
>           var folder = GetLoadedMsgFolder();

This one is not in msgHdrViewOverlay.js,
Please, move it to the right bug.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b) <msgHdrViewOverlay.js> ++

Av1a, synchonized to current Trunk,
and fixing new comment 4 (1st) warning.

Comment 7

[Christian Weiske]

The three warnings are not that hard to fix; it's trivial.
So could please someone of the devs fix it - I hate having the javascript console full of errors or warnings - this disturbs a lot when programming own extensions.

Comment 8

[Serge Gautherie (:sgautherie)]

[Mozilla Thunderbird, version 2 alpha 1 (20060425)] (nightly) (W98SE)

Bug still there
[[
Warning: variable displayName hides argument
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 399, Column: 10
Source Code:
      var displayName = displayName.replace(/ +/g, " ");

Warning: variable emailAddress hides argument
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 809, Column: 6
Source Code:
  var emailAddress = addressNode.getTextAttribute("emailAddress");
]]

Comment 9

[Ronny Perinke]

addressNode.getTextAttribute("emailAddress") <-- this is set in updateEmailAddressNode()
using
emailAddressNode.setTextAttribute("emailAddress", address.emailAddress);

AddExtraAddressProcessing() is called with address.emailAddress as value for the emailAddress parameter.

So why do we need to retrieve the value again with addressNode.getTextAttribute("emailAddress")?

AddExtraAddressProcessing() without
var emailAddress = addressNode.getTextAttribute("emailAddress");
should be sufficient.

Comment 10

[Ronny Perinke]

(In reply to comment #8)
> [Mozilla Thunderbird, version 2 alpha 1 (20060425)] (nightly) (W98SE)
> 
> Bug still there
> [[
> Warning: variable displayName hides argument
> Source File: chrome://messenger/content/msgHdrViewOverlay.js
> Line: 399, Column: 10
> Source Code:
>       var displayName = displayName.replace(/ +/g, " ");
> 
a simple renaming should work


var displayName_striped = displayName.replace(/ +/g, " ");          

currentAttachments.push (new createNewAttachmentInfo(contentType, url, displayName_striped, uri, isExternalAttachment));

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)
> > Source Code:
> >       var displayName = displayName.replace(/ +/g, " ");
> >
> a simple renaming should work

No need to have two "variables", as the initial value is not needed after this line.

Anyway, this part was fixed in bug 283729, where a patch was prepared 1,5 months before mine and checked in 2 weeks after :-/

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <msgHdrViewOverlay.js> ++

Av1b, synchonized to current Trunk,
with comment 10 suggestion(s), which reverts/improves part of bug 283729 fix,
with more space nits.

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> Anyway, this part was fixed in bug 283729, where a patch was prepared 1,5
> months before mine and checked in 2 weeks after :-/

To be explicit, my first patch was submitted here 6 months before the one in the other bug:
I am very disappointed again to see my patchs unreviewed for months/years and other people spend time redoing what I already did :-//

Comment 14

[[e-mail]]

Comment on attachment 223104 [details] [diff] [review]
(Av2) <msgHdrViewOverlay.js> ++

>-    addressNode.setAttribute("tooltiptext", mailAddress);
>     addressNode.setAttribute("tooltip", "emailAddressTooltip");
This duplication is bogus; I don't want my name associated with it.

>-  var menuseparator = document.createElement('menuseparator');
>-  openpopup.appendChild(menuseparator);
>+  openpopup.appendChild(document.createElement("menuseparator"));
r=me on the whitespace and menuseparator changes.

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) <msgHdrViewOverlay.js> ++

Av2, with comment 14 suggestion(s),
with more code and space nits fixes.

*Removal of |addressNode.setAttribute("tooltiptext", emailAddress);|: that's how I understood your previous comment, but I'm not 100% sure...
*Removal of |addressNode.removeAttribute("tooltiptext");| as it's already done in the calling function(s).
*|fillInEmailAddressTooltip()| seemed to have been a bad [which should generate an error.!?.] copy and paste of |FillInAttachmentTooltip()| :-(
*Removal of try+catch, as there's none at the 2 other calling sites...

Comment 16

[[e-mail]]

Comment on attachment 223182 [details] [diff] [review]
(Av3) <msgHdrViewOverlay.js> ++

Please don't ask me to review mail/base/content/msgHdrViewOverlay.js again.

(In reply to comment #15)
>*Removal of |addressNode.setAttribute("tooltiptext", emailAddress);|: that's
>how I understood your previous comment, but I'm not 100% sure...
Neither was I, and I really didn't want to waste time on this, but the lowdown is that you should use tooltiptext if you know what it is (as we do here), and tooltip if you don't, and I'm not sure what happens if you set both.

>*Removal of |addressNode.removeAttribute("tooltiptext");| as it's already done
>in the calling function(s).
I'm guessing here that this is supposed to be there after all, see below.

>*|fillInEmailAddressTooltip()| seemed to have been a bad [which should generate
>an error.!?.] copy and paste of |FillInAttachmentTooltip()| :-(
Yes, and so badly copied and pasted that I'm guessing that it's never used and the entire tooltip should be removed and tooltiptext used instead.

>*Removal of try+catch, as there's none at the 2 other calling sites...
No idea whether this needs to be there.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Av2a-MailNews) <msgHdrViewOverlay.js> ++

Av2, MailNews part only,
with one more space nit fix.

Keeping
{{
 ------- Comment #14 From [e-mail]  2006-05-24 04:26 PDT  [reply] -------

r=me on the whitespace and menuseparator changes.
}}

Comment 18

[Paul Tomlin]

I believe this has been fixed via bug 283729 @ rev 1.62

var displayName ... -> displayName 
http://lxr.mozilla.org/seamonkey/source/mail/base/content/msgHdrViewOverlay.js#439

var emailAddress ... -> var mailAddress
http://lxr.mozilla.org/seamonkey/source/mail/base/content/msgHdrViewOverlay.js#935


http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/mail/base/content&command=DIFF_FRAMESET&file=msgHdrViewOverlay.js&rev2=1.62&rev1=1.61


However, and this gets confusing, I'm still seeing both warnings from the above code in TB version 2 alpha 1 (20060724). 1.62 was committed 2005-11-09 13:40.

Does this just need copying to a branch?

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Serge, any update on this bug?

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)
> I believe this has been fixed via bug 283729 @ rev 1.62

See comment 11 !

> code in TB version 2 alpha 1 (20060724). 1.62 was committed 2005-11-09 13:40.
> Does this just need copying to a branch?

Sure, TB v2.0a1 is on the 1.8(.1) branch.
NB: A later bug 283729 patch ported the fix(es) to it.

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> Serge, any update on this bug?

Let me see
...
I have other (more recent) patches touching these files;
I'll get back to (what's left of) this one, after them.

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #17)
> Created an attachment (id=223192) [details]
> (Av2a-MailNews) <msgHdrViewOverlay.js> ++
> 
> Av2, MailNews part only,
> with one more space nit fix.
> 
> Keeping
> {{
>  ------- Comment #14 From [e-mail]  2006-05-24 04:26 PDT  [reply]
> -------
> 
> r=me on the whitespace and menuseparator changes.
> }}

Serge, this patch seems slightly rotted, the mailnews/base/resources/content/mailWidgets.xml file might have moved. Perhaps it should be updated?

(also note the many-year-old neil sr request)

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-SM) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync'

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-TB) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync' [Checkin: See comment 29]

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 385668 [details] [diff] [review]
(Cv1-TB) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync'
[Checkin: See comment 29]

>-            for (var i=0; i<targetIds.length;i++) {
>+            var j = 0;
>+            for (var i = 0; i < targetIds.length; ++i) {

Tend to prefer i++ myself, and you could make it  |let i| while you're at it (here and the other place).

Comment 26

[Ronny Perinke]

(In reply to comment #25)
> (From update of attachment 385668 [details] [diff] [review])
> >-            for (var i=0; i<targetIds.length;i++) {
> >+            var j = 0;
> >+            for (var i = 0; i < targetIds.length; ++i) {
> 
> Tend to prefer i++ myself, and you could make it  |let i| while you're at it
> (here and the other place).

... and because j is only used inside the for-loop and the size (length) of targetIds does not change after each iteration, one could also use this approach:

for (let i = 0, j = 0, l = targetIds.length; i < l; i++) {


Where the length of the array only has to be calculated once, which saves one statement on each iteration.

Comment 27

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1a-TB) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync'

Cv1-TB, with comment 25 and comment 26 suggestion(s).

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 385898 [details] [diff] [review]
(Cv1a-TB) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync'

>+            for (let i = 0, j = 0, l = targetIds.length; i < l; i++) {

Splitting the l out here is pointless - we don't cross the xpcom boundaries so it's exactly (more less) as fast both ways.

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 385668 [details] [diff] [review]
(Cv1-TB) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync'
[Checkin: See comment 29]


http://hg.mozilla.org/comm-central/rev/e2f87bd1beee
(Cv1b-TB) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync'

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a-SM) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync' [Checkin: See comment 38]

Comment 31

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1-TB) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix [Checkin: Comment 37]

NB: The long diff block at the start is whitespace only.

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 388059 [details] [diff] [review]
(Dv1-TB) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix
[Checkin: Comment 37]

r=mkmelin

Comment 33

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1-SM-Bw) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix

"diff -Bw", to ease review.

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1-SM) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix [Checkin: Comment 39]

See Ev1-SM-Bw.

Comment 35

[Ian Neal]

Comment on attachment 388174 [details] [diff] [review]
(Ev1-SM) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix
[Checkin: Comment 39]

There are some places where you are changing a line due to a whitespace but you could have changed the quotes from single to double. If you feel like spinning a new version with those changes too, fine, but either way r=me

Comment 36

[Serge Gautherie (:sgautherie)]

(In reply to comment #35)
> If you feel like spinning
> a new version with those changes too, fine, but either way r=me

Ah, yes, I probably could (and would) have done some more changes,
but I wanted to focus on synchronization (only) ftb :-|

Comment 37

[Serge Gautherie (:sgautherie)]

Comment on attachment 388059 [details] [diff] [review]
(Dv1-TB) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix
[Checkin: Comment 37]


http://hg.mozilla.org/comm-central/rev/e14746a5a158

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 386269 [details] [diff] [review]
(Bv1a-SM) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync'
[Checkin: See comment 38]


http://hg.mozilla.org/comm-central/rev/fd6ac539335e

after fixing context for
{
patching file suite/mailnews/mailWidgets.xml
Hunk #4 FAILED at 757
Hunk #5 FAILED at 931
Hunk #6 FAILED at 1424
3 out of 7 hunks FAILED
}

Comment 39

[Serge Gautherie (:sgautherie)]

Comment on attachment 388174 [details] [diff] [review]
(Ev1-SM) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix
[Checkin: Comment 39]


http://hg.mozilla.org/comm-central/rev/08bde0c3a277

Comment 40

[Magnus Melin [:mkmelin]]

Nothing more to do here.