Convert tests to use the non-deprecated WebContentsObserver navigation methods.

Convert tests to use the non-deprecated WebContentsObserver navigation methods.

Some unit tests use a harness that is not very realistic, so it doesn't send the new callbacks when PlzNavigate isn't enabled. In those cases I've added both methods and I'm using each of them depending on the mode. This makes it trivial to delete the old code path when switching PlzNavigate on and deleting the old methods, instead of having to update the tests then.

BUG=682002
Review-Url: https://codereview.chromium.org/2640473003
Cr-Commit-Position: refs/heads/master@{#445115}
Committed: https://chromium.googlesource.com/chromium/src/+/f174850ffb24a21a76cd786551203c26a47b97d4

[jam, 2017-01-17 22:41:10]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 22:41:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-17 23:48:10]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 23:48:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-18 00:02:22]

Description was changed from

==========
Convert tests to use the non-deprecated WebContentsObserver navigation methods.

Some unit tests use a harness that is not very realistic, so it doesn't send the
new callbacks when PlzNavigate isn't enabled. In those cases I've added both
methods and I'm using each of them depending on the mode. This makes it trivial
to delete the old code path when switching PlzNavigate on and deleting the old
methods, instead of having to update the tests then.

BUG=682002
==========

to

==========
Convert tests to use the non-deprecated WebContentsObserver navigation methods.

Some unit tests use a harness that is not very realistic, so it doesn't send the
new callbacks when PlzNavigate isn't enabled. In those cases I've added both
methods and I'm using each of them depending on the mode. This makes it trivial
to delete the old code path when switching PlzNavigate on and deleting the old
methods, instead of having to update the tests then.

BUG=682002
==========

[jam, 2017-01-18 00:02:23]

jam@chromium.org changed reviewers:
+ clamy@chromium.org

[commit-bot: I haz the power, 2017-01-18 00:30:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 00:30:08]

Dry run: This issue passed the CQ dry run.

[clamy, 2017-01-18 14:33:34]

Thanks! A few questions below.

https://codereview.chromium.org/2640473003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2640473003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:290: if
(!content::IsBrowserSideNavigationEnabled())
Does it fail without this if PlzNavigate isn't enabled? If so, is it because of
timing (DidStartNavigation happens later in non-PlzNavigate case) or because
DidStartNavigation is also fired for renderer initiated navs?

https://codereview.chromium.org/2640473003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/2640473003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:161: void
DidStartNavigation(NavigationHandle* navigation_handle) override {
Should we exclude same page navigation? They wouldn't have triggered
DidStartProvisionalLoadForFrame before.

https://codereview.chromium.org/2640473003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/2640473003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:181: void
DidStartNavigation(NavigationHandle* navigation_handle) override {
Note: the tests here could maybe use the TestNavigationManager?

[jam, 2017-01-18 16:35:21]

https://codereview.chromium.org/2640473003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2640473003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:290: if
(!content::IsBrowserSideNavigationEnabled())
On 2017/01/18 14:33:34, clamy wrote:
> Does it fail without this if PlzNavigate isn't enabled? If so, is it because
of
> timing (DidStartNavigation happens later in non-PlzNavigate case) or because
> DidStartNavigation is also fired for renderer initiated navs?

Yep, it fails because both methods are called

https://codereview.chromium.org/2640473003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/2640473003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:161: void
DidStartNavigation(NavigationHandle* navigation_handle) override {
On 2017/01/18 14:33:34, clamy wrote:
> Should we exclude same page navigation? They wouldn't have triggered
> DidStartProvisionalLoadForFrame before.

this is only used by a single test which doesn't need this extra check, so i
left it as simple as could be to make the test pass

https://codereview.chromium.org/2640473003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/2640473003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:181: void
DidStartNavigation(NavigationHandle* navigation_handle) override {
On 2017/01/18 14:33:34, clamy wrote:
> Note: the tests here could maybe use the TestNavigationManager?

Done.

[jam, 2017-01-18 16:35:33]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 16:35:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 17:12:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 17:12:46]

Dry run: This issue passed the CQ dry run.

[clamy, 2017-01-19 14:59:37]

https://codereview.chromium.org/2640473003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2640473003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:290: if
(!content::IsBrowserSideNavigationEnabled())
On 2017/01/18 16:35:21, jam wrote:
> On 2017/01/18 14:33:34, clamy wrote:
> > Does it fail without this if PlzNavigate isn't enabled? If so, is it because
> of
> > timing (DidStartNavigation happens later in non-PlzNavigate case) or because
> > DidStartNavigation is also fired for renderer initiated navs?
> 
> Yep, it fails because both methods are called

Acknowledged.

https://codereview.chromium.org/2640473003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/2640473003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:161: void
DidStartNavigation(NavigationHandle* navigation_handle) override {
On 2017/01/18 16:35:21, jam wrote:
> On 2017/01/18 14:33:34, clamy wrote:
> > Should we exclude same page navigation? They wouldn't have triggered
> > DidStartProvisionalLoadForFrame before.
> 
> this is only used by a single test which doesn't need this extra check, so i
> left it as simple as could be to make the test pass

Acknowledged.

https://codereview.chromium.org/2640473003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/2640473003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:827:
nav_watcher.WaitForNavigationFinished();
This doesn't correspond to the same point in time in navigation.
WaitForNavigationFinished waits until the navigation commits or gets canceled.

[jam, 2017-01-20 05:52:16]

https://codereview.chromium.org/2640473003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/2640473003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:827:
nav_watcher.WaitForNavigationFinished();
On 2017/01/19 14:59:37, clamy wrote:
> This doesn't correspond to the same point in time in navigation.
> WaitForNavigationFinished waits until the navigation commits or gets canceled.

I originally tried  WaitForRequestStart() to keep same behavior as before, but
that didn't work since the testnavigationmanager didn't get it and only got
finished, so it returned false in WaitForDesiredState.

This doesn't actually matter though, since this is a test and if we wait a
little bit longer than before, that's fine.

[clamy, 2017-01-20 17:43:50]

Ok lgtm.

[jam, 2017-01-20 18:16:30]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-01-20 18:16:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 19:01:45]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484936190859990,
"parent_rev": "866204443431a62c1b6e1404ed3c19c3f529ce45", "commit_rev":
"f174850ffb24a21a76cd786551203c26a47b97d4"}

[commit-bot: I haz the power, 2017-01-20 19:02:17]

Description was changed from

==========
Convert tests to use the non-deprecated WebContentsObserver navigation methods.

Some unit tests use a harness that is not very realistic, so it doesn't send the
new callbacks when PlzNavigate isn't enabled. In those cases I've added both
methods and I'm using each of them depending on the mode. This makes it trivial
to delete the old code path when switching PlzNavigate on and deleting the old
methods, instead of having to update the tests then.

BUG=682002
==========

to

==========
Convert tests to use the non-deprecated WebContentsObserver navigation methods.

Some unit tests use a harness that is not very realistic, so it doesn't send the
new callbacks when PlzNavigate isn't enabled. In those cases I've added both
methods and I'm using each of them depending on the mode. This makes it trivial
to delete the old code path when switching PlzNavigate on and deleting the old
methods, instead of having to update the tests then.

BUG=682002

Review-Url: https://codereview.chromium.org/2640473003
Cr-Commit-Position: refs/heads/master@{#445115}
Committed:
https://chromium.googlesource.com/chromium/src/+/f174850ffb24a21a76cd78655120...
==========

[commit-bot: I haz the power, 2017-01-20 19:02:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f174850ffb24a21a76cd78655120...