Remove the use of page id from building the commit params.

Remove the use of page id from building the commit params.

BUG=369661
TEST=tests stay green

Committed: https://crrev.com/66f3ec8e5ee6c3b528b5a6054cdca85ac0b29699
Reverted: https://crrev.com/96545af0f94ec1ac19a57c86806c95834de5d33c

Committed: https://crrev.com/5fe8124cafd9ea996b99cfc88366315aad0c63b2
Reverted: https://crrev.com/b05344954395b9cbb021057269875d931268dfd0

Committed: https://crrev.com/8c46f7e14826aff30a5fe3bb0751380e0078609d
Cr-Commit-Position: refs/heads/master@{#314370}

[Avi (use Gerrit), 2015-01-14 16:33:00]

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

[Charlie Reis, 2015-01-14 18:18:40]

https://codereview.chromium.org/839413004/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/839413004/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:3688: if (render_view_->page_id_ >
render_view_->last_page_id_sent_to_browser_)
If this works, that means last_page_id_sent_to_browser_ is dead code now.  Want
to remove it?

https://codereview.chromium.org/839413004/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/839413004/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:3686: if
(ds->replacesCurrentHistoryItem())
Interesting.  This is true for the first navigation in a frame?  That's great.

What about back/forward in a subframe?  I think that's supposed to report
AUTO_SUBFRAME.  (If we don't have test coverage for that, we should add it.  I
think most of NavigationController's tests in this area are unit tests that
don't check what the renderer reports.)

[Avi (use Gerrit), 2015-01-14 21:23:20]

https://codereview.chromium.org/839413004/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/839413004/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:3686: if
(ds->replacesCurrentHistoryItem())
On 2015/01/14 18:18:40, Charlie Reis wrote:
> Interesting.  This is true for the first navigation in a frame?  That's great.

Yes, it is. That's the only way in my rework that I can tell the difference.

> What about back/forward in a subframe?  I think that's supposed to report
> AUTO_SUBFRAME.

Dammit, no. Going backwards, replacesCurrentHistoryItem returns false (as it's a
real nav) but the page id will obviously not be bigger.

> (If we don't have test coverage for that, we should add it.  I
> think most of NavigationController's tests in this area are unit tests that
> don't check what the renderer reports.)

Ugh. Lemme get to that.

[Avi (use Gerrit), 2015-01-16 15:28:25]

Charlie, this is better now. A test, a better implementation, and the world is
good.

[Charlie Reis, 2015-01-23 07:19:27]

Great-- I like the new fix, and it's good to have a test for this.

[Charlie Reis, 2015-01-23 07:19:55]

And LGTM, of course.

[Charlie Reis, 2015-01-23 07:20:01]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-01-23 07:20:51]

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

[commit-bot: I haz the power, 2015-01-23 07:40:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-23 07:41:55]

Patchset 3 (id:??) landed as
https://crrev.com/66f3ec8e5ee6c3b528b5a6054cdca85ac0b29699
Cr-Commit-Position: refs/heads/master@{#312791}

[kochi, 2015-01-23 09:29:47]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/867963002/ by kochi@chromium.org.

The reason for reverting is: The series of changes (r312793, r312792, r312791)
are suspected to break
pushState() related layout tests in Blink.  Might be some of them only affected,
but let me revert all of them.

BUG=451351.

[Avi (use Gerrit), 2015-01-27 23:40:15]

FYI, it turns out that when pushState is used, replacesCurrentHistoryItem() is
true. ಠ_ಠ. This is rather un-awesome; investigating alternatives.

[Charlie Reis, 2015-01-27 23:46:29]

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

[Charlie Reis, 2015-01-27 23:46:29]

[+japhet]

On 2015/01/27 23:40:15, Avi wrote:
> FYI, it turns out that when pushState is used, replacesCurrentHistoryItem() is
> true. ಠ_ಠ. This is rather un-awesome; investigating alternatives.

That sounds like a bug.  Nate, should
WebDataSource::replacesCurrentHistoryItem() be returning true for pushState?  It
seems like it should for replaceState, but pushState creates a new history item.

[Nate Chapin, 2015-01-27 23:47:23]

On 2015/01/27 23:46:29, Charlie Reis (slow until 1-29) wrote:
> [+japhet]
> 
> On 2015/01/27 23:40:15, Avi wrote:
> > FYI, it turns out that when pushState is used, replacesCurrentHistoryItem()
is
> > true. ಠ_ಠ. This is rather un-awesome; investigating alternatives.
> 
> That sounds like a bug.  Nate, should
> WebDataSource::replacesCurrentHistoryItem() be returning true for pushState? 
It
> seems like it should for replaceState, but pushState creates a new history
item.

I agree, pushState() should not return true for replacesCurrentHistoryItem.

[Avi (use Gerrit), 2015-01-27 23:49:00]

On 2015/01/27 23:47:23, Nate Chapin wrote:
> On 2015/01/27 23:46:29, Charlie Reis (slow until 1-29) wrote:
> > [+japhet]
> > 
> > On 2015/01/27 23:40:15, Avi wrote:
> > > FYI, it turns out that when pushState is used,
replacesCurrentHistoryItem()
> is
> > > true. ಠ_ಠ. This is rather un-awesome; investigating alternatives.
> > 
> > That sounds like a bug.  Nate, should
> > WebDataSource::replacesCurrentHistoryItem() be returning true for pushState?

> It
> > seems like it should for replaceState, but pushState creates a new history
> item.
> 
> I agree, pushState() should not return true for replacesCurrentHistoryItem.

OK, let me dig into Blink. This CL makes sense, so let's see if we can fix
things on that side of the divide.

[Nate Chapin, 2015-01-27 23:49:48]

On 2015/01/27 23:47:23, Nate Chapin wrote:
> On 2015/01/27 23:46:29, Charlie Reis (slow until 1-29) wrote:
> > [+japhet]
> > 
> > On 2015/01/27 23:40:15, Avi wrote:
> > > FYI, it turns out that when pushState is used,
replacesCurrentHistoryItem()
> is
> > > true. ಠ_ಠ. This is rather un-awesome; investigating alternatives.
> > 
> > That sounds like a bug.  Nate, should
> > WebDataSource::replacesCurrentHistoryItem() be returning true for pushState?

> It
> > seems like it should for replaceState, but pushState creates a new history
> item.
> 
> I agree, pushState() should not return true for replacesCurrentHistoryItem.

Interesting, yeah, pushState/replaceState don't update that start on
DocumentLoader, so they get whatever value was set when that DocumentLoader was
created. The setReplacesCurrentHistoryItem() call in
FrameLoader::loadInSameDocument should probably move to
DocumentLoader::updateForSameDocumentNavigation() and take the
SameDocumentNavigationSource parameter into consideration.

[Avi (use Gerrit), 2015-01-28 00:25:10]

On 2015/01/27 23:49:48, Nate Chapin wrote:
> Interesting, yeah, pushState/replaceState don't update that start on
> DocumentLoader, so they get whatever value was set when that DocumentLoader
was
> created. The setReplacesCurrentHistoryItem() call in
> FrameLoader::loadInSameDocument should probably move to
> DocumentLoader::updateForSameDocumentNavigation() and take the
> SameDocumentNavigationSource parameter into consideration.

First, the SameDocumentNavigationSource parameter is
SameDocumentNavigationHistoryApi for all uses of the history api, pushState as
well as replaceState, so we can't use it. Second, History::stateObjectAdded
calls to the FrameLoader's updateForSameDocumentNavigation, not the
DocumentLoader's.

I'm looking into how much info the FrameLoader's updateForSameDocumentNavigation
has if we can put it there.

[Avi (use Gerrit), 2015-01-28 21:26:57]

With the blink fix, this looks like it works now.

[Avi (use Gerrit), 2015-01-28 21:28:15]

The CQ bit was checked by avi@chromium.org

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

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

[commit-bot: I haz the power, 2015-01-28 21:33:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-28 21:34:41]

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

[Avi (use Gerrit), 2015-01-29 19:53:13]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/874743003/ by avi@chromium.org.

The reason for reverting is:
NavigationControllerBrowserTest.ManualAndAutoSubframeNavigationTransitions fails
on the --site-per-process FYI bot.

[Charlie Reis, 2015-02-02 17:50:32]

Test updates LGTM with nits.

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

https://codereview.chromium.org/839413004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:95:
node->current_frame_host()->delegate()->GetAsWebContents()),
This feels a bit awkward to go through the RFH's delegate to get the
WebContents.  Maybe it's worth passing that in as a separate parameter?

(I agree that we shouldn't just use web_contents() here, which could be wrong in
a bunch of cases, like tests that have a separate tab, or the upcoming plans for
<webview>.)

https://codereview.chromium.org/839413004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:125: // The
id of the FrameTreeNode in which navigations are peformed.
of the FrameTreeNode to observe.

(It's fine for navigations to occur in other frames as well; this class will
still work.)

https://codereview.chromium.org/839413004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:140: // in
two different enums; http://crbug.com/453555
nit: End with period.  (Codesearch gets the link right.)

https://codereview.chromium.org/839413004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:159:
embedded_test_server()->GetURL("/frame_tree/2-1.html"));
nit: Wrong indent.

