787794 - fix some JS strict mode warnings that appear at Thunderbird startup

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[:aceman]

There are many warnings at TB startup (in debug mode) like this:

Warning: SyntaxError: in strict mode code, functions may be declared only at top level or immediately within another function
Source file: chrome://messenger/content/mail3PaneWindowCommands.js
Line: 511, Column: 17
Source code:
        function canCompact(folder) { 

or

Warning: ReferenceError: reference to undefined property node.id
Source file: chrome://messenger/content/folderWidgets.xml
Line: 23

I try to fix some of them, to ease spotting new warnings by developers when creating new code. I fix only those produced by comm-central code and only those that I understand how to fix :)

Comment 1

[:aceman]

Attachment: patch

This fixes the warnings I have seen.

I could not fix these as they are too involved:
Warning: ReferenceError: reference to undefined property aTabType.panelId
Source file: chrome://messenger/content/tabmail.xml
Line: 352

and 

Warning: Use of Mutation Events is deprecated. Use MutationObserver instead.
Source file: chrome://messenger/content/msgMail3PaneWindow.js
Line: 945

If anybody hints me how to rework the code there I can add it.

Also if anybody sees some other fixable warnings at startup, just post them here.

Comment 2

[Philip Chee]

> Warning: Use of Mutation Events is deprecated. Use MutationObserver instead.
> Source file: chrome://messenger/content/msgMail3PaneWindow.js
> Line: 945
See Attachment 655455 [details] [diff] in SeaMonkey Bug 785745 (Stop using DOMAttrModified events) for an example

Comment 3

[:aceman]

Thanks, but I still do not understand how that observing works, so I'll not do it in this patch.

Comment 4

[Nomis101]

