Browser Plugin: <webview> should inherit partition attribute of opener on attachment.

Browser Plugin: <webview> should inherit partition attribute of opener on attachment.

BUG=140316
Test=Updated WebViewTest.NewWindow

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

[Fady Samuel, 2013-03-25 17:24:00]

Hi Istiaque,

Could you please take an initial look at this? I'll pass this along to Charlie
afterward.

Fady

[lazyboy, 2013-03-25 18:12:55]

Some comments/nits, feel free to add Charlie as reviewer after addressing them,
thanks.

https://codereview.chromium.org/13032003/diff/1/chrome/test/data/extensions/p...
File chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js
(right):

https://codereview.chromium.org/13032003/diff/1/chrome/test/data/extensions/p...
chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js:23: '
partition="' + partitionName + '"' +
Also keeping a test that doesn't have any partition attribute specified seems
useful.

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

https://codereview.chromium.org/13032003/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.cc:726: 
nit: remove new line after comment line.

https://codereview.chromium.org/13032003/diff/1/content/common/browser_plugin...
File content/common/browser_plugin/browser_plugin_messages.h (right):

https://codereview.chromium.org/13032003/diff/1/content/common/browser_plugin...
content/common/browser_plugin/browser_plugin_messages.h:307: 
nit: Remove extra newline.

https://codereview.chromium.org/13032003/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/13032003/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:463: std::string
partition_name = (params.persist_storage ? "persist:" : "") +
s/"persist"/browser_plugin::kPersistPrefix

[Fady Samuel, 2013-03-25 19:28:20]

+creis: Please take a look.

https://codereview.chromium.org/13032003/diff/1/chrome/test/data/extensions/p...
File chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js
(right):

https://codereview.chromium.org/13032003/diff/1/chrome/test/data/extensions/p...
chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js:23: '
partition="' + partitionName + '"' +
On 2013/03/25 18:12:55, lazyboy wrote:
> Also keeping a test that doesn't have any partition attribute specified seems
> useful.

Done.

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

https://codereview.chromium.org/13032003/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.cc:726: 
On 2013/03/25 18:12:55, lazyboy wrote:
> nit: remove new line after comment line.

Done.

https://codereview.chromium.org/13032003/diff/1/content/common/browser_plugin...
File content/common/browser_plugin/browser_plugin_messages.h (right):

https://codereview.chromium.org/13032003/diff/1/content/common/browser_plugin...
content/common/browser_plugin/browser_plugin_messages.h:307: 
On 2013/03/25 18:12:55, lazyboy wrote:
> nit: Remove extra newline.

Done.

https://codereview.chromium.org/13032003/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/13032003/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:463: std::string
partition_name = (params.persist_storage ? "persist:" : "") +
On 2013/03/25 18:12:55, lazyboy wrote:
> s/"persist"/browser_plugin::kPersistPrefix

Done.

[Charlie Reis, 2013-03-26 17:02:32]

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

https://codereview.chromium.org/13032003/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:707: persist_storage_ =
opener()->persist_storage_;
The partition name and whether it is persistent is stored in our SiteInstance,
which was inherited from the opener.  (See
BrowserPluginGuestManager::CreateGuest.)  Do we need to keep a separate copy of
it?

https://codereview.chromium.org/13032003/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:731: new
BrowserPluginMsg_Attach_ACK(instance_id_, ack_params));
I don't understand why we need to send these in their own ACK message.

What is the timeline for window.open?  Is there another message that tells the
renderer to create the BrowserPlugin for the new guest?  Seems like these would
belong on that message, rather than happening after the fact.

https://codereview.chromium.org/13032003/diff/8001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/13032003/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:424:
allocate_instance_id_sent_ = true;
I don't understand this line.  We currently set it to true when calling
AllocateInstanceID, which makes sense.  Does this need to be renamed?

[Fady Samuel, 2013-03-26 17:15:40]

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

https://codereview.chromium.org/13032003/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:707: persist_storage_ =
opener()->persist_storage_;
On 2013/03/26 17:02:32, creis wrote:
> The partition name and whether it is persistent is stored in our SiteInstance,
> which was inherited from the opener.  (See
> BrowserPluginGuestManager::CreateGuest.)  Do we need to keep a separate copy
of
> it?

I thought about that. The issue is BrowserPluginGuestManager::CreateGuest
formats the partition as a URL and so we'd need to parse out these bits out of
the URL. If the URL format changes, the parsing would also need to change so
this is a bit brittle.

https://codereview.chromium.org/13032003/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:731: new
BrowserPluginMsg_Attach_ACK(instance_id_, ack_params));
On 2013/03/26 17:02:32, creis wrote:
> I don't understand why we need to send these in their own ACK message.
> 
> What is the timeline for window.open?  Is there another message that tells the
> renderer to create the BrowserPlugin for the new guest?  Seems like these
would
> belong on that message, rather than happening after the fact.

