Only increment the history list offset if we are not replacing the current item.

Only increment the history list offset if we are not replacing the current item.

BUG=453708
TEST=as in bug

Committed: https://crrev.com/26b1681180785dfcedc55cf0574e43f21de8fb10
Cr-Commit-Position: refs/heads/master@{#315060}

[Avi (use Gerrit), 2015-02-06 00:55:05]

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

[Charlie Reis, 2015-02-06 05:03:49]

Wow, did we have that wrong all along?  The fix seems sensible to me-- we
shouldn't change the offset or length in cases like location.replace.

Seems like we should be able observe that in a fairly simple test.  Something
like:
1) Visit pages A1 and A2.
2) Go back to A1, so that A2 is in the forward history.
3) Do a location.replace to A3.
4) Check that history.length is 2, not 1.
(We should not have cleared the forward history or changed the offset.)

That can be checked pretty easily with Javascript code, so perhaps
navigation_controller_impl_browsertest.cc is a good place for it?

[Avi (use Gerrit), 2015-02-06 16:09:59]

On 2015/02/06 05:03:49, Charlie Reis wrote:
> Wow, did we have that wrong all along?  The fix seems sensible to me-- we
> shouldn't change the offset or length in cases like location.replace.
> 
> Seems like we should be able observe that in a fairly simple test.  Something
> like:
> 1) Visit pages A1 and A2.
> 2) Go back to A1, so that A2 is in the forward history.
> 3) Do a location.replace to A3.
> 4) Check that history.length is 2, not 1.
> (We should not have cleared the forward history or changed the offset.)
> 
> That can be checked pretty easily with Javascript code, so perhaps
> navigation_controller_impl_browsertest.cc is a good place for it?

That's not the bug, though. (Your proposed test passes without the fix.)

The bug is when the navigation is initiated by the browser with the
should_replace_current_entry flag set. I need to write a test for that.

[Avi (use Gerrit), 2015-02-06 17:07:22]

Charlie, please take a look.

As a side note, yes, even before my page_id removal patch this test fails. It's
just that before, the offset/length being wrong didn't matter in terms of
everyday navigation. Now it does.

[Charlie Reis, 2015-02-06 17:29:35]

LGTM with one minor question below.

On 2015/02/06 16:09:59, Avi wrote:
> On 2015/02/06 05:03:49, Charlie Reis wrote:
> > Wow, did we have that wrong all along?  The fix seems sensible to me-- we
> > shouldn't change the offset or length in cases like location.replace.
> > 
> > Seems like we should be able observe that in a fairly simple test. 
Something
> > like:
> > 1) Visit pages A1 and A2.
> > 2) Go back to A1, so that A2 is in the forward history.
> > 3) Do a location.replace to A3.
> > 4) Check that history.length is 2, not 1.
> > (We should not have cleared the forward history or changed the offset.)
> > 
> > That can be checked pretty easily with Javascript code, so perhaps
> > navigation_controller_impl_browsertest.cc is a good place for it?
> 
> That's not the bug, though. (Your proposed test passes without the fix.)
> 
> The bug is when the navigation is initiated by the browser with the
> should_replace_current_entry flag set. I need to write a test for that.

I found it strange that renderer-initiated location.replace would work, but your
offline explanation makes sense: it probably isn't of type
blink::WebStandardCommit, so we probably don't treat it as a new navigation.

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

https://codereview.chromium.org/902083002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:111:
web_contents->Focus();
Is this line needed?  I don't see why the focus would change, and I didn't see
similar calls in content_browser_test_utils.

[Avi (use Gerrit), 2015-02-06 17:43:14]

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

https://codereview.chromium.org/902083002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:111:
web_contents->Focus();
On 2015/02/06 17:29:35, Charlie Reis wrote:
> Is this line needed?  I don't see why the focus would change, and I didn't see
> similar calls in content_browser_test_utils.

In content_browser_test_utils, NavigateToURLBlockUntilNavigationsComplete has
"window->LoadURL()". That is Shell::LoadURL, which calls through to
Shell::LoadURLForFrame which does exactly what I do in my test.

This is an unrolling of all the calls into one linear function. I don't know
what would happen, but I wanted to keep in line with what all the other versions
of this function do.

[Avi (use Gerrit), 2015-02-06 17:43:18]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2015-02-06 17:44:14]

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

[commit-bot: I haz the power, 2015-02-06 17:47:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-06 17:47:54]

Patchset 2 (id:??) landed as
https://crrev.com/26b1681180785dfcedc55cf0574e43f21de8fb10
Cr-Commit-Position: refs/heads/master@{#315060}