Convert TestFrameNavigationObserver to use the new navigation callbacks.

Convert TestFrameNavigationObserver to use the new navigation callbacks.

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

[jam, 2017-01-26 00:02:58]

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

[commit-bot: I haz the power, 2017-01-26 00:03:44]

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-26 01:32:01]

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

[commit-bot: I haz the power, 2017-01-26 03:19:10]

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

[commit-bot: I haz the power, 2017-01-26 03:19:11]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nasko, 2017-01-26 14:51:21]

https://codereview.chromium.org/2655893004/diff/1/content/public/test/test_fr...
File content/public/test/test_frame_navigation_observer.cc (right):

https://codereview.chromium.org/2655893004/diff/1/content/public/test/test_fr...
content/public/test/test_frame_navigation_observer.cc:67:
navigation_handle->IsErrorPage() ||
I think DidCommitProvisionalLoadForFrame is getting called when we commit the
error page, so that condition might not be necessary. I have to double check to
be sure though.

[jam, 2017-01-26 16:00:00]

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

[commit-bot: I haz the power, 2017-01-26 16:00:19]

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-26 16:04:13]

https://codereview.chromium.org/2655893004/diff/1/content/public/test/test_fr...
File content/public/test/test_frame_navigation_observer.cc (right):

https://codereview.chromium.org/2655893004/diff/1/content/public/test/test_fr...
content/public/test/test_frame_navigation_observer.cc:67:
navigation_handle->IsErrorPage() ||
On 2017/01/26 14:51:20, nasko wrote:
> I think DidCommitProvisionalLoadForFrame is getting called when we commit the
> error page, so that condition might not be necessary. I have to double check
to
> be sure though.

Yeah we get a didfailprovisionalload for the URL and then
didcommitprovisionalload for the error page. I figured all the users of this
class are waiting for successful commits, which we can see from the try run
results. So it's a tradeoff between maintaining the exact behavior as before,
which may just be a side effect of the previous navigation callbacks, vs waiting
only for successful commits (which the comments in the header seem to imply).

i don't feel strongly one way or another, but lean slightly towards the first
patchset. I uploaded the one that waits for success or error pages in patchset
2. do you have a preference?

[nasko, 2017-01-26 17:07:53]

On 2017/01/26 16:04:13, jam wrote:
>
https://codereview.chromium.org/2655893004/diff/1/content/public/test/test_fr...
> File content/public/test/test_frame_navigation_observer.cc (right):
> 
>
https://codereview.chromium.org/2655893004/diff/1/content/public/test/test_fr...
> content/public/test/test_frame_navigation_observer.cc:67:
> navigation_handle->IsErrorPage() ||
> On 2017/01/26 14:51:20, nasko wrote:
> > I think DidCommitProvisionalLoadForFrame is getting called when we commit
the
> > error page, so that condition might not be necessary. I have to double check
> to
> > be sure though.
> 
> Yeah we get a didfailprovisionalload for the URL and then
> didcommitprovisionalload for the error page. I figured all the users of this
> class are waiting for successful commits, which we can see from the try run
> results. So it's a tradeoff between maintaining the exact behavior as before,
> which may just be a side effect of the previous navigation callbacks, vs
waiting
> only for successful commits (which the comments in the header seem to imply).
> 
> i don't feel strongly one way or another, but lean slightly towards the first
> patchset. I uploaded the one that waits for success or error pages in patchset
> 2. do you have a preference?

The accidental behavior point you raise is a good one. Since no tests failed in
PS1, it is probably a good indication that people indeed didn't care about the
error cases. Let's go with PS1.

[commit-bot: I haz the power, 2017-01-26 17:47:20]

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

[commit-bot: I haz the power, 2017-01-26 17:47:21]

Dry run: This issue passed the CQ dry run.

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

On 2017/01/26 17:07:53, nasko wrote:
> On 2017/01/26 16:04:13, jam wrote:
> >
>
https://codereview.chromium.org/2655893004/diff/1/content/public/test/test_fr...
> > File content/public/test/test_frame_navigation_observer.cc (right):
> > 
> >
>
https://codereview.chromium.org/2655893004/diff/1/content/public/test/test_fr...
> > content/public/test/test_frame_navigation_observer.cc:67:
> > navigation_handle->IsErrorPage() ||
> > On 2017/01/26 14:51:20, nasko wrote:
> > > I think DidCommitProvisionalLoadForFrame is getting called when we commit
> the
> > > error page, so that condition might not be necessary. I have to double
check
> > to
> > > be sure though.
> > 
> > Yeah we get a didfailprovisionalload for the URL and then
> > didcommitprovisionalload for the error page. I figured all the users of this
> > class are waiting for successful commits, which we can see from the try run
> > results. So it's a tradeoff between maintaining the exact behavior as
before,
> > which may just be a side effect of the previous navigation callbacks, vs
> waiting
> > only for successful commits (which the comments in the header seem to
imply).
> > 
> > i don't feel strongly one way or another, but lean slightly towards the
first
> > patchset. I uploaded the one that waits for success or error pages in
patchset
> > 2. do you have a preference?
> 
> The accidental behavior point you raise is a good one. Since no tests failed
in
> PS1, it is probably a good indication that people indeed didn't care about the
> error cases. Let's go with PS1.

ok I've deleted patchset 2.

[jam, 2017-01-26 18:35:25]

Patchset #2 (id:20001) has been deleted

[nasko, 2017-01-27 00:58:17]

LGTM

[jam, 2017-01-27 00:58:49]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-01-27 01:00:35]

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-27 02:52:53]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485478729016740, "parent_rev":
"b9459d35906f49fdc039311dcbf9b5d99238c652", "commit_rev":
"a11139d2e593f76401816eabc27631a86e9bb278"}

[commit-bot: I haz the power, 2017-01-27 02:53:20]

Description was changed from

==========
Convert TestFrameNavigationObserver to use the new navigation callbacks.

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

to

==========
Convert TestFrameNavigationObserver to use the new navigation callbacks.

BUG=682002

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

[commit-bot: I haz the power, 2017-01-27 02:53:21]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/a11139d2e593f76401816eabc276...