Remove frame ID from DidCommitProvisionalLoad.

Remove frame ID from DidCommitProvisionalLoad.

Also passes RenderFrameHosts to RendererDidNavigate and cleans up the
RenderFrameHost interface.

BUG=314791
TEST=No behavior change.
R=nasko@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254576

[Charlie Reis, 2014-03-01 01:10:51]

Here's another big chunk of frame ID and RFH related cleanup.  It's actually
starting to look like we intend to navigate via RenderFrameHosts now!

https://codereview.chromium.org/182713005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (left):

https://codereview.chromium.org/182713005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:926:
temp.append(base::IntToString(params.frame_id));
This was the motivation for tackling the RendererDidNavigate(rfh) TODO in this
CL.

[jam, 2014-03-01 01:21:08]

btw i've been lurking and appreciating all this cleanup :) some drive by
comments

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
content/public/browser/render_frame_host.h:31: virtual int GetRoutingID() const
= 0;
nit: here and in the rest of the file, i have avoided putting const identifier
on content api interfaces. the reason is that the embedder shouldn't care about
the implementation detail of a method. it's also cumbersome when a const method
suddenly changes implementation, and as a result, embedder code has to be
changed because it was passing const objects and now all these need to be
updated.

see
http://www.chromium.org/developers/content-module/content-api

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
content/public/browser/render_frame_host.h:40: virtual int GetEnabledBindings()
const = 0;
two questions:
right now this is only called inside content so it should be on
RenderFrameHostImpl until we need it externally

but more importantly, should this be a property of WebContentsImpl instead? i.e.
do we forsee that in the future we would set this on some frames only of a page,
but not the rest? if not, then i think it's simpler to keep this on WebContents,
as that way the embedder can interact with WebContents for the most part.
RenderFrameHost is really an implementation detail.

[Charlie Reis, 2014-03-01 01:39:05]

Thanks, good questions.  I'll take care of them when I upload the next patch.

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
content/public/browser/render_frame_host.h:31: virtual int GetRoutingID() const
= 0;
On 2014/03/01 01:21:08, jam wrote:
> nit: here and in the rest of the file, i have avoided putting const identifier
> on content api interfaces. the reason is that the embedder shouldn't care
about
> the implementation detail of a method. it's also cumbersome when a const
method
> suddenly changes implementation, and as a result, embedder code has to be
> changed because it was passing const objects and now all these need to be
> updated.
> 
> see
> http://www.chromium.org/developers/content-module/content-api

Ah, fair point.  I'll remove that.

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
content/public/browser/render_frame_host.h:40: virtual int GetEnabledBindings()
const = 0;
On 2014/03/01 01:21:08, jam wrote:
> two questions:
> right now this is only called inside content so it should be on
> RenderFrameHostImpl until we need it externally

Sure, I can change it in the next patch.

> but more importantly, should this be a property of WebContentsImpl instead?
i.e.
> do we forsee that in the future we would set this on some frames only of a
page,
> but not the rest? if not, then i think it's simpler to keep this on
WebContents,
> as that way the embedder can interact with WebContents for the most part.
> RenderFrameHost is really an implementation detail.

Well, it couldn't live on WebContents because it at least changes over time as
we navigate from one page to another.  Having it on RenderViewHost has helped us
catch a bunch of bugs of privileges leaking after a navigation.

That said, I can imagine case where one subframe has or lacks bindings compared
to the main frame or other frames, so I think it should eventually live on
RenderFrameHost.  One example is the <webview> tag inside apps.

[Charlie Reis, 2014-03-03 07:14:49]

New patchset up.  PTAL.

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
content/public/browser/render_frame_host.h:31: virtual int GetRoutingID() const
= 0;
On 2014/03/01 01:39:05, Charlie Reis wrote:
> On 2014/03/01 01:21:08, jam wrote:
> > nit: here and in the rest of the file, i have avoided putting const
identifier
> > on content api interfaces. the reason is that the embedder shouldn't care
> about
> > the implementation detail of a method. it's also cumbersome when a const
> method
> > suddenly changes implementation, and as a result, embedder code has to be
> > changed because it was passing const objects and now all these need to be
> > updated.
> > 
> > see
> > http://www.chromium.org/developers/content-module/content-api
> 
> Ah, fair point.  I'll remove that.

