597887 - mochitest-plain-1: "Error: useless setTimeout call (missing quotes around argument?)" at "file_bug546995.html : 28"

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Serge Gautherie (:sgautherie)]

Noticed in Error Console, when run on my local Win2K:
{
Error: useless setTimeout call (missing quotes around argument?)
Source File: http://mochi.test:8888/tests/content/html/content/test/file_bug546995.html
Line: 28
}

Code is
{
28   setTimeout(parent.gGen.next(), 0);
}
It probably should not have parenthesis. (Or is setTimeout() actually useless?)

Comment 1

[Mounir Lamouri (:mounir)]

Maybe SimpletTest.executeSoon would be more appropriate.

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> Maybe SimpletTest.executeSoon would be more appropriate.

I usually prefer it. Yet this "bug" should be the same with either function.

Comment 3

[Justin Wood (:Callek)]

Attachment: v1

Use executeSoon and fix

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 478991 [details] [diff] [review]
v1

Does this really work? Looks to me like it'll execute the 'next' function but with the global object as 'this'. You have to do something like:

SimpleTest.executeSoon(parent.gGen.next.bind(parent.gGen.next))

Comment 5

[Justin Wood (:Callek)]

(In reply to comment #4)
> Comment on attachment 478991 [details] [diff] [review]
> v1
> 
> Does this really work? Looks to me like it'll execute the 'next' function but
> with the global object as 'this'. You have to do something like:
> 
> SimpleTest.executeSoon(parent.gGen.next.bind(parent.gGen.next))

I thought executeSoon did that magic for us... to make sure the right scopes are used. Is that not so? and/or should I take your r- as "use this code I provided and if you do r+"?

Comment 6

[Justin Wood (:Callek)]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 478991 [details] [diff] [review] [details]
> > v1
> > 
> > Does this really work? Looks to me like it'll execute the 'next' function but
> > with the global object as 'this'. You have to do something like:
> > 
> > SimpleTest.executeSoon(parent.gGen.next.bind(parent.gGen.next))
> 
> I thought executeSoon did that magic for us... to make sure the right scopes
> are used. Is that not so? and/or should I take your r- as "use this code I
> provided and if you do r+"?

yea, I'm not sure you are right sicking, but I'm a bit too tired to triple check the JS Syntax differences and if/how they matter in this case: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#512

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't see that executeSoon could do the magic we want since it's only given a reference to Iterator.next, from which there is no way to track back to the actual parent.gGen object.

That said, my r- means "this looks fishy, please double-check that it does what you want it to".

In fact, I'm surprised that the test is passing both before and after this patch? Shouldn't it be failing if the next() function isn't called when it's supposed to?

Comment 8

[Justin Wood (:Callek)]

Attachment: v2

Turns out this bind stuff is actually needed, and an OMG I forgot to test this notice when I get errors on SimpleTest not being defined in scope.

Comment 9

[Justin Wood (:Callek)]

sicking, ping? [only because you told me over IRC you would get to it within 24 hours (so on the first)]

Comment 10

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/a88c80b1b471

Comment 11

[Serge Gautherie (:sgautherie)]

V.Fixed, per
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1286468200.1286471496.20397.gz
Linux comm-central-trunk debug test mochitests-1/5 on 2010/10/07 09:16:40