Browser Plugin: Expose frame name changes to the content API

Browser Plugin: Expose frame name changes to the content API.

In the near future, the <webview> API will be moved out of content leaving only
the core BrowserPlugin.

This patch takes us towards that goal by exposing frame name updates to
WebContentsObserver.

BUG=166165

[Fady Samuel, 2013-04-08 21:08:31]

Hi Istiaque,

Could you please take a quick look at this before I pass it onto John?

Thanks,

Fady

[lazyboy, 2013-04-08 22:01:21]

lgtm

Look OK.

[Fady Samuel, 2013-04-08 22:02:46]

+jam@ for content/public review.

[Fady Samuel, 2013-04-08 22:03:10]

On 2013/04/08 22:02:46, Fady Samuel wrote:
> +jam@ for content/public review.

and content review (Charlie is on vacation).

[jam, 2013-04-09 16:11:51]

https://codereview.chromium.org/13467038/diff/6001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/13467038/diff/6001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:631: rvh->SetName(name_);
is the intent that this file will move out content, why is why you're calling a
RVH method instead of sending the IPC directly?

it looks like the IPC is only sent by here. why not just make it get answered by
a browser plugin class in the renderer and have it call out to webkit directly?
i.e. you don't need to change content for this (once this code is outside)

https://codereview.chromium.org/13467038/diff/6001/content/public/browser/web...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/13467038/diff/6001/content/public/browser/web...
content/public/browser/web_contents_observer.h:140: virtual void
DidUpdateFrameName(int frame_id,
nit: all the methods here are documented. please follow the convention

https://codereview.chromium.org/13467038/diff/6001/content/public/browser/web...
content/public/browser/web_contents_observer.h:143: RenderViewHost*
render_view_host) {}
seems this should match the order of WCImpl

[Charlie Reis, 2013-04-17 17:35:21]

[+nasko, since he's working on related frame stuff]

Fady, does this need to be specific to BrowserPlugin at all?  Nasko and I want
the browser process to know window and frame names for the OOP iframe logic, so
it seems like it would be much simpler to just have RenderView notify
RenderViewHost and WebContents any time the top-level frame name changes without
involving any BrowserPlugin logic.

The end goal would be to have a WebContentsImpl::window_name_ (probably next to
WebContentsImpl::opener_), and you can add an event to the WebContentsObserver
if you need it there.  (Nasko will later make the names specific to each frame,
once we start keeping track of the frame tree in the browser process.)

[Fady Samuel, 2013-04-18 19:41:57]

PTAL nasko, or creis

[Charlie Reis, 2013-04-18 21:06:56]

Sorry, not sure if I was clear.  This is still being set by BrowserPlugin code,
and not by anything else.  I meant that all RenderViews should listen for
top-level window name changes from their WebFrames, and then notify
RenderViewHost and WebContentsImpl.  I don't see why we need anything specific
to BrowserPlugin.

Maybe it would be clearer to have a bug saying that WebContentsImpl should know
the window name?  (It wouldn't need to mention BrowserPlugin at all.)

Also, let's not add it to the public WebContents API until someone actually
needs it.  Adding it to WebContentsObserver might be ok if you have immediate
plans to use it there.

[Charlie Reis, 2013-04-18 21:17:38]

On 2013/04/18 21:06:56, creis wrote:
> Sorry, not sure if I was clear.  This is still being set by BrowserPlugin
code,
> and not by anything else.  I meant that all RenderViews should listen for
> top-level window name changes from their WebFrames, and then notify
> RenderViewHost and WebContentsImpl.  I don't see why we need anything specific
> to BrowserPlugin.
> 
> Maybe it would be clearer to have a bug saying that WebContentsImpl should
know
> the window name?  (It wouldn't need to mention BrowserPlugin at all.)
> 
> Also, let's not add it to the public WebContents API until someone actually
> needs it.  Adding it to WebContentsObserver might be ok if you have immediate
> plans to use it there.

I appear to have misunderstood your CL, after chatting with Nasko.  I'll need to
take another look.

[Charlie Reis, 2013-04-18 21:26:50]

Ok, Nasko caught my mistake: I didn't realize RenderView was already
broadcasting the name updates, so some of this work is already done.

I think we understand why you're adding it to WebContentsObserver, but I don't
understand why the get/set API is needed on WebContents.

https://codereview.chromium.org/13467038/diff/12001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/13467038/diff/12001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:210:
GetWebContents()->SetWindowName(params.name);
Why is this needed?  Won't the RenderView tell the WebContents?

https://codereview.chromium.org/13467038/diff/12001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:376:
new_contents_impl->SetWindowName(UTF16ToUTF8(frame_name));
Again, won't RenderView tell us?

https://codereview.chromium.org/13467038/diff/12001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:965:
GetWebContents()->SetWindowName(name);
Again, why is this needed?  If we change the name in the RenderView, it will let
the WebContents know on its own.

https://codereview.chromium.org/13467038/diff/12001/content/public/browser/we...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/13467038/diff/12001/content/public/browser/we...
content/public/browser/web_contents.h:183: // Allows reading and writing the
name of the top-level window.
I don't understand why we need this in the public WebContents API, since it
seems like the WebContentsObserver thing should be sufficient.  Can we eliminate
this?

https://codereview.chromium.org/13467038/diff/12001/content/public/browser/we...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/13467038/diff/12001/content/public/browser/we...
content/public/browser/web_contents_observer.h:140: virtual void
DidUpdateFrameName(int frame_id,
This needs a comment.