Done.

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
content/public/browser/render_frame_host.h:40: virtual int GetEnabledBindings()
const = 0;
On 2014/03/01 01:39:05, Charlie Reis wrote:
> On 2014/03/01 01:21:08, jam wrote:
> > two questions:
> > right now this is only called inside content so it should be on
> > RenderFrameHostImpl until we need it externally
> 
> Sure, I can change it in the next patch.
> 
> > but more importantly, should this be a property of WebContentsImpl instead?
> i.e.
> > do we forsee that in the future we would set this on some frames only of a
> page,
> > but not the rest? if not, then i think it's simpler to keep this on
> WebContents,
> > as that way the embedder can interact with WebContents for the most part.
> > RenderFrameHost is really an implementation detail.
> 
> Well, it couldn't live on WebContents because it at least changes over time as
> we navigate from one page to another.  Having it on RenderViewHost has helped
us
> catch a bunch of bugs of privileges leaking after a navigation.
> 
> That said, I can imagine case where one subframe has or lacks bindings
compared
> to the main frame or other frames, so I think it should eventually live on
> RenderFrameHost.  One example is the <webview> tag inside apps.

Done.

https://codereview.chromium.org/182713005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/182713005/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2112: int render_frame_id =
render_frame_host->GetRoutingID();
Nasko: I changed this to store the render_frame_id outside the observer loop
because of these failures in SigninSkipForNowAndGoBack:
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...

Looks like you added that test in
https://chromiumcodereview.appspot.com/19699007, and it's deleting
|render_frame_host| as we iterate through the observers.  The test passes again
so this CL is safe, but we may want to revisit how that test works if possible.

[nasko, 2014-03-03 15:01:22]

LGTM

https://codereview.chromium.org/182713005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/182713005/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2112: int render_frame_id =
render_frame_host->GetRoutingID();
On 2014/03/03 07:14:50, Charlie Reis wrote:
> Nasko: I changed this to store the render_frame_id outside the observer loop
> because of these failures in SigninSkipForNowAndGoBack:
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...
> 
> Looks like you added that test in
> https://chromiumcodereview.appspot.com/19699007, and it's deleting
> |render_frame_host| as we iterate through the observers.  The test passes
again
> so this CL is safe, but we may want to revisit how that test works if
possible.

The test will likely be removed as the old style signin code is phased out.

[jam, 2014-03-03 16:08:04]

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/182713005/diff/1/content/public/browser/rende...
content/public/browser/render_frame_host.h:40: virtual int GetEnabledBindings()
const = 0;
On 2014/03/01 01:39:05, Charlie Reis wrote:
> On 2014/03/01 01:21:08, jam wrote:
> > two questions:
> > right now this is only called inside content so it should be on
> > RenderFrameHostImpl until we need it externally
> 
> Sure, I can change it in the next patch.
> 
> > but more importantly, should this be a property of WebContentsImpl instead?
> i.e.
> > do we forsee that in the future we would set this on some frames only of a
> page,
> > but not the rest? if not, then i think it's simpler to keep this on
> WebContents,
> > as that way the embedder can interact with WebContents for the most part.
> > RenderFrameHost is really an implementation detail.
> 
> Well, it couldn't live on WebContents because it at least changes over time as
> we navigate from one page to another.  Having it on RenderViewHost has helped
us
> catch a bunch of bugs of privileges leaking after a navigation.

ah, makes senses! ignore that then :)

> 
> That said, I can imagine case where one subframe has or lacks bindings
compared
> to the main frame or other frames, so I think it should eventually live on
> RenderFrameHost.  One example is the <webview> tag inside apps.

https://codereview.chromium.org/182713005/diff/20001/content/public/browser/r...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/182713005/diff/20001/content/public/browser/r...
content/public/browser/render_frame_host.h:31: virtual int GetRoutingID() = 0;
nit: if you're changing this order, please also update the impl files

[Charlie Reis, 2014-03-03 17:05:23]

https://codereview.chromium.org/182713005/diff/20001/content/public/browser/r...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/182713005/diff/20001/content/public/browser/r...
content/public/browser/render_frame_host.h:31: virtual int GetRoutingID() = 0;
On 2014/03/03 16:08:04, jam wrote:
> nit: if you're changing this order, please also update the impl files

I already did, right?  Or did I put it in the wrong place?

[Charlie Reis, 2014-03-03 17:34:35]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-03-03 17:40:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/182713005/20001

[commit-bot: I haz the power, 2014-03-03 18:29:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/182713005/20001

[commit-bot: I haz the power, 2014-03-03 19:56:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/182713005/20001

[Charlie Reis, 2014-03-03 22:11:51]

Committed patchset #2 manually as r254576 (presubmit successful).