(In reply to :aceman from comment #1)
> Also if anybody sees some other fixable warnings at startup, just post them
> here.

I see this warning five times in my error console:

Timestamp: 08.09.12 10:42:42
Warning: XUL box for _moz_generated_content_before element contained an inline #text child, forcing all its children to be wrapped in a block.
Source File: chrome://global/content/bindings/toolbar.xml
Line: 276

Comment 5

[Jim Porter (:squib)]

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

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

Just a quick drive-by because I can't sleep. :)

::: mail/base/content/folderPane.js
@@ -846,5 @@
>  
>        // Note that these children may have been open when we were last closed,
>        // and if they are, we also have to add those grandchildren to the map
>        let oldCount = this._rowMap.length;
> -      function recursivelyAddToMap(aChild, aNewIndex, tree) {

To minimize the size of the diff and to eliminate the chance that we don't mess up any variable closures here, I think it would be better to simply change this line to:

  let recursivelyAddToMap = function recursivelyAddToMap(aChild, aNewIndex, tree);

Do please keep the function name after the equals sign like so, since otherwise the function will show up as having no name during debugging.

Comment 6

[Jim Porter (:squib)]

The above, of course, applies to other similar locations.

Comment 7

[Philip Chee]

> I see this warning five times in my error console:
> 
> Timestamp: 08.09.12 10:42:42
> Warning: XUL box for _moz_generated_content_before element contained an inline #text child, forcing all its children to be wrapped in a block.
> Source File: chrome://global/content/bindings/toolbar.xml
> Line: 276

Possibly caused by some extensions with gnarly custom toolbar buttons. Do you see this in Thunderbird safe mode?

Comment 8

[Nomis101]

(In reply to Philip Chee from comment #7)
> > I see this warning five times in my error console:
> > 
> > Timestamp: 08.09.12 10:42:42
> > Warning: XUL box for _moz_generated_content_before element contained an inline #text child, forcing all its children to be wrapped in a block.
> > Source File: chrome://global/content/bindings/toolbar.xml
> > Line: 276
> 
> Possibly caused by some extensions with gnarly custom toolbar buttons. Do
> you see this in Thunderbird safe mode?
I don't have any extensions installed. And yes, I also see this in safe mode. The first time I've seen this in the console was around the time TB on Mac switched to the monochrom toolbar buttons. I found some bugs which have seen this in Firefox as well (636367, 715293).

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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

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

r=me with the changes below applied. Thanks!

::: mail/base/content/folderPane.js
@@ -846,5 @@
>  
>        // Note that these children may have been open when we were last closed,
>        // and if they are, we also have to add those grandchildren to the map
>        let oldCount = this._rowMap.length;
> -      function recursivelyAddToMap(aChild, aNewIndex, tree) {

Sure, that sounds fine to me.

::: mailnews/base/content/jsTreeView.js
@@ +124,5 @@
>     * Opens or closes a container with children.  The logic here is a bit hairy, so
>     * be very careful about changing anything.
>     */
>    toggleOpenState: function jstv_toggleOpenState(aIndex) {
> +    function recursivelyAddToMap(aChild, aNewIndex, tree) {

I think squib's comment before can apply here as well.

Comment 10

[:aceman]

(In reply to Nomis101 from comment #4)
> I see this warning five times in my error console:
> 
> Timestamp: 08.09.12 10:42:42
> Warning: XUL box for _moz_generated_content_before element contained an
> inline #text child, forcing all its children to be wrapped in a block.
> Source File: chrome://global/content/bindings/toolbar.xml
> Line: 276

Thanks for coming in, but this warning seems emitted from Gecko core and I said I will not touch that in this bug.

Comment 11

[:aceman]

Attachment: patch v2

Thanks guys.
There are some /mailnews parts so I'll try Neil too.
I move the whole accountCompare function in mailnews/base/content/folderWidgets.xml, but I have already adapted patch for bug 749200 to that so please leave it so.

Comment 12

[[e-mail]]

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

I haven't looked too closely at any of the /mail/ files.

>         while (node) {
>-          if (/wrapper-.*/.test(node.id)) {
>+          if (("id" in node) && /wrapper-.*/.test(node.id)) {
I'd prefer to use while (node instanceof XULElement) as that's the interface that supplies the id property.

>+          function accountCompare(a, b) {
Weren't you changing this code? In which case, why move the old version?

>-      function recursivelyAddToMap(aChild, aNewIndex, tree) {
>+      let recursivelyAddToMap = function recursivelyAddToMap(aChild, aNewIndex, tree) {
I wonder whether this should be a single-invocation function expression i.e.

(function recursivelyAddToMap(aChild, aNewIndex, tree) {
  ...
})(this._rowMap[aIndex], aIndex, this);

Comment 13

[:aceman]

(In reply to [e-mail] from comment #12)
> Comment on attachment 659828 [details] [diff] [review]
> patch v2
> 
> I haven't looked too closely at any of the /mail/ files.
That's fine, I wanted you to look at /mailnews per comment 11.

> 
> >         while (node) {
> >-          if (/wrapper-.*/.test(node.id)) {
> >+          if (("id" in node) && /wrapper-.*/.test(node.id)) {
> I'd prefer to use while (node instanceof XULElement) as that's the interface
> that supplies the id property.
OK, I will try that.

> 
> >+          function accountCompare(a, b) {
> Weren't you changing this code? In which case, why move the old version?

I tried to change it but that bug has stalled so far. So this is working on the old code as it is so far.

> >-      function recursivelyAddToMap(aChild, aNewIndex, tree) {
> >+      let recursivelyAddToMap = function recursivelyAddToMap(aChild, aNewIndex, tree) {
> I wonder whether this should be a single-invocation function expression i.e.
> 
> (function recursivelyAddToMap(aChild, aNewIndex, tree) {
>   ...
> })(this._rowMap[aIndex], aIndex, this);

I do not know such JS plays so can't answer that:)

Comment 14

[:aceman]

Attachment: patch v3

Comment 15

[[e-mail]]

(In reply to aceman from comment #13)
> (In reply to comment #12)
> > (From update of attachment 659828 [details] [diff] [review])
> > >+          function accountCompare(a, b) {
> > Weren't you changing this code? In which case, why move the old version?
> I tried to change it but that bug has stalled so far. So this is working on
> the old code as it is so far.
Sure, but won't bitrotting yourself slow that bug down even further?

> > >-      function recursivelyAddToMap(aChild, aNewIndex, tree) {
> > >+      let recursivelyAddToMap = function recursivelyAddToMap(aChild, aNewIndex, tree) {
> > I wonder whether this should be a single-invocation function expression i.e.
> > 
> > (function recursivelyAddToMap(aChild, aNewIndex, tree) {
> >   ...
> > })(this._rowMap[aIndex], aIndex, this);
> I do not know such JS plays so can't answer that:)
I was hoping squib or maybe mconley might comment...

Comment 16

[:aceman]

(In reply to [e-mail] from comment #15)
> (In reply to aceman from comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 659828 [details] [diff] [review])
> > > >+          function accountCompare(a, b) {
> > > Weren't you changing this code? In which case, why move the old version?
> > I tried to change it but that bug has stalled so far. So this is working on
> > the old code as it is so far.
> Sure, but won't bitrotting yourself slow that bug down even further?
I have said in comment 11 that I have already adapted (not yet attached) the patch in bug 749200 to the changes in this patch. The whole function is removed there so it's position is not very relevant. What is slowing me down is waiting on standard8's review :)

> > > >-      function recursivelyAddToMap(aChild, aNewIndex, tree) {
> > > >+      let recursivelyAddToMap = function recursivelyAddToMap(aChild, aNewIndex, tree) {
> > > I wonder whether this should be a single-invocation function expression i.e.
> > > 
> > > (function recursivelyAddToMap(aChild, aNewIndex, tree) {
> > >   ...
> > > })(this._rowMap[aIndex], aIndex, this);
> > I do not know such JS plays so can't answer that:)
> I was hoping squib or maybe mconley might comment...
So let's ask them.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to :aceman from comment #16)
> (In reply to [e-mail] from comment #15)
> > (In reply to aceman from comment #13)
> > > (In reply to comment #12)
> > > > (From update of attachment 659828 [details] [diff] [review])
> > > > >+          function accountCompare(a, b) {
> > > > Weren't you changing this code? In which case, why move the old version?
> > > I tried to change it but that bug has stalled so far. So this is working on
> > > the old code as it is so far.
> > Sure, but won't bitrotting yourself slow that bug down even further?
> I have said in comment 11 that I have already adapted (not yet attached) the
> patch in bug 749200 to the changes in this patch. The whole function is
> removed there so it's position is not very relevant. What is slowing me down
> is waiting on standard8's review :)
> 
> > > > >-      function recursivelyAddToMap(aChild, aNewIndex, tree) {
> > > > >+      let recursivelyAddToMap = function recursivelyAddToMap(aChild, aNewIndex, tree) {
> > > > I wonder whether this should be a single-invocation function expression i.e.
> > > > 
> > > > (function recursivelyAddToMap(aChild, aNewIndex, tree) {
> > > >   ...
> > > > })(this._rowMap[aIndex], aIndex, this);
> > > I do not know such JS plays so can't answer that:)
> > I was hoping squib or maybe mconley might comment...
> So let's ask them.

It's neat, but I don't think it adds much. In fact, I think it detracts a little from readability. So, in my opinion, the original correction is acceptable.

Comment 18

[:aceman]

I agree with mconley. If I stumbled upon code like that I would not know what it actually does and would skip it in sweeping bugs like these even if it contained something to fix.

Comment 19

[:aceman]

Neil?

Comment 20

[[e-mail]]

Comment on attachment 660518 [details] [diff] [review]
patch v3

>-      function recursivelyAddToMap(aChild, aNewIndex, tree) {
So, I stumbled across bug 682096. That bug switched from using a scoped variable (let tree = this;) to using an additional parameter. At this point, the function doesn't use its scope, so it could be made global.

Alternatively, there is another way to pass "this" to the function, and that is to put it on the prototype, where you can call it using this._recursivelyAddToMap(child, index);

But I quite understand you not wanting to use a function expression.

Comment 21

[[e-mail]]

Comment on attachment 660518 [details] [diff] [review]
patch v3

>-      function checkUnsent() {
>+      let checkUnsent = function checkUnsent() {
>         if (MailOfflineMgr.shouldSendUnsentMessages())
>           SendUnsentMessages();
>       }
>       setTimeout(checkUnsent, 0);
On the other hand, this function is small enough that I would still suggest using a function expression, although obviously I'm not a peer of this code so you are quite within your rights to completely ignore me.
setTimeout(function checkUnsent() {
  if (MailOfflineMgr.shouldSendUnsentMessages())
    SendUnsentMessages();
}, 0);

Comment 22

[:aceman]

I think that is OK, it is understandable.

Comment 23

[:aceman]

Attachment: patch v4

Do you mean it like this?

Comment 24

[[e-mail]]

Comment on attachment 676794 [details] [diff] [review]
patch v4

Thanks, this looks good. Sorry for the delayed review though.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/comm-central/rev/7b25c50ce213