Fix webkit_unit_tests to use the threaded parser and enable everywhere.

Fix webkit_unit_tests to use the threaded parser and enable everywhere.

In order to support the threaded parser, FrameTestHelpers utilities has
been updated to magically wait for loads to finish. There are also
several new helpers for other common operations in tests that
understand how to wait for the threaded parser to complete.

Tests that need to interleave more complicated operations can fall back
to pumpPendingRequestsDoNotUse(). Most tests should prefer the new
helper utilities though.

BUG=366354
R=abarth@chromium.org, eseidel@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173731

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173771

[dcheng, 2014-05-08 22:13:54]

I've gone through and audited existing usages of:
- FrameTestHelpers::loadFrame to make sure test semantics didn't change (the
only test I found this to affect was ReloadWhileProvisional).
- FrameTestHelpers::runPendingTasks, removing calls where unnecessary.
- serveAsynchronousMockedRequests(), removing calls where unnecessary.

[abarth-chromium, 2014-05-09 03:00:52]

LGTM

Glad to see such ugly code go!

https://codereview.chromium.org/271793007/diff/230001/Source/web/tests/FrameT...
File Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/271793007/diff/230001/Source/web/tests/FrameT...
Source/web/tests/FrameTestHelpers.cpp:91: explicit
ServeAsyncRequestsTask(TestWebFrameClient* client) : m_client(client)
: m_client(client) goes on its own line.

https://codereview.chromium.org/271793007/diff/230001/Source/web/tests/FrameT...
Source/web/tests/FrameTestHelpers.cpp:153: ReloadTask(WebFrame* frame, bool
ignoreCache) : m_frame(frame), m_ignoreCache(ignoreCache)
This should be three lines.

[eseidel, 2014-05-09 03:08:35]

lgtm

Thank you so much for taking this on.

[dcheng, 2014-05-09 06:00:00]

I restructured some of the helpers a bit (specifically, I made
pumpPendingRequests enter the run loop internally to hopefully simplify it for
callers) and fixed the style comments.

https://codereview.chromium.org/271793007/diff/230001/Source/web/tests/FrameT...
File Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/271793007/diff/230001/Source/web/tests/FrameT...
Source/web/tests/FrameTestHelpers.cpp:91: explicit
ServeAsyncRequestsTask(TestWebFrameClient* client) : m_client(client)
On 2014/05/09 03:00:53, abarth wrote:
> : m_client(client) goes on its own line.

Done.

https://codereview.chromium.org/271793007/diff/230001/Source/web/tests/FrameT...
Source/web/tests/FrameTestHelpers.cpp:153: ReloadTask(WebFrame* frame, bool
ignoreCache) : m_frame(frame), m_ignoreCache(ignoreCache)
On 2014/05/09 03:00:53, abarth wrote:
> This should be three lines.

Done.

[dcheng, 2014-05-09 06:49:12]

Committed patchset #17 manually as r173731 (presubmit successful).

[falken, 2014-05-09 08:36:27]

A revert of this CL has been created in
https://codereview.chromium.org/272143002/ by falken@chromium.org.

The reason for reverting is: Sorry to revert this patch. The assert it adds is
getting hit in webkit_unit_tests on WebKit Mac 10.6 (dbg).

Log:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20%28db...

Snippet from log:
ASSERTION FAILED: !testClientForFrame(m_webView->mainFrame())->isLoading()
../../third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp(271) : void
blink::FrameTestHelpers::WebViewHelper::reset().

[falken, 2014-05-09 13:18:13]

On 2014/05/09 08:36:27, falken wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/272143002/ by mailto:falken@chromium.org.
> 
> The reason for reverting is: Sorry to revert this patch. The assert it adds is
> getting hit in webkit_unit_tests on WebKit Mac 10.6 (dbg).
> 
> Log:
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20%28db...
> 
> Snippet from log:
> ASSERTION FAILED: !testClientForFrame(m_webView->mainFrame())->isLoading()
> ../../third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp(271) : void
> blink::FrameTestHelpers::WebViewHelper::reset().

The assert was also hit on Android Dbg:
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...

[dcheng, 2014-05-09 19:23:41]

Committed patchset #19 manually as r173771 (presubmit successful).

[Nico, 2015-08-29 22:07:48]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-08-29 22:07:49]

https://codereview.chromium.org/271793007/diff/350001/Source/web/tests/FrameT...
File Source/web/tests/FrameTestHelpers.h (right):

https://codereview.chromium.org/271793007/diff/350001/Source/web/tests/FrameT...
Source/web/tests/FrameTestHelpers.h:64: // this. Use one of the above helpers.
I found this comment pretty confusing today. The method name says "do not use".
The comment here also says "Don't use this". Yet when I clicked through to this
review, the commit message says

"""Tests that need to interleave more complicated operations can fall back
to pumpPendingRequestsDoNotUse(). Most tests should prefer the new
helper utilities though."""

which is a lot weaker than "do not use". The reviewer on
https://codereview.chromium.org/364213004 found this confusing too. Can the
comment here to be improved to weaken the "do not use", and maybe the comment
could also explain when it's ok to use this and why it's usually not a good idea
to use this?

[abarth-chromium, 2015-08-29 22:44:06]