No, the BrowserPlugin is a pre-existing BrowserPlugin, it is not created on
window.open. We simply attach a new guest to a pre-existing BrowserPlugin.

https://codereview.chromium.org/13032003/diff/8001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/13032003/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:424:
allocate_instance_id_sent_ = true;
On 2013/03/26 17:02:32, creis wrote:
> I don't understand this line.  We currently set it to true when calling
> AllocateInstanceID, which makes sense.  Does this need to be renamed?

Possibly but I'm not sure what's a good name for it. Perhaps
navigation_initiated_? allocate_instance_id_sent_ is used a flag to indicate
that we will have a guest attached soon or we already have a guest attached.

We've been using it in BrowserPlugin::ParsePartitionAttribute to ensure that the
partition attribute cannot change again after a navigation has been initiated.

[Charlie Reis, 2013-03-26 17:49:30]

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

https://codereview.chromium.org/13032003/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:707: persist_storage_ =
opener()->persist_storage_;
On 2013/03/26 17:15:40, Fady Samuel wrote:
> On 2013/03/26 17:02:32, creis wrote:
> > The partition name and whether it is persistent is stored in our
SiteInstance,
> > which was inherited from the opener.  (See
> > BrowserPluginGuestManager::CreateGuest.)  Do we need to keep a separate copy
> of
> > it?
> 
> I thought about that. The issue is BrowserPluginGuestManager::CreateGuest
> formats the partition as a URL and so we'd need to parse out these bits out of
> the URL. If the URL format changes, the parsing would also need to change so
> this is a bit brittle.

I'm not happy with having a separate copy of these that could diverge. 
Conceptually, they're a property of the SiteInstance and process, and it's too
easy to end up with bugs if they differ.

The information is accessible as GURL::path() and GURL::query(), and I'd be fine
having helper methods on BrowserPluginGuest for getting to them.  I think it's
important to read them from the guest's SiteInstance, though.

https://codereview.chromium.org/13032003/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:731: new
BrowserPluginMsg_Attach_ACK(instance_id_, ack_params));
On 2013/03/26 17:15:40, Fady Samuel wrote:
> On 2013/03/26 17:02:32, creis wrote:
> > I don't understand why we need to send these in their own ACK message.
> > 
> > What is the timeline for window.open?  Is there another message that tells
the
> > renderer to create the BrowserPlugin for the new guest?  Seems like these
> would
> > belong on that message, rather than happening after the fact.
> 
> No, the BrowserPlugin is a pre-existing BrowserPlugin, it is not created on
> window.open. We simply attach a new guest to a pre-existing BrowserPlugin.

So the guest is already created in the correct partition, and we're just telling
it so that it can set its DOM attribute accordingly?  Ok, that sounds alright to
me.

https://codereview.chromium.org/13032003/diff/8001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/13032003/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:424:
allocate_instance_id_sent_ = true;
On 2013/03/26 17:15:40, Fady Samuel wrote:
> On 2013/03/26 17:02:32, creis wrote:
> > I don't understand this line.  We currently set it to true when calling
> > AllocateInstanceID, which makes sense.  Does this need to be renamed?
> 
> Possibly but I'm not sure what's a good name for it. Perhaps
> navigation_initiated_? allocate_instance_id_sent_ is used a flag to indicate
> that we will have a guest attached soon or we already have a guest attached.
> 
> We've been using it in BrowserPlugin::ParsePartitionAttribute to ensure that
the
> partition attribute cannot change again after a navigation has been initiated.


If that's the use case, then you could flip it around and call it
before_first_navigation_.  navigation_initiated_ might work as well, but it's
less clear that it's only referring to the first one.

[Fady Samuel, 2013-04-04 16:30:36]

PTAL charlie. I've addressed your comments.

[Charlie Reis, 2013-04-04 21:30:23]

Thanks.  LGTM with nit.

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

https://codereview.chromium.org/13032003/diff/18001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:755: const GURL& site_url
= GetWebContents()->GetSiteInstance()->GetSiteURL();
nit: Move this declaration below the comment, and add a bit of an explanation of
our assumptions to the comment.  Something like:
We pull the partition information from the site's URL, which is of the form
guest://site/{persist}?{partition_name}.

[Fady Samuel, 2013-04-04 22:40:31]

+cdn@ for IPC audit. Thanks.

[Cris Neckar, 2013-04-05 18:42:39]

lgtm for ipc changes

[commit-bot: I haz the power, 2013-04-05 18:43:16]

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

[commit-bot: I haz the power, 2013-04-05 23:27:58]

Change committed as 192646