Add render_frame_host to WebContentsObserver::DidNavigateAnyFrame().

Add render_frame_host to WebContentsObserver::DidNavigateAnyFrame().

This will allow WebContentsObserver implementations to be aware if a
frame hase been navigated without having to worry about provisional load.

BUG=304341, 421085
Committed: https://crrev.com/2d2fd2fb805ae3b56a40b07485a2c5c24784cc63
Cr-Commit-Position: refs/heads/master@{#298722}

[mlamouri (slow - plz ping), 2014-10-07 15:03:40]

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

[mlamouri (slow - plz ping), 2014-10-07 15:03:41]

creis@, how does that sound to add the |render_frame_host| in
DidNavigateAnyFrame()? It would be simper for a consumer of that API to know if
the navigation actually happened.

It sounds simpler than requesting consumers to all have to use provisional loads
related callbacks or re-implement part of the logic around
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

This is being used by https://codereview.chromium.org/625963003/ and might be
used by other places handling permissions in order to cancel the permission
request from the browser instead of having Blink asking for it.

[Charlie Reis, 2014-10-07 17:32:05]

[+nasko,nick,site-isolation-reviews]

On 2014/10/07 15:03:41, Mounir Lamouri wrote:
> creis@, how does that sound to add the |render_frame_host| in
> DidNavigateAnyFrame()? It would be simper for a consumer of that API to know
if
> the navigation actually happened.
> 
> It sounds simpler than requesting consumers to all have to use provisional
loads
> related callbacks or re-implement part of the logic around
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> 
> This is being used by https://codereview.chromium.org/625963003/ and might be
> used by other places handling permissions in order to cancel the permission
> request from the browser instead of having Blink asking for it.

Actually, from John's advice we're trying to eliminate RenderFrameHost from the
public API.  This will reduce the chance that people will mistakenly assume the
RFH won't change over time, and it will simplify the API.  (This was in response
to the number of problems John encountered when RenderViewHost was exposed.)

Thus, we probably don't want to do this.  I think we're exposing routing IDs and
process IDs instead, but I don't have the details handy.  Nasko, do you remember
the specifics?

[nasko, 2014-10-07 18:50:44]

On 2014/10/07 17:32:05, Charlie Reis wrote:
> [+nasko,nick,site-isolation-reviews]
> 
> On 2014/10/07 15:03:41, Mounir Lamouri wrote:
> > creis@, how does that sound to add the |render_frame_host| in
> > DidNavigateAnyFrame()? It would be simper for a consumer of that API to know
> if
> > the navigation actually happened.
> > 
> > It sounds simpler than requesting consumers to all have to use provisional
> loads
> > related callbacks or re-implement part of the logic around
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > 
> > This is being used by https://codereview.chromium.org/625963003/ and might
be
> > used by other places handling permissions in order to cancel the permission
> > request from the browser instead of having Blink asking for it.
> 
> Actually, from John's advice we're trying to eliminate RenderFrameHost from
the
> public API.  This will reduce the chance that people will mistakenly assume
the
> RFH won't change over time, and it will simplify the API.  (This was in
response
> to the number of problems John encountered when RenderViewHost was exposed.)
> 
> Thus, we probably don't want to do this.  I think we're exposing routing IDs
and
> process IDs instead, but I don't have the details handy.  Nasko, do you
remember
> the specifics?

I think long term we want to hide RFH, but currently that is not possible.
WebContentsObserver already passes the RFH for most Did* callbacks, so adding
one to DidNavigateAnyFrame makes sense. It will also make it easier later on to
look wholesale for RFH exposure to remove (versus exposing process/routing id
pair).

[Charlie Reis, 2014-10-07 21:35:03]

On 2014/10/07 18:50:44, nasko wrote:
> On 2014/10/07 17:32:05, Charlie Reis wrote:
> > [+nasko,nick,site-isolation-reviews]
> > 
> > On 2014/10/07 15:03:41, Mounir Lamouri wrote:
> > > creis@, how does that sound to add the |render_frame_host| in
> > > DidNavigateAnyFrame()? It would be simper for a consumer of that API to
know
> > if
> > > the navigation actually happened.
> > > 
> > > It sounds simpler than requesting consumers to all have to use provisional
> > loads
> > > related callbacks or re-implement part of the logic around
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > > 
> > > This is being used by https://codereview.chromium.org/625963003/ and might
> be
> > > used by other places handling permissions in order to cancel the
permission
> > > request from the browser instead of having Blink asking for it.
> > 
> > Actually, from John's advice we're trying to eliminate RenderFrameHost from
> the
> > public API.  This will reduce the chance that people will mistakenly assume
> the
> > RFH won't change over time, and it will simplify the API.  (This was in
> response
> > to the number of problems John encountered when RenderViewHost was exposed.)
> > 
> > Thus, we probably don't want to do this.  I think we're exposing routing IDs
> and
> > process IDs instead, but I don't have the details handy.  Nasko, do you
> remember
> > the specifics?
> 
> I think long term we want to hide RFH, but currently that is not possible.
> WebContentsObserver already passes the RFH for most Did* callbacks, so adding
> one to DidNavigateAnyFrame makes sense. It will also make it easier later on
to
> look wholesale for RFH exposure to remove (versus exposing process/routing id
> pair).

Yes, I suppose you're right.  I didn't realize how many WebContentsObserver
methods already had it.  We'll need a more concrete plan to replace those cases
before we can deprecate RFH, so I suppose it doesn't make sense to take a
different approach here yet.

Please use bug number 304341, and otherwise LGTM.

[mlamouri (slow - plz ping), 2014-10-08 09:16:46]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org, mkosiba@chromium.org

[mlamouri (slow - plz ping), 2014-10-08 09:16:47]

mkosiba@, can you rs changes in android_webview/?
jochen@, can you rs changes in chrome/ and components/?

Thanks :)

[mkosiba (inactive), 2014-10-08 10:51:05]

lgtm

[jochen (gone - plz use gerrit), 2014-10-08 15:00:45]

lgtm

[mlamouri (slow - plz ping), 2014-10-08 15:31:37]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-10-08 15:32:56]

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

[commit-bot: I haz the power, 2014-10-08 16:00:18]

Committed patchset #4 (id:60001) as 3fd78d832096f261b69ba17b6a609aac5e6d8cff

[commit-bot: I haz the power, 2014-10-08 16:01:00]

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