Browser Plugin: Refactor BrowserPluginEmbedder to allow creating guests with openers

Browser Plugin: Refactor BrowserPluginEmbedder to allow creating guests with openers

Changes:

1. Move most of logic in BrowserPluginEmbedder:OnCreateGuest to BrowserPluginEmbedder::CreateGuest. BrowserPluginEmbedder::CreateGuest takes in an opener guest and a routing ID. BrowserPluginEmbedder::OnCreateGuest calls CreateGuest with a NULL opener.

2. WebContentsImpl::CreateGuest now takes in a routing_id, and a base_web_contents.

3. WebContentsImpl::opener() is now a public accessor.

4. BrowserPluginEmbedder::GetGuestByInstanceID and BrowserPluginEmbedder::DestroyGuestByInstanceID are now public to allow a guest to take (temporary) "ownership" of another guest until it is attached to the embedder's DOM tree.

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

[Fady Samuel, 2013-01-04 18:06:46]

+sadrul Please take a look at the changes overall.
+creis: for web_contents_impl changes and approval.

[Charlie Reis, 2013-01-04 18:53:19]

Looks pretty good.

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:84:
guest_site_instance = guest_opener->GetWebContents()->GetSiteInstance();
Yep, this looks right.  This will ensure the new guest and its opener share a
process, which is what we want.

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:114: WebContentsImpl*
base_web_contents = static_cast<WebContentsImpl*>(
What does base mean?  Might be clearer to call this opener_web_contents.

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:154: // it has not yet
necessarily been attached to a BrowserPlugin.
The comment makes it sound like we should be checking guest_opener here.  Should
we?

https://codereview.chromium.org/11748034/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/11748034/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.h:88: // Returns the opener
WebContentsImpl.
If we're making this accessible, let's say a little more:

Returns the opener WebContentsImpl, if any.  This can be set to null if the
opener is closed or the page clears its window.opener.

https://codereview.chromium.org/11748034/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.h:96: WebContentsImpl*
base_web_contents,
Please call this opener_web_contents.

[sadrul, 2013-01-07 15:09:54]

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:117:
web_contents()->GetBrowserContext(),
Should you be using the browser-context of opener_web_contents (when not null)?
Does it matter? (I notice that this is part of the code moved from
OnCreateGuest, as opposed to new code)

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.h:60: void
CreateGuest(int instance_id,
Doc

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.h:71: void
DestroyGuestByInstanceID(int instance_id);
It doesn't look like either GuestGuestByInstanceID or Destroy... is called from
anywhere outside of BPEmbedder. Is exposing these in public necessary for the
follow-on patches?

[Fady Samuel, 2013-01-08 17:00:49]

PTAL

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:84:
guest_site_instance = guest_opener->GetWebContents()->GetSiteInstance();
On 2013/01/04 18:53:19, creis wrote:
> Yep, this looks right.  This will ensure the new guest and its opener share a
> process, which is what we want.
OK

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:114: WebContentsImpl*
base_web_contents = static_cast<WebContentsImpl*>(
On 2013/01/04 18:53:19, creis wrote:
> What does base mean?  Might be clearer to call this opener_web_contents.

Done.

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:117:
web_contents()->GetBrowserContext(),
On 2013/01/07 15:09:54, sadrul wrote:
> Should you be using the browser-context of opener_web_contents (when not
null)?
> Does it matter? (I notice that this is part of the code moved from
> OnCreateGuest, as opposed to new code)

This is the embedder's BrowserContext. All guests use the embedder's browser
context. Charlie, could you please comment on whether this needs to change? I
guess I just don't know the scope of what is "browser context".

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:154: // it has not yet
necessarily been attached to a BrowserPlugin.
On 2013/01/04 18:53:19, creis wrote:
> The comment makes it sound like we should be checking guest_opener here. 
Should
> we?

This is a bad comment. Thanks for catching this! At some point I had an if
statement guarding Initialize in my code locally but then I realized it's not
necessary.

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.h:60: void
CreateGuest(int instance_id,
On 2013/01/07 15:09:54, sadrul wrote:
> Doc

Done.

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.h:71: void
DestroyGuestByInstanceID(int instance_id);
On 2013/01/07 15:09:54, sadrul wrote:
> It doesn't look like either GuestGuestByInstanceID or Destroy... is called
from
> anywhere outside of BPEmbedder. Is exposing these in public necessary for the
> follow-on patches?

This will be used by follow-on patches. I am making the simple changes here in
order to simply the review process in follow-on patches.

https://codereview.chromium.org/11748034/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/11748034/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.h:88: // Returns the opener
WebContentsImpl.
On 2013/01/04 18:53:19, creis wrote:
> If we're making this accessible, let's say a little more:
> 
> Returns the opener WebContentsImpl, if any.  This can be set to null if the
> opener is closed or the page clears its window.opener.

Done.

https://codereview.chromium.org/11748034/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.h:96: WebContentsImpl*
base_web_contents,
On 2013/01/04 18:53:19, creis wrote:
> Please call this opener_web_contents.

Done.

[Charlie Reis, 2013-01-08 21:13:52]

LGTM.

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11748034/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:117:
web_contents()->GetBrowserContext(),
On 2013/01/08 17:00:49, Fady Samuel wrote:
> On 2013/01/07 15:09:54, sadrul wrote:
> > Should you be using the browser-context of opener_web_contents (when not
> null)?
> > Does it matter? (I notice that this is part of the code moved from
> > OnCreateGuest, as opposed to new code)
> 
> This is the embedder's BrowserContext. All guests use the embedder's browser
> context. Charlie, could you please comment on whether this needs to change? I
> guess I just don't know the scope of what is "browser context".

The BrowserContext is the part of Profile that lives in the content module. 
You're right that the embedder and both old and new guests necessarily have the
same BrowserContext.  I don't think it needs to change.

https://codereview.chromium.org/11748034/diff/7001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/11748034/diff/7001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:62: // attached to a
|guest_opener|, and may be attached a pre-selected
nit: attached to?

[commit-bot: I haz the power, 2013-01-08 21:26:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11748034/11002