Fix mis-classification of navigations when there is a pending page.

Fix mis-classification of navigations when there is a pending page.

BUG=369661
TEST=NavigationControllerBrowserTest.NavigationTypeClassification_On1InPageTo☐While☐Pending

Committed: https://crrev.com/da96ccb1f10231bcb6ea2fa2491f9629b7c3c995
Cr-Commit-Position: refs/heads/master@{#328000}

[Avi (use Gerrit), 2015-05-01 19:07:20]

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

[Avi (use Gerrit), 2015-05-01 19:07:20]

One last review before you head off.

[Charlie Reis, 2015-05-01 19:40:48]

These are fun to think through.  :)   I think I caught a bug; do you agree?

https://codereview.chromium.org/1114403002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1114403002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1036: if (params.url ==
pending_entry_->GetURL() ||
I think we should be comparing params.url to existing_entry->GetURL(), right?

Consider the case that the replaceState URL matches the stalled (pending) url2. 
We want to consider the replaceState as in-page, not same-page, because we're
comparing against the last committed entry and not the pending one.

https://codereview.chromium.org/1114403002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1114403002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1088:
DoReplaceStateWhilePending(shell(), url, url);
Nice.

Can we add a test that does a replaceState to url2, to catch the bug I
mentioned?

[Avi (use Gerrit), 2015-05-01 20:30:57]

https://codereview.chromium.org/1114403002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1114403002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1036: if (params.url ==
pending_entry_->GetURL() ||
On 2015/05/01 19:40:48, Charlie Reis (OOO until 5-11) wrote:
> I think we should be comparing params.url to existing_entry->GetURL(), right?
> 
> Consider the case that the replaceState URL matches the stalled (pending)
url2. 
> We want to consider the replaceState as in-page, not same-page, because we're
> comparing against the last committed entry and not the pending one.

Yeah, that sounds right. Let me update the patch and also think about it a lot.

https://codereview.chromium.org/1114403002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1114403002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1088:
DoReplaceStateWhilePending(shell(), url, url);
On 2015/05/01 19:40:48, Charlie Reis (OOO until 5-11) wrote:
> Nice.
> 
> Can we add a test that does a replaceState to url2, to catch the bug I
> mentioned?

Done.

[Charlie Reis, 2015-05-01 21:35:08]

LGTM!

[Avi (use Gerrit), 2015-05-01 21:36:45]

The CQ bit was checked by avi@chromium.org

[Charlie Reis, 2015-05-01 21:37:10]

https://codereview.chromium.org/1114403002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1114403002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1036:
!params.was_within_same_page &&
And just for posterity, this looks correct to me because the (confusingly named
in this case) |was_within_same_page| is true for things like replaceState (which
get classified as IN_PAGE) and false for the "hit enter to reload" case (which
gets classified as SAME_PAGE).

[commit-bot: I haz the power, 2015-05-01 21:38:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1114403002/60001

[Avi (use Gerrit), 2015-05-01 21:41:55]

https://codereview.chromium.org/1114403002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1114403002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1036:
!params.was_within_same_page &&
On 2015/05/01 21:37:09, Charlie Reis (OOO until 5-11) wrote:
> And just for posterity, this looks correct to me because the (confusingly
named
> in this case) |was_within_same_page| is true for things like replaceState
(which
> get classified as IN_PAGE) and false for the "hit enter to reload" case (which
> gets classified as SAME_PAGE).

This line is what allows NavigationTypeClassification_On1InPageTo1While1Pending
to pass.

[commit-bot: I haz the power, 2015-05-01 22:47:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-01 22:48:00]

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