On 2015/08/29 at 22:07:49, thakis wrote:
>
https://codereview.chromium.org/271793007/diff/350001/Source/web/tests/FrameT...
> File Source/web/tests/FrameTestHelpers.h (right):
> 
>
https://codereview.chromium.org/271793007/diff/350001/Source/web/tests/FrameT...
> Source/web/tests/FrameTestHelpers.h:64: // this. Use one of the above helpers.
> I found this comment pretty confusing today. The method name says "do not
use". The comment here also says "Don't use this". Yet when I clicked through to
this review, the commit message says
> 
> """Tests that need to interleave more complicated operations can fall back
> to pumpPendingRequestsDoNotUse(). Most tests should prefer the new
> helper utilities though."""
> 
> which is a lot weaker than "do not use". The reviewer on
https://codereview.chromium.org/364213004 found this confusing too. Can the
comment here to be improved to weaken the "do not use", and maybe the comment
could also explain when it's ok to use this and why it's usually not a good idea
to use this?

Rather than raising this review thread from over a year ago, I'd recommend
creating a new CL with the change you'd like to make and get it reviewed by the
current owners of this code.  Neither of the reviewers of this CL work on this
code anymore and therefore do not have the relevant context to answer your
question.

[Nico, 2015-08-29 22:45:38]

On 2015/08/29 22:44:06, abarth-chromium wrote:
> On 2015/08/29 at 22:07:49, thakis wrote:
> >
>
https://codereview.chromium.org/271793007/diff/350001/Source/web/tests/FrameT...
> > File Source/web/tests/FrameTestHelpers.h (right):
> > 
> >
>
https://codereview.chromium.org/271793007/diff/350001/Source/web/tests/FrameT...
> > Source/web/tests/FrameTestHelpers.h:64: // this. Use one of the above
helpers.
> > I found this comment pretty confusing today. The method name says "do not
> use". The comment here also says "Don't use this". Yet when I clicked through
to
> this review, the commit message says
> > 
> > """Tests that need to interleave more complicated operations can fall back
> > to pumpPendingRequestsDoNotUse(). Most tests should prefer the new
> > helper utilities though."""
> > 
> > which is a lot weaker than "do not use". The reviewer on
> https://codereview.chromium.org/364213004 found this confusing too. Can the
> comment here to be improved to weaken the "do not use", and maybe the comment
> could also explain when it's ok to use this and why it's usually not a good
idea
> to use this?
> 
> Rather than raising this review thread from over a year ago, I'd recommend
> creating a new CL with the change you'd like to make and get it reviewed by
the
> current owners of this code.  Neither of the reviewers of this CL work on this
> code anymore and therefore do not have the relevant context to answer your
> question.

The author is still around though. The approach of commenting on reviews seems
to work pretty well in general, I do this pretty frequently.

[abarth-chromium, 2015-08-29 22:49:29]

On 2015/08/29 at 22:45:38, thakis wrote:
> The author is still around though. The approach of commenting on reviews seems
to work pretty well in general, I do this pretty frequently.

Would you be willing to email the author directly?  I don't want to be CCed on
this thread, but I cannot unsubscribe without messing up this CL.

[Nico, 2015-08-29 23:01:10]

On 2015/08/29 22:49:29, abarth-chromium wrote:
> On 2015/08/29 at 22:45:38, thakis wrote:
> > The author is still around though. The approach of commenting on reviews
seems
> to work pretty well in general, I do this pretty frequently.
> 
> Would you be willing to email the author directly?  I don't want to be CCed on
> this thread, but I cannot unsubscribe without messing up this CL.

No need, they already got my message I sent to this thread :-) I'm not sure if
emailing them directly would un-cc you from this thread here anyway. I would've
expected that no further replies appear here (looks I was wrong), but I can
certainly remove you from the cc list on this review here, I'll do that right
now. (I don't think it'll screw up the review thread, as your LG is clearly
marked in the review history on rietveld.)

[Nico, 2015-08-29 23:01:24]

thakis@chromium.org changed reviewers:
- abarth@chromium.org, thakis@chromium.org

[dcheng, 2015-08-31 16:43:40]

On 2015/08/29 at 23:01:10, thakis wrote:
> On 2015/08/29 22:49:29, abarth-chromium wrote:
> > On 2015/08/29 at 22:45:38, thakis wrote:
> > > The author is still around though. The approach of commenting on reviews
seems
> > to work pretty well in general, I do this pretty frequently.
> > 
> > Would you be willing to email the author directly?  I don't want to be CCed
on
> > this thread, but I cannot unsubscribe without messing up this CL.
> 
> No need, they already got my message I sent to this thread :-) I'm not sure if
emailing them directly would un-cc you from this thread here anyway. I would've
expected that no further replies appear here (looks I was wrong), but I can
certainly remove you from the cc list on this review here, I'll do that right
now. (I don't think it'll screw up the review thread, as your LG is clearly
marked in the review history on rietveld.)

At the time I wrote this, I was hoping we'd be able to get rid of all explicit
calls to pumpPendingRequests(). That doesn't seem likely at this point, so I'll
write up a patch to rename the method and update the comment to describe how
calling this (instead of serveAsynchronousMockedRequests directly) is important
to having non-flaky tests.