571517 - [SeaMonkey] Don't pass strings to setTimeout

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

See Thunderbird Bug 431978 for example.

I'm going to do this in several steps e.g. mailnews, common, etc since my tiny little mind can't encompass all of it at one go.

Comment 1

[Philip Chee]

Attachment: Patch vM1.0 first cut.

> addrbook/abCardOverlay.js
> 
>   if ( focus ) {
>     // XXX Using the setTimeout hack until bug 103197 is fixed
>     setTimeout( function(firstTextBox) { firstTextBox.focus(); }, 0, focus );
>   }

Not sure why there is an inline function here so I didn't touch it.

>  function InitCharsetMenuCheckMark()
>  {
>    // Check the menu
>    UpdateMailEditCharset();
>    // use setTimeout workaround to delay checkmark the menu
>    // when onmenucomplete is ready then use it instead of oncreate
>    // see bug #78290 for the details
> -  setTimeout("UpdateMailEditCharset()", 0);
> +  setTimeout(UpdateMailEditCharset, 50);

For some reason I had to increase the timeout to 50 before this started working.

> -            setTimeout(function() { box.ensureRowIsVisible(index); }, 0);
> +            setTimeout(box.ensureRowIsVisible, 0, index);

Is this correct?

Comment 2

[[e-mail]]

(In reply to comment #1)
>>     setTimeout( function(firstTextBox) { firstTextBox.focus(); }, 0, focus );
>Not sure why there is an inline function here so I didn't touch it.
Because setTimeout is a property of the window, and always calls the function as if it was a global. We want to call the focus method of the firstTextBox as a function of the textbox, not of the window.

>>    // Check the menu
>>    UpdateMailEditCharset();
>>    // use setTimeout workaround to delay checkmark the menu
>>    // when onmenucomplete is ready then use it instead of oncreate
>>    // see bug #78290 for the details
>> -  setTimeout("UpdateMailEditCharset()", 0);
>> +  setTimeout(UpdateMailEditCharset, 50);
> For some reason I had to increase the timeout to 50 before this started working.
Odd, because it seems to work for me, I see the menu radio marker first time.

> > -            setTimeout(function() { box.ensureRowIsVisible(index); }, 0);
> > +            setTimeout(box.ensureRowIsVisible, 0, index);
> Is this correct?
No, see above; this does box.ensureRowIsVisible.call(window, index)

Comment 3

[[e-mail]]

Comment on attachment 450706 [details] [diff] [review]
Patch vM1.0 first cut.

>-    var theNewRow = awGetListItem(top.awRow);
>+  var theNewRow = awGetListItem(top.awRow);
diff -w next time?

>                 gCollectAddress = header.headerValue;
Don't need this any more, just pass header.headerValue directly.

>+                gCollectAddressTimer = setTimeout(abAddressCollector.collectAddress,
This is also wrong for the same reason; you'll need a helper function.

>+    var args = window.arguments;
Any reason for this?

Comment 4

[Philip Chee]

>>+    var args = window.arguments;
> Any reason for this?

The crazy line wrapping was irritating me. This shortens the repeated window.arguments.foobar enough I can unwrap the subsequent lines.

Comment 5

[Philip Chee]

This also roughly matches what Thunderbird does except that they use arg0, etc.

Comment 6

[Philip Chee]

Attachment: Patch vM1.1-w diff-w

> (From update of attachment 450706 [details] [diff] [review])
> diff -w next time?

Done.

>>                 gCollectAddress = header.headerValue;
> Don't need this any more, just pass header.headerValue directly.

Fixed.

>>+                gCollectAddressTimer = setTimeout(abAddressCollector.collectAddress,
> This is also wrong for the same reason; you'll need a helper function.

I hope I've got it right this time, the diff is a bit messy.

>>+    var args = window.arguments;
> Any reason for this?

The crazy line wrapping was irritating me. This shortens the repeated window.arguments.foobar enough I can unwrap the subsequent lines. This also roughly matches what Thunderbird does except that they use arg0, etc.

Comment 7

[[e-mail]]

Comment on attachment 450983 [details] [diff] [review]
Patch vM1.1-w diff-w

>-var abAddressCollector = null;
Ideally this would eventually get turned into a lazy service getter, so I don't think it's worthwhile making this and related changes.

>+                gCollectAddressTimer = setTimeout(function(aAddresses, aCreateCard, aSendFormat)
>+                                       {
>+                                         abAddressCollector.collectAddress(aAddresses, aCreateCard, aSendFormat);
>+                                       },
>+                                       2000, header.headerValue, createCard,
>+                                       Components.interfaces.nsIAbPreferMailFormat.unknown);
This is badly formatted but no matter how I tried to reformat it it was still ugly. You might be better off using a separate top level function instead.

Comment 8

[Philip Chee]

Attachment: [checked-in] Patch vM1.2 r=IanN sr=Neil

Carrying forward sr=Neil

>  [e-mail]      2010-06-14 05:56:10 PDT

> (From update of attachment 450983 [details] [diff] [review])
> >-var abAddressCollector = null;
> Ideally this would eventually get turned into a lazy service getter, so I don't
> think it's worthwhile making this and related changes.

Reverted.

> >+                gCollectAddressTimer = setTimeout(function(aAddresses, aCreateCard, aSendFormat)
> >+                                       {
> >+                                         abAddressCollector.collectAddress(aAddresses, aCreateCard, aSendFormat);
> >+                                       },
> >+                                       2000, header.headerValue, createCard,
> >+                                       Components.interfaces.nsIAbPreferMailFormat.unknown);
> This is badly formatted but no matter how I tried to reformat it it was still
> ugly. You might be better off using a separate top level function instead.

Fixed.

Comment 9

[Philip Chee]

Comment on attachment 451536 [details] [diff] [review]
[checked-in] Patch vM1.2 r=IanN sr=Neil

Actually set/carry forward sr=Neil.

Pushed to Comm-Central as:
http://hg.mozilla.org/comm-central/rev/cbaa2cb21cf3

Comment 10

[Philip Chee]

Attachment: Patch 2.0 Rest of suite/

> -      setTimeout("gURLBar.focus();", 0);
> +      gURLBar.focus();
>      else
> -      setTimeout("content.focus();", 0);
> +      content.focus();

Firefox Bug 247606 changed the strings to inline functions. But then Firefox Bug 410075 removed the timeouts completely. Gavin and Ehsan did some CVS archaelogy back to 2001 and concluded that Hyatt was smoking something when he added the timeout. The trail went something like Bug 102598 -> Bug 91884 -> Bug 91788.

> -    //debug ("observer: assert");

Perhaps I shouldn't have removed this.

> -  setTimeout("persist_width()",100);
> +  PersistWidth();

PersistWidth() already calls a setTimeout()

> -        setTimeout(function() {window.close();}, 5000);
> +        setTimeout(window.close, 5000);

Untested but this is what the Firefox version does.

I fixed all js errors caused by my patch except this one:

Error: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIRDFService.GetResource]
Source file: chrome://communicator/content/sidebar/sidebarOverlay.js
Line: 1269

Eventually I realized that this error occurs even without my patch. The whole sidebar code is a total mess if you ask me.

Comment 11

[[e-mail]]

Comment on attachment 452958 [details] [diff] [review]
Patch 2.0 Rest of suite/

>-  // This fuction is a redo of the fix for jag bug 91884
>+  // This fuction is a redo of the fix for jag bug 91884.
Nit: might as well fix the spelling of "function" too ;-)

>-      setTimeout("gURLBar.focus();", 0);
>+      gURLBar.focus();
This change didn't work for me (opening a new blank tab results in no caret). Maybe you should use WindowFocusTimerCallback.

>-        setTimeout('fixup_children("'+ treeitem.getAttribute('id') +'")', 100);
>+        setTimeout(fixup_children, 100, treeitem.getAttribute("id"));
Nit: local style is to use 's

>+function Persist(aAttribute, aValue) document.persist(aAttribute, aValue);
Nit: prefer proper top-level functions. (The inline style is reasonable when working with small anonymous functions.)

>+        setTimeout(window.close, 5000);
Yeah, I think this works because setTimeout calls the method on the window. In fact setTimeout(close, 5000) probably works.

Comment 12

[Philip Chee]

Attachment: Patch v2.1 Rest of Suite/ Fixed nits.

> (From update of attachment 452958 [details] [diff] [review])
>>-  // This fuction is a redo of the fix for jag bug 91884
>>+  // This fuction is a redo of the fix for jag bug 91884.
> Nit: might as well fix the spelling of "function" too ;-)
Fixed.

>>-      setTimeout("gURLBar.focus();", 0);
>>+      gURLBar.focus();
> This change didn't work for me (opening a new blank tab results in no caret).
> Maybe you should use WindowFocusTimerCallback.
Switched to Using WindowFocusTimerCallback.

>>-        setTimeout('fixup_children("'+ treeitem.getAttribute('id') +'")', 100);
>>+        setTimeout(fixup_children, 100, treeitem.getAttribute("id"));
> Nit: local style is to use 's
Fixed.

>>+function Persist(aAttribute, aValue) document.persist(aAttribute, aValue);
> Nit: prefer proper top-level functions. (The inline style is reasonable when
> working with small anonymous functions.)
Fixed.

>>+        setTimeout(window.close, 5000);
> Yeah, I think this works because setTimeout calls the method on the window. In
> fact setTimeout(close, 5000) probably works.
In fact that's what Firefox uses. I was just being redundant.
Fixed.

Comment 13

[[e-mail]]

Comment on attachment 454362 [details] [diff] [review]
Patch v2.1 Rest of Suite/ Fixed nits.

>   onAssert : function(ds,src,prop,target) {
>-    //debug ("observer: assert");
Missed this last time sorry. r=me with this restored.

Comment 14

[Philip Chee]

Attachment: [checked-in] Patch v2.1 Rest of Suite/ r=Neil

Carrying forward r=Neil

>>   onAssert : function(ds,src,prop,target) {
>>-    //debug ("observer: assert");
> Missed this last time sorry. r=me with this restored.
Fixed.

Comment 15

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/b016d4eabd86

Comment 16

[Philip Chee]

Attachment: Patch vE3.0 /editor and /mailnews/

This takes care of /editor and one remaining string in /mailnews. Once this is in the only remaining consumers are in /calendar.

Comment 17

[[e-mail]]

Comment on attachment 455655 [details] [diff] [review]
Patch vE3.0 /editor and /mailnews/

>-    setTimeout("gDialog.SelectionList.focus()", 0);
>+    setTimeout(gDialog.SelectionList.focus, 0);
This won't work.

Comment 18

[Philip Chee]

Attachment: [checked-in] Patch vE3.1 /editor and /mailnews/

> [e-mail]      2010-07-02 03:58:14 PDT
> 
> (From update of attachment 455655 [details] [diff] [review])
>>-    setTimeout("gDialog.SelectionList.focus()", 0);
>>+    setTimeout(gDialog.SelectionList.focus, 0);
> This won't work.

From #seamonkey
> [11:07:40] <NeilAway> RattyAway: actually the dialog code automatically focuses the right element anyway
> [11:08:18] <RattyAway> NeilAway: so should I use an inline function then?
> [11:08:31] <NeilAway> RattyAway: just delete those 5 lines
Fixed.

Comment 19

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/0b57c7793c9a