Last Comment Bug 571517 - [SeaMonkey] Don't pass strings to setTimeout
: [SeaMonkey] Don't pass strings to setTimeout
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a3
Assigned To: Philip Chee
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-11 09:11 PDT by Philip Chee
Modified: 2010-07-25 19:24 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch vM1.0 first cut. (9.91 KB, patch)
2010-06-11 11:57 PDT, Philip Chee
neil: feedback-
Details | Diff | Splinter Review
Patch vM1.1-w diff-w (10.80 KB, patch)
2010-06-13 21:24 PDT, Philip Chee
neil: superreview+
Details | Diff | Splinter Review
[checked-in] Patch vM1.2 r=IanN sr=Neil (11.20 KB, patch)
2010-06-16 03:17 PDT, Philip Chee
iann_bugzilla: review+
philip.chee: superreview+
Details | Diff | Splinter Review
Patch 2.0 Rest of suite/ (10.87 KB, patch)
2010-06-21 20:47 PDT, Philip Chee
neil: review-
Details | Diff | Splinter Review
Patch v2.1 Rest of Suite/ Fixed nits. (10.94 KB, patch)
2010-06-27 10:43 PDT, Philip Chee
neil: review+
Details | Diff | Splinter Review
[checked-in] Patch v2.1 Rest of Suite/ r=Neil (10.94 KB, patch)
2010-06-28 03:33 PDT, Philip Chee
philip.chee: review+
Details | Diff | Splinter Review
Patch vE3.0 /editor and /mailnews/ (5.13 KB, patch)
2010-07-02 02:57 PDT, Philip Chee
neil: review-
Details | Diff | Splinter Review
[checked-in] Patch vE3.1 /editor and /mailnews/ (5.22 KB, patch)
2010-07-03 07:40 PDT, Philip Chee
neil: review+
Details | Diff | Splinter Review

Description Philip Chee 2010-06-11 09:11:33 PDT
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 2010-06-11 11:57:47 PDT
Created attachment 450706 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-06-11 12:16:58 PDT
(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 neil@parkwaycc.co.uk 2010-06-11 12:22:35 PDT
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 2010-06-11 19:34:02 PDT
>>+    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 2010-06-11 19:46:25 PDT
This also roughly matches what Thunderbird does except that they use arg0, etc.
Comment 6 Philip Chee 2010-06-13 21:24:31 PDT
Created attachment 450983 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-06-14 05:56:10 PDT
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 2010-06-16 03:17:23 PDT
Created attachment 451536 [details] [diff] [review]
[checked-in] Patch vM1.2 r=IanN sr=Neil

Carrying forward sr=Neil

>  neil@parkwaycc.co.uk      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 2010-06-17 01:20:16 PDT
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 2010-06-21 20:47:30 PDT
Created attachment 452958 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-06-22 04:07:01 PDT
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 2010-06-27 10:43:46 PDT
Created attachment 454362 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-06-27 13:42:19 PDT
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 2010-06-28 03:33:28 PDT
Created attachment 454475 [details] [diff] [review]
[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 2010-06-28 03:37:53 PDT
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/b016d4eabd86
Comment 16 Philip Chee 2010-07-02 02:57:09 PDT
Created attachment 455655 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-07-02 03:58:14 PDT
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 2010-07-03 07:40:10 PDT
Created attachment 455861 [details] [diff] [review]
[checked-in] Patch vE3.1 /editor and /mailnews/

> neil@parkwaycc.co.uk      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 2010-07-23 07:49:19 PDT
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/0b57c7793c9a

Note You need to log in before you can comment on or make changes to this bug.