Open Bug 285596 Opened 15 years ago Updated 11 years ago

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

Categories

(Thunderbird :: Mail Window Front End, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: sgautherie, Assigned: sgautherie)

References

Details

Attachments

(4 files, 9 obsolete files)

6.73 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
6.76 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
13.04 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
32.43 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
[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");
}}
Attached patch (Av1) <msgHdrViewOverlay.js> ++ (obsolete) — Splinter Review
[Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE)

Fixing by removing the useless parameter;
plus a little nit.
Assignee: mscott → gautheri
Status: NEW → ASSIGNED
Attachment #177017 - Flags: superreview?(mscott)
Attachment #177017 - Flags: review?(mscott)
Comment on attachment 177017 [details] [diff] [review]
(Av1) <msgHdrViewOverlay.js> ++


Could you (super-)review/check in this patch ? Thanks.
Attachment #177017 - Flags: superreview?(mscott)
Attachment #177017 - Flags: review?(mscott)
Depends on: 285134
Attached patch (Av1a) <msgHdrViewOverlay.js> ++ (obsolete) — Splinter Review
Av1, plus fixing another redeclaration from bug 285134.

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

Could you review/check in this patch ? Thanks.
Attachment #177017 - Attachment is obsolete: true
Attachment #177970 - Flags: review?(mscott)
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();
(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.
Depends on: sa15907
Attached patch (Av1b) <msgHdrViewOverlay.js> ++ (obsolete) — Splinter Review
Av1a, synchonized to current Trunk,
and fixing new comment 4 (1st) warning.
Attachment #177970 - Attachment is obsolete: true
Attachment #200431 - Flags: superreview?(mscott)
Attachment #200431 - Flags: review?(mscott)
Attachment #177970 - Flags: review?(mscott)
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.
[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");
]]
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.
(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));
(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 :-/
Depends on: 283729
Attached patch (Av2) <msgHdrViewOverlay.js> ++ (obsolete) — Splinter Review
Av1b, synchonized to current Trunk,
with comment 10 suggestion(s), which reverts/improves part of bug 283729 fix,
with more space nits.
Attachment #200431 - Attachment is obsolete: true
Attachment #223104 - Flags: review?
Attachment #200431 - Flags: superreview?(mscott)
Attachment #200431 - Flags: review?(mscott)
Attachment #223104 - Flags: review? → review?(neil)
(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 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.
Attachment #223104 - Flags: review?(neil)
Attached patch (Av3) <msgHdrViewOverlay.js> ++ (obsolete) — Splinter Review
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...
Attachment #223104 - Attachment is obsolete: true
Attachment #223182 - Flags: review?(neil)
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.
Attachment #223182 - Flags: review?(neil)
Attachment #223182 - Attachment is obsolete: true
Av2, MailNews part only,
with one more space nit fix.

Keeping
{{
 ------- Comment #14 From neil@parkwaycc.co.uk  2006-05-24 04:26 PDT  [reply] -------

r=me on the whitespace and menuseparator changes.
}}
Attachment #223192 - Flags: superreview?(neil)
Attachment #223192 - Flags: review+
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?
QA Contact: front-end
Serge, any update on this bug?
(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.
(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.
Depends on: 309057
(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 neil@parkwaycc.co.uk  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 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).
Attachment #385668 - Flags: review?(mkmelin+mozilla) → review+
(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.
Cv1-TB, with comment 25 and comment 26 suggestion(s).
Attachment #385668 - Attachment is obsolete: true
Attachment #385898 - Flags: review?(mkmelin+mozilla)
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.
Attachment #385898 - Flags: review?(mkmelin+mozilla) → review-
Attachment #385898 - Attachment is obsolete: true
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'
Attachment #385668 - Attachment description: (Cv1-TB) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync' → (Cv1-TB) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync' [Checkin: See comment 29]
Attachment #385668 - Attachment is obsolete: false
Attachment #385667 - Attachment is obsolete: true
Attachment #386269 - Flags: review?(iann_bugzilla)
Attachment #385667 - Flags: review?(iann_bugzilla)
Attachment #386269 - Flags: review?(iann_bugzilla) → review+
Keywords: checkin-needed
Whiteboard: [c-n: Bv1a-SM after SM2.0b1]
NB: The long diff block at the start is whitespace only.
Attachment #388059 - Flags: review?(mkmelin+mozilla)
Comment on attachment 388059 [details] [diff] [review]
(Dv1-TB) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix
[Checkin: Comment 37]

r=mkmelin
Attachment #388059 - Flags: review?(mkmelin+mozilla) → review+
Whiteboard: [c-n: Bv1a-SM after SM2.0b1] → [c-n: Bv1a-SM, Dv1-TB, after SM2.0b1/TBv3.0b3]
Attachment #223192 - Attachment is obsolete: true
Attachment #223192 - Flags: superreview?(neil)
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
Attachment #388174 - Flags: review?(iann_bugzilla) → review+
Attachment #388173 - Attachment is obsolete: true
(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 :-|
Whiteboard: [c-n: Bv1a-SM, Dv1-TB, after SM2.0b1/TBv3.0b3] → [c-n: Bv1a-SM, Dv1-TB, Ev1-SM, after SM2.0b1/TBv3.0b3]
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
Attachment #388059 - Attachment description: (Dv1-TB) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix → (Dv1-TB) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix [Checkin: Comment 37]
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
}
Attachment #386269 - Attachment description: (Bv1a-SM) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync' → (Bv1a-SM) mailWidgets.xml: tabs cleanup, whitespaces sync' and minor code sync' [Checkin: See comment 38]
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
Attachment #388174 - Attachment description: (Ev1-SM) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix → (Ev1-SM) msgHdrViewOverlay.js: whitespaces sync' and minor code sync'/fix [Checkin: Comment 39]
Keywords: checkin-needed
Whiteboard: [c-n: Bv1a-SM, Dv1-TB, Ev1-SM, after SM2.0b1/TBv3.0b3]
You need to log in before you can comment on or make changes to this bug.