Change TestFrameNavigationObserver to wait for load stop instead of commit.

Change TestFrameNavigationObserver default wait to be for load stop instead of commit.

BUG=418236
Committed: https://crrev.com/8206fa1942048cd3a92b893ae1d005b279926aef
Cr-Commit-Position: refs/heads/master@{#382477}

[nasko, 2016-03-21 16:50:55]

Patchset #3 (id:40001) has been deleted

[nasko, 2016-03-21 17:36:11]

Description was changed from

==========
Change TestFrameNavigationObserver to wait for load stop instead of commit.

BUG=
==========

to

==========
Change TestFrameNavigationObserver default wait to be for load stop instead of
commit.

BUG=
==========

[nasko, 2016-03-21 17:38:18]

nasko@chromium.org changed reviewers:
+ alexmos@chromium.org

[nasko, 2016-03-21 17:38:19]

Hey Alex,
Can you review this patch for me? It changes the default behavior of
TestFrameNavigationObserver::Wait to wait for load stop instead of commit, since
the existing behavior was leading to subtle failures in tests.
I had to still provide WaitForCommit, as there is one test explicitly depending
on commit time.
Thanks in advance!
Nasko

[alexmos, 2016-03-21 23:00:23]

alexmos@chromium.org changed reviewers:
+ creis@chromium.org

[alexmos, 2016-03-21 23:00:24]

LGTM.  Adding Charlie, as he mentioned he had a test in mind that should wait
for commit and wanted to check that with this CL.

One thought is maybe we should file a bug for cleaning up NavigateFrameToURL,
and then reference it in the description?  I see lots of other cleanup that will
be possible after this lands, and we can then use the bug to track that cleanup
as well.  E.g., some tests use WaitForLoadStop to wait for full load after
NavigateFrameToURL, such as:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

Others wrap it with TestNavigationObserver or even TestFrameNavigationObserver:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/si...

And there are also several that use WaitForRenderFrameReady.  E.g.:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/si...

I think all of these waits are now no-ops and can just go away now, but no need
to clutter this CL with that.  I'd be especially happy to go and remove the
WaitForRenderFrameReady ones, since I added some of them. :)  

Also, another wish for later is to be able to verify that the URL was loaded
properly with EXPECT_TRUE(NavigateFrameToURL(...)), just like for regular
NavigateToURL.

https://codereview.chromium.org/1816733002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1816733002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:5505: frame_observer.Wait();
nit: This doesn't look like it's needed anymore (or even previously).  Mind
removing it?  (Also ok to remove it later, along with all the other unnecessary
waiting logic around NavigateFrameToURL.)

[nasko, 2016-03-21 23:26:44]

Description was changed from

==========
Change TestFrameNavigationObserver default wait to be for load stop instead of
commit.

BUG=
==========

to

==========
Change TestFrameNavigationObserver default wait to be for load stop instead of
commit.

BUG=418236
==========

[nasko, 2016-03-21 23:42:54]

I've associated this CL with https://crbug.com/418236, which is all about
improving test code for --site-per-process. It fits the bill very well and I'd
suggest we use it for tracking all the improvements you proposed.

https://codereview.chromium.org/1816733002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1816733002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:5505: frame_observer.Wait();
On 2016/03/21 23:00:24, alexmos wrote:
> nit: This doesn't look like it's needed anymore (or even previously).  Mind
> removing it?  (Also ok to remove it later, along with all the other
unnecessary
> waiting logic around NavigateFrameToURL.)

Done.

[Charlie Reis, 2016-03-21 23:55:50]

On 2016/03/21 23:00:24, alexmos wrote:
> LGTM.  Adding Charlie, as he mentioned he had a test in mind that should wait
> for commit and wanted to check that with this CL.

Thanks.  No need to wait for me-- I realized that test hasn't landed yet, so
I'll include the new version before I put it up for review.

[nasko, 2016-03-22 00:56:44]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-03-22 00:56:45]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/1816733002/#ps80001
(title: "Remove unneeded observer and Wait call.")

[commit-bot: I haz the power, 2016-03-22 00:57:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1816733002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1816733002/80001

[commit-bot: I haz the power, 2016-03-22 02:24:21]

Description was changed from

==========
Change TestFrameNavigationObserver default wait to be for load stop instead of
commit.

BUG=418236
==========

to

==========
Change TestFrameNavigationObserver default wait to be for load stop instead of
commit.

BUG=418236
==========

[commit-bot: I haz the power, 2016-03-22 02:24:22]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-03-22 02:25:50]

Description was changed from

==========
Change TestFrameNavigationObserver default wait to be for load stop instead of
commit.

BUG=418236
==========

to

==========
Change TestFrameNavigationObserver default wait to be for load stop instead of
commit.

BUG=418236

Committed: https://crrev.com/8206fa1942048cd3a92b893ae1d005b279926aef
Cr-Commit-Position: refs/heads/master@{#382477}
==========

[commit-bot: I haz the power, 2016-03-22 02:25:51]

Patchset 4 (id:??) landed as
https://crrev.com/8206fa1942048cd3a92b893ae1d005b279926aef
Cr-Commit-Position: refs/heads/master@{#382477}