https://codereview.chromium.org/839413004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:189:
embedded_test_server()->GetURL("/frame_tree/2-3.html"));
nit: Wrong indent.

[Avi (use Gerrit), 2015-02-02 20:59:36]

PTAL.

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

https://codereview.chromium.org/839413004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:95:
node->current_frame_host()->delegate()->GetAsWebContents()),
On 2015/02/02 17:50:32, Charlie Reis wrote:
> This feels a bit awkward to go through the RFH's delegate to get the
> WebContents.

This is stolen straight from TestFrameNavigationObserver. I don't mind the
complexity; I'd rather pass in one item and be awkward getting everything we
need if it guarantees that nothing is contradictory.

https://codereview.chromium.org/839413004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:125: // The
id of the FrameTreeNode in which navigations are peformed.
On 2015/02/02 17:50:32, Charlie Reis wrote:
> of the FrameTreeNode to observe.
> 
> (It's fine for navigations to occur in other frames as well; this class will
> still work.)

Done.

https://codereview.chromium.org/839413004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:140: // in
two different enums; http://crbug.com/453555
On 2015/02/02 17:50:32, Charlie Reis wrote:
> nit: End with period.  (Codesearch gets the link right.)

Done.

https://codereview.chromium.org/839413004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:159:
embedded_test_server()->GetURL("/frame_tree/2-1.html"));
On 2015/02/02 17:50:32, Charlie Reis wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/839413004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:189:
embedded_test_server()->GetURL("/frame_tree/2-3.html"));
On 2015/02/02 17:50:32, Charlie Reis wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/839413004/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/839413004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:523: }
My question is whether this block is useful any more. It was smashing the
transition in the unit test, but it turns out that when we do a cross-process
subframe transition, we're getting the correct value!

I made a quick CL to remove it (https://codereview.chromium.org/894843002) but
without a --site-per-process trybot, try results there are useless.

[Avi (use Gerrit), 2015-02-02 21:06:21]

https://codereview.chromium.org/839413004/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/839413004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:523: }
On 2015/02/02 20:59:36, Avi wrote:
> it turns out that when we do a cross-process
> subframe transition, we're getting the correct value!

What I mean is, in the
NavigationControllerBrowserTest.ManualAndAutoSubframeNavigationClassification
test, where we explicitly generate both types of subframe navigations, it passes
in --site-per-process with this block completely gone.

Because that is stated in the comments as being the point of the block, is the
block useless now?

[Charlie Reis, 2015-02-02 21:24:29]

LGTM if you remove that whole block in navigator_impl.cc.

https://codereview.chromium.org/839413004/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/839413004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:523: }
On 2015/02/02 21:06:21, Avi wrote:
> On 2015/02/02 20:59:36, Avi wrote:
> > it turns out that when we do a cross-process
> > subframe transition, we're getting the correct value!
> 
> What I mean is, in the
> NavigationControllerBrowserTest.ManualAndAutoSubframeNavigationClassification
> test, where we explicitly generate both types of subframe navigations, it
passes
> in --site-per-process with this block completely gone.
> 
> Because that is stated in the comments as being the point of the block, is the
> block useless now?

Yes!  I'm pretty sure that your render_frame_impl.cc change makes it
unnecessary.  I'm happy to see this block die.

(Indeed, I'd already killed it for similar reasons in the draft session history
CL:
https://codereview.chromium.org/281653003/diff/100001/content/browser/frame_h...)

[Avi (use Gerrit), 2015-02-02 22:09:13]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2015-02-02 22:10:18]

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

[commit-bot: I haz the power, 2015-02-02 23:04:49]

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

[commit-bot: I haz the power, 2015-02-02 23:04:50]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2015-02-03 17:39:39]

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

[Avi (use Gerrit), 2015-02-03 17:39:40]

Charlie, Nasko:

Please look at the WebContentsImplTest.StartStopEventsBalance test change.
Charlie because you know this area, and Nasko, this is your test.

https://codereview.chromium.org/839413004/diff/140001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/839413004/diff/140001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:2912: subframe, 3,
bar_url, ui::PAGE_TRANSITION_MANUAL_SUBFRAME);
This seemed to work before because the code in the NavigatorImpl was overriding
this value.

AFAIK, subframes can only ever have the transition types manual subframe and
auto subframe. Charlie, is that true? It certainly seems to be enforced by the
code.

[nasko, 2015-02-03 17:45:48]

https://codereview.chromium.org/839413004/diff/140001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/839413004/diff/140001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:2912: subframe, 3,
bar_url, ui::PAGE_TRANSITION_MANUAL_SUBFRAME);
On 2015/02/03 17:39:39, Avi wrote:
> This seemed to work before because the code in the NavigatorImpl was
overriding
> this value.
> 
> AFAIK, subframes can only ever have the transition types manual subframe and
> auto subframe. Charlie, is that true? It certainly seems to be enforced by the
> code.

Totally makes sense. More of a copy/paste error on my side than intentional
value.

[Charlie Reis, 2015-02-03 17:46:56]

https://codereview.chromium.org/839413004/diff/140001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/839413004/diff/140001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:2912: subframe, 3,
bar_url, ui::PAGE_TRANSITION_MANUAL_SUBFRAME);
On 2015/02/03 17:45:48, nasko wrote:
> On 2015/02/03 17:39:39, Avi wrote:
> > This seemed to work before because the code in the NavigatorImpl was
> overriding
> > this value.
> > 
> > AFAIK, subframes can only ever have the transition types manual subframe and
> > auto subframe. Charlie, is that true? It certainly seems to be enforced by
the
> > code.
> 
> Totally makes sense. More of a copy/paste error on my side than intentional
> value.

Great.  LGTM.

[Avi (use Gerrit), 2015-02-03 17:59:39]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2015-02-03 18:00:44]

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

[commit-bot: I haz the power, 2015-02-03 18:28:14]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-03 18:30:08]

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