Remove page id from FrameNavigateParams.

Remove page id from FrameNavigateParams.

BUG=407376
TEST=no visible change

[Avi (use Gerrit), 2014-09-10 22:38:03]

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

[Avi (use Gerrit), 2014-09-10 22:38:04]

Charlie, WDYT? This gets rid of the page id parameter from FrameNavigateParams,
and more importantly, its subclass FrameHostMsg_DidCommitProvisionalLoad_Params.

Of course, this can't go in until the current page id patch sticks permanently,
but I wanted to get your thoughts.

This is a big patch, but the way to look at it is at both sides of
FrameHostMsg_DidCommitProvisionalLoad. On the sending side, page id is no longer
put into the FrameHostMsg_DidCommitProvisionalLoad_Params, and on the receiving
side, the page id is retrieved from the earlier FrameHostMsg_DidAssignPageId
message and passed alongside of the FrameHostMsg_DidCommitProvisionalLoad_Params
wherever it is needed.

[Charlie Reis, 2014-09-11 22:55:40]

Yes, I think this will work.  It does a nice job of preparing the browser-side
of things for a world where the page ID doesn't come from the renderer.

LGTM with one test question.

https://codereview.chromium.org/556703004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/556703004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:675: this,
render_view_host_->page_id_, validated_params);
Yes, I think having the page ID on the RVH should work here (even though many
different RFHs may access it), since the current page ID applies to all
subframes on the page.

https://codereview.chromium.org/556703004/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/556703004/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_unittest.cc:1464:
interstitial->TestDidNavigate(url2);
All of these tests used to pass a page ID to TestDidNavigate, and now it's gone
from the test entirely (unlike the other cases where it now gets passed in
elsewhere).  Is the page ID entirely unnecessary in this case?

[Avi (use Gerrit), 2014-09-11 23:10:10]

https://codereview.chromium.org/556703004/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/556703004/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_unittest.cc:1464:
interstitial->TestDidNavigate(url2);
On 2014/09/11 22:55:39, Charlie Reis wrote:
> All of these tests used to pass a page ID to TestDidNavigate, and now it's
gone
> from the test entirely (unlike the other cases where it now gets passed in
> elsewhere).  Is the page ID entirely unnecessary in this case?

In a previous patchset, I commented TestDidNavigate with:

// *** page_id?

I was shocked, but the page id doesn't seem to matter. The interstitial page, if
you look at those files in this patch, ignored the page id parameter that was
passed into them, so I guess it makes sense that we can drop all of these.

The bots are green. Surely that means that this patch is correct? :)

[Charlie Reis, 2014-09-11 23:15:28]

https://codereview.chromium.org/556703004/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/556703004/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_unittest.cc:1464:
interstitial->TestDidNavigate(url2);
On 2014/09/11 23:10:10, Avi wrote:
> On 2014/09/11 22:55:39, Charlie Reis wrote:
> > All of these tests used to pass a page ID to TestDidNavigate, and now it's
> gone
> > from the test entirely (unlike the other cases where it now gets passed in
> > elsewhere).  Is the page ID entirely unnecessary in this case?
> 
> In a previous patchset, I commented TestDidNavigate with:
> 
> // *** page_id?
> 
> I was shocked, but the page id doesn't seem to matter. The interstitial page,
if
> you look at those files in this patch, ignored the page id parameter that was
> passed into them, so I guess it makes sense that we can drop all of these.

Ah, that makes sense.  Interstitials don't maintain a session history of their
own; they just want to know if their one page has committed or not.

> The bots are green. Surely that means that this patch is correct? :)

[Avi (use Gerrit), 2014-12-10 22:20:33]

Closing; in discussions we decided to not go this route.