Actually process the work queue if layoutTestController.waitUntilDone() is...

Don't use work_queue_ for the isolated world scripts.  Instead, we just
evaluate the script directly.

This change will break the isolatedWorld tests until I land the change to
rename the layoutTestController method upstream.

R=darin
TEST=Coverged by LayoutTests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=22467

[abarth-chromium, 2009-08-03 06:33:58]

Please review this change carefully.  I'm not 100% sure it's correct.

[darin (slow to review), 2009-08-03 06:39:01]

http://codereview.chromium.org/159782/diff/1/2
File webkit/tools/test_shell/layout_test_controller.cc (right):

http://codereview.chromium.org/159782/diff/1/2#newcode463
Line 463: work_queue_.ProcessWorkSoon();
this crap is way more subtle than it should be.  were you able to compare this
to DumpRenderTree to determine if it behaves similarly?  (the wait_until_done
check here appears to be from the initial commit.)

[abarth-chromium, 2009-08-03 06:42:45]

http://codereview.chromium.org/159782/diff/1/2
File webkit/tools/test_shell/layout_test_controller.cc (right):

http://codereview.chromium.org/159782/diff/1/2#newcode463
Line 463: work_queue_.ProcessWorkSoon();
On 2009/08/03 06:39:01, darin wrote:
> this crap is way more subtle than it should be.  were you able to compare this
> to DumpRenderTree to determine if it behaves similarly?  (the wait_until_done
> check here appears to be from the initial commit.)

No, but I'll look now.   I ran into this trying to get the tests in
https://bugs.webkit.org/show_bug.cgi?id=27703 to pass.  Without this change, the
work queue never seems to run.

[abarth-chromium, 2009-08-03 06:50:57]

http://codereview.chromium.org/159782/diff/1/2
File webkit/tools/test_shell/layout_test_controller.cc (right):

http://codereview.chromium.org/159782/diff/1/2#newcode463
Line 463: work_queue_.ProcessWorkSoon();
So, this branch does exist in FrameLoadDelegate::locationChangeDone...  I can
try removing the branch upstream and seeing if all the tests pass.  I don't see
how I can write a test that both waits until done and uses a work queue when
this branch prevents ProcessWork from ever being called!

[abarth-chromium, 2009-08-03 07:00:38]

On 2009/08/03 06:50:57, abarth wrote:
> So, this branch does exist in FrameLoadDelegate::locationChangeDone...  I can
> try removing the branch upstream and seeing if all the tests pass.

Removing this upstream causes a bunch of animation tests to fail.  :(

> I don't see
> how I can write a test that both waits until done and uses a work queue when
> this branch prevents ProcessWork from ever being called!

Any suggestions for how to write these tests?  I need to call setTimeout, which
seems to imply that I need to waitUntilDone, but I need to eval the script for
the isolatedWorld in a clean stack...

[darin (slow to review), 2009-08-03 16:12:57]

On Mon, Aug 3, 2009 at 12:00 AM, <abarth@chromium.org> wrote:

> On 2009/08/03 06:50:57, abarth wrote:
>
>> So, this branch does exist in FrameLoadDelegate::locationChangeDone...
>>
>  I can
>
>> try removing the branch upstream and seeing if all the tests pass.
>>
>
> Removing this upstream causes a bunch of animation tests to fail.  :(
>
>  I don't see
>> how I can write a test that both waits until done and uses a work
>>
> queue when
>
>> this branch prevents ProcessWork from ever being called!
>>
>
> Any suggestions for how to write these tests?  I need to call
> setTimeout, which seems to imply that I need to waitUntilDone, but I
> need to eval the script for the isolatedWorld in a clean stack...
>

Hmm... perhaps we need a call to ProcessWorkSoon in notifyDone?
-Darin



>
>
> http://codereview.chromium.org/159782
>

[abarth-chromium, 2009-08-03 18:54:20]

Ok, can you take another look?  Turns out we don't need to use the work_queue_
at all.  The existing tests all pass if we just evaluate the script directly,
which simplifies life dramatically.

[darin (slow to review), 2009-08-03 19:21:23]

LGTM