731889 - variable event redeclares argument: let event = document.createEvent("Events");

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[ISHIKAWA, Chiaki]

While testing the latest comm-central version (I pulled it a couple of days ago),
I noticed this following entry in debug console:

Warning: variable event redeclares argument
Source File: chrome://messenger/content/mailWidgets.xml
Line: 399, Column: 12
Source Code:
        let event = document.createEvent("Events"); 

The following is the snippet I got from mailWidgets.xml:

    <handlers>
      <handler event="click" button="0" clickcount="2"><![CDATA[
        let event = document.createEvent("Events");
        event.initEvent("command", true, true);
        this.dispatchEvent(event);
      ]]></handler>
    </handlers>

I think the warning message should be suppressed by
renaming the let variable one way or the other.

We have enough warning messages to clutter up debug console and
any change that can decrease the clutter will be appreciated before next release.

I have no idea where this code comes from at the moment, though.

TIA

Comment 1

[:aceman]

I could try this but I do not know how to test it if it breaks any function. What were you doing when it appeared? I have not seen this error.

Comment 2

[ISHIKAWA, Chiaki]

After starting TB,
I simply visited my folder and tried to save an attachment.
It is hard to tell when this message appears, but
it appeared after 
 - the saving of an attachment by clicking on the attachment itself 
 - (and overwrite an existing file with the same name),
 -  and then after it was written, I checked the error console
    and found the error 

I just repeated the process again.

This is with the local build of TB from comm-central.
The version string is 13.0a1 and under 32 bit linux.
(Sorry I was reporting from a Windows 7 and that platform was
registered as the platform where the problem was observed.)
  
I am trying to change the platform information with this post.



The message was not in the error console before I tried saving the attachment.

I don't think if it breaks any function because it is a warning IMHO. But still seeing this type of warning 25 times by the time above warning appears in the error console is not very encouraging. I wonder how many of the developers check the
error console content.

Admittedly, many are undefined properties related to web page access (from TB, a mailer :-(, but
if the web pages are from www.mozilla.org, it is not very encouraging.

I counted the warnings:
Reference to undefined property : seven (7) times 
unknown property ... Declaration dropped : fifteen (15) times
Then this warning appeared.

(Sorry about the Japanese time stamp message)
Timestamp: 2012年03月01日 23時28分22秒
Warning: variable event redeclares argument
Source File: chrome://messenger/content/mailWidgets.xml
Line: 399, Column: 12
Source Code:
        let event = document.createEvent("Events"); 

I think we developers should pay more attention to these warnings (also to the warnings during compilation.)

TIA

Comment 3

[:aceman]

Thanks, I'll try those STR.

I agree with you, there are too many warnings in the error console (they are probably only shown when you make a debug build).

I wonder if some of those "Reference to undefined property" aren't real bugs.

Comment 4

[Jim Porter (:squib)]

This happens when you double-click an attachment.

Comment 5

[:aceman]

Attachment: patch

I tested that also after renaming the 'event' variable, another one still exists (the one passed automatically).
I also change 2 other places with the same pattern (new variable 'event' defined) just for safety.
Other places seem to really want to use the automatic variable so I do not touch them.

Comment 6

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #5)
> Created attachment 602071 [details] [diff] [review]
> patch

Thank you for looking into this.

I didn't realize that 
>> I wonder if some of those "Reference to undefined property" aren't real bugs.
(as in comment 3.
and you recently reported Bug 732144

Had I known, I would have reported similar problems long time ago :-(
Yes I am using local Debug build to fix a certain bug which plagued me for a long time :-(
The number of warnings I see error console go up and own all the time as releases come and go, but there are way too many to my taste.

Comment 7

[:aceman]

Yes, there are too many of them.
I don't know if the developers know about them or not. But I think all of them must be running debug builds. They probably do not care much. But it is not easy to decide how to fix each warning. It needs somebody who knows the code module.

Bug 732144 shows that some of them are real bugs (a feature was broken) but not much harmful.

Comment 8

[Jim Porter (:squib)]

Comment on attachment 602071 [details] [diff] [review]
patch

Review of attachment 602071 [details] [diff] [review]:
-----------------------------------------------------------------

I have a couple of comments below. r=me with those changes.

::: mail/base/content/mailWidgets.xml
@@ +288,5 @@
>          if (this.currentItem) {
>            this.addItemToSelection(this.currentItem);
> +          let newEvent = document.createEvent("Events");
> +          newEvent.initEvent("command", true, true);
> +          this.currentItem.dispatchEvent(newEvent);

While we're here, could we replace this with something like the following?

  let evt = document.createEvent("XULCommandEvent");
  evt.initCommandEvent("command", true, true, window, 0, event.ctrlKey,
                       event.altKey, event.shiftKey, event.metaKey, null);

That would be more correct usage than the generic "Events" category. Here's the IDL file, which explains the arguments: <http://mxr.mozilla.org/comm-central/source/mozilla/dom/interfaces/xul/nsIDOMXULCommandEvent.idl#68>.

@@ +397,5 @@
>      <handlers>
>        <handler event="click" button="0" clickcount="2"><![CDATA[
> +        let newEvent = document.createEvent("Events");
> +        newEvent.initEvent("command", true, true);
> +        this.dispatchEvent(newEvent);

Likewise here.

Comment 9

[:aceman]

Attachment: patch v2

No idea what it does, but as you wish.

Comment 10

[Jim Porter (:squib)]

Comment on attachment 602605 [details] [diff] [review]
patch v2

Review of attachment 602605 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me by inspection. Could you fix the indentation on the last block, though?

Comment 11

[:aceman]

Attachment: patch v3

Thanks, fixed. Carrying over r+.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/95938cc17483