Browser Plugin: Allocate Instance IDs in BrowserPluginEmbedder instead of BrowserPluginManager

Browser Plugin: Allocate Instance IDs in BrowserPluginEmbedder instead of BrowserPluginManager

This patch is a step towards enabling window.open.

How it works:

1. On the initial navigation, BrowserPlugin asynchronously requests a new Instance ID from the browser process.

2. The request and response BrowserPluginHostMsg_AllocateInstanceIDRequest, and BrowserPluginMsg_AllocateInstanceIDResponse are managed by BrowserPluginManager.

3. Once the browser process has allocated an instance ID and relayed it to BrowserPlugin, then BrowserPlugin collects all the current state of BrowserPlugin and stores it in a BrowserPluginHostMsg_CreateGuest_Params struct and ships it to the browser process using the new instance ID to create a new guest with that initial state. Initial state includes the current value of the src attribute.

4. Once BrowserPlugin has an instance ID, it will begin sending IPCs to the browser process as state changes, rather than merely accumulating state changes in member variables.

BUG=140316
Test=BrowserPluginHostTest.*, BrowserPluginTest.*, WebViewTest.*

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

[lazyboy, 2013-01-17 17:39:25]

Some comments.

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_embedder.cc:35:
instance_counter_(0) {
prefer naming it such that it's clear that this is related to instance_id_
field, e.g. instance_id_counter_

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:724:
ASSERT_NE(new_test_guest, static_cast<void*>(NULL));
nit: maybe ASSERT_TRUE(new_tests_guest != NULL); is more readable

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/browser/web...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:2359: // The first
BrowserPluginHostMsg_CreateGuest message from this WebContents'
Update this comment.

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/common/brow...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/common/brow...
content/common/browser_plugin_messages.h:252: int /* instance_id*/)
nit: space after instance_id

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (left):

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:246: // Record that we sent a
NavigateGuest message to embedder.
Keep this comment in OnAllocateInstanceIDResponse()?

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:145: // If the BrowserPlugin
has never navigated then the browser process, and
Do we have a test that destroys BrowserPlugin before navigating it?

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:236: if
(!allocate_instance_id_in_flight_) {
I'd put a short version of your CL description as code comment here, describing
how the flow is supposed to work.

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:238:
allocate_instance_id_in_flight_ = true;
more state booleans. possible cleanup opportunities later:
from looking at navigate_src_sent_ and allocate_instance_id_in_flight_, seems
the browser plugin can be in the following 3 states regarding these booleans
(not 4),
1. not_navigated_and_not_allocated
2. allocating (in flight..)
3. navigated
and once it is in a step x, it cannot go back to state < x.

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:18:
request_counter_(0) {
nit: request_id_counter_ so it's obvious that this is related to request_id-s

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:34:
requests_.AddWithID(browser_plugin, request_id);
requests_ probably too broad name, maybe name it to be more specific? e.g.
pending_allocate_instance_id_requests_

https://chromiumcodereview.appspot.com/11956022/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:76:
plugin->OnMessageReceived(message);
Can we just read both request_id and instance_id (by making the handler
IPC_MESSAGE_HANDLER instead of *HANDLER_GENERIC) from message and call helper
function directly?:
plugin->AllocateInstanceId(request_id, instance_id);
Otherwise seems a bit confusing to handle this message in both places where
BrowserPlugin never directly receives this msg as IPC.

[Fady Samuel, 2013-01-17 18:40:38]

PTAL

https://codereview.chromium.org/11956022/diff/2001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11956022/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:35:
instance_counter_(0) {
On 2013/01/17 17:39:25, lazyboy wrote:
> prefer naming it such that it's clear that this is related to instance_id_
> field, e.g. instance_id_counter_

Done.

https://codereview.chromium.org/11956022/diff/2001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11956022/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:724:
ASSERT_NE(new_test_guest, static_cast<void*>(NULL));
On 2013/01/17 17:39:25, lazyboy wrote:
> nit: maybe ASSERT_TRUE(new_tests_guest != NULL); is more readable

Done.

https://codereview.chromium.org/11956022/diff/2001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11956022/diff/2001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:2359: // The first
BrowserPluginHostMsg_CreateGuest message from this WebContents'
On 2013/01/17 17:39:25, lazyboy wrote:
> Update this comment.

Done.

https://codereview.chromium.org/11956022/diff/2001/content/common/browser_plu...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11956022/diff/2001/content/common/browser_plu...
content/common/browser_plugin_messages.h:252: int /* instance_id*/)
On 2013/01/17 17:39:25, lazyboy wrote:
> nit: space after instance_id

Done.

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (left):

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:246: // Record that we sent a
NavigateGuest message to embedder.
On 2013/01/17 17:39:25, lazyboy wrote:
> Keep this comment in OnAllocateInstanceIDResponse()?

Done.

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

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:145: // If the BrowserPlugin
has never navigated then the browser process, and
On 2013/01/17 17:39:25, lazyboy wrote:
> Do we have a test that destroys BrowserPlugin before navigating it?

Done. Added a test.

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:236: if
(!allocate_instance_id_in_flight_) {
On 2013/01/17 17:39:25, lazyboy wrote:
> I'd put a short version of your CL description as code comment here,
describing
> how the flow is supposed to work.

Done.

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:236: if
(!allocate_instance_id_in_flight_) {
On 2013/01/17 17:39:25, lazyboy wrote:
> I'd put a short version of your CL description as code comment here,
describing
> how the flow is supposed to work.

Done.

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:236: if
(!allocate_instance_id_in_flight_) {
On 2013/01/17 17:39:25, lazyboy wrote:
> I'd put a short version of your CL description as code comment here,
describing
> how the flow is supposed to work.

Done.

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:238:
allocate_instance_id_in_flight_ = true;
On 2013/01/17 17:39:25, lazyboy wrote:
> more state booleans. possible cleanup opportunities later:
> from looking at navigate_src_sent_ and allocate_instance_id_in_flight_, seems
> the browser plugin can be in the following 3 states regarding these booleans
> (not 4),
> 1. not_navigated_and_not_allocated
> 2. allocating (in flight..)
> 3. navigated
> and once it is in a step x, it cannot go back to state < x.

Agreed, there's room for cleanup here. mthiesse@ is doing a cleanup that might
improve things a bit here. Really, instance_id_ == INSTANCE_ID_NONE and
navigate_src_sent_ more or less mean the same thing now but that's possibly less
obvious.

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:18:
request_counter_(0) {
On 2013/01/17 17:39:25, lazyboy wrote:
> nit: request_id_counter_ so it's obvious that this is related to request_id-s

Done.

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:34:
requests_.AddWithID(browser_plugin, request_id);
On 2013/01/17 17:39:25, lazyboy wrote:
> requests_ probably too broad name, maybe name it to be more specific? e.g.
> pending_allocate_instance_id_requests_

Done.

https://codereview.chromium.org/11956022/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:76:
plugin->OnMessageReceived(message);
On 2013/01/17 17:39:25, lazyboy wrote:
> Can we just read both request_id and instance_id (by making the handler
> IPC_MESSAGE_HANDLER instead of *HANDLER_GENERIC) from message and call helper
> function directly?:
> plugin->AllocateInstanceId(request_id, instance_id);
> Otherwise seems a bit confusing to handle this message in both places where
> BrowserPlugin never directly receives this msg as IPC.

I'd prefer to leave it this way because eventually, when I introduce
BrowserPluginObservers: https://codereview.chromium.org/11826005/

I'll like Observers to get an opportunity to handle as many message types as
possible, in order to allow as much opportunity for modularity as possible.

[lazyboy, 2013-01-17 19:51:17]

lgtm

[Fady Samuel, 2013-01-17 19:54:05]

+creis@ for content OWNER review.

Thanks,

Fady

[Charlie Reis, 2013-01-18 01:54:31]

Nice.  Glad that we're making the browser responsible for the instance IDs,
which should work well with window.open.  A few nits plus a question about the
IPC message handler.

https://codereview.chromium.org/11956022/diff/9001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11956022/diff/9001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:310: if
(params.src.empty())
Nice.  Can you update the comment in the .h file to say that this will navigate
the guest if the params has a src?

(Also, I'm fine with it as is, but is there a reason you put the check here
instead of inside CreateGuest?)

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

https://codereview.chromium.org/11956022/diff/9001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:152: int
instance_id_counter_;
Should we be more consistent with the name above, like next_instance_id_?

https://codereview.chromium.org/11956022/diff/9001/content/common/browser_plu...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11956022/diff/9001/content/common/browser_plu...
content/common/browser_plugin_messages.h:128:
IPC_MESSAGE_ROUTED1(BrowserPluginHostMsg_AllocateInstanceIDRequest,
We have a lot of similar Foo/Foo_ACK message pairs in the code, and the ACK
isn't always empty (e.g., ViewMsg_AsyncOpenFile_ACK).  Perhaps these should be
named AllocateInstanceID and AllocateInstanceID_ACK?  It's a bit shorter, though
I don't feel strongly about it.

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

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:145: // If the BrowserPlugin
has never navigated then the browser process, and
nit: no comma

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:325: instance_id_ =
instance_id;
Perhaps we should CHECK() that it isn't INSTANCE_ID_NONE.

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager.h (right):

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager.h:41:
BrowserPluginManager(RenderViewImpl* render_view);
While we're at it, this should have "explicit" (one arg constructor).

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:70:
PickleIterator iter(message);
This is usually a red flag.  Seems like we should be able to get by with the
normal message handling, without having to parse it ourselves.

Why does BrowserPlugin need a message handler for this?  I can't think of a case
it would be receiving the message directly.  Couldn't it just have a normal
method that takes in the instance_id and request_id from this handler?

[Fady Samuel, 2013-01-18 02:23:10]

PTAL

https://codereview.chromium.org/11956022/diff/9001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11956022/diff/9001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:310: if
(params.src.empty())
On 2013/01/18 01:54:31, creis wrote:
> Nice.  Can you update the comment in the .h file to say that this will
navigate
> the guest if the params has a src?
> 
> (Also, I'm fine with it as is, but is there a reason you put the check here
> instead of inside CreateGuest?)

Not really, I guess. window.open uses CreateGuest too, but it does the
navigation from the guest process, instead of here. However, we can simply keep
the src parameter empty for the window.open case so we don't do an extra
navigation request.

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

https://codereview.chromium.org/11956022/diff/9001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:152: int
instance_id_counter_;
On 2013/01/18 01:54:31, creis wrote:
> Should we be more consistent with the name above, like next_instance_id_?

Done.

https://codereview.chromium.org/11956022/diff/9001/content/common/browser_plu...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11956022/diff/9001/content/common/browser_plu...
content/common/browser_plugin_messages.h:128:
IPC_MESSAGE_ROUTED1(BrowserPluginHostMsg_AllocateInstanceIDRequest,
On 2013/01/18 01:54:31, creis wrote:
> We have a lot of similar Foo/Foo_ACK message pairs in the code, and the ACK
> isn't always empty (e.g., ViewMsg_AsyncOpenFile_ACK).  Perhaps these should be
> named AllocateInstanceID and AllocateInstanceID_ACK?  It's a bit shorter,
though
> I don't feel strongly about it.

Done.

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

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:145: // If the BrowserPlugin
has never navigated then the browser process, and
On 2013/01/18 01:54:31, creis wrote:
> nit: no comma

Done.

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:325: instance_id_ =
instance_id;
On 2013/01/18 01:54:31, creis wrote:
> Perhaps we should CHECK() that it isn't INSTANCE_ID_NONE.

Done.

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager.h (right):

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager.h:41:
BrowserPluginManager(RenderViewImpl* render_view);
On 2013/01/18 01:54:31, creis wrote:
> While we're at it, this should have "explicit" (one arg constructor).

Done.

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:70:
PickleIterator iter(message);
On 2013/01/18 01:54:31, creis wrote:
> This is usually a red flag.  Seems like we should be able to get by with the
> normal message handling, without having to parse it ourselves.
> 
> Why does BrowserPlugin need a message handler for this?  I can't think of a
case
> it would be receiving the message directly.  Couldn't it just have a normal
> method that takes in the instance_id and request_id from this handler?

Fixed. Using the same technique Istiaque used below.

[Charlie Reis, 2013-01-18 05:36:45]

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/11956022/diff/9001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:70:
PickleIterator iter(message);
On 2013/01/18 02:23:10, Fady Samuel wrote:
> Fixed. Using the same technique Istiaque used below.

Sure, that's better.  But why is it a message handler in BrowserPlugin? 
BrowserPlugin doesn't even need the request ID.  If the message always goes
through here, shouldn't we just have BrowserPlugin::SetInstanceID(instance_id)
or BrowserPlugin::Init(instance_id) instead?

https://codereview.chromium.org/11956022/diff/32001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/11956022/diff/32001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.h:63: // |routing_id|.
Please update this comment to say that it does the navigation as well if
params.src is present.  (I think that got missed in my last round of comments.)

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:325: instance_id_ =
instance_id;
This should set allocate_instance_id_in_flight_ to false, right?  (That or
change in_flight_ to sent_.)

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:246: void
OnAllocateInstanceIDACK(int request_id, int instance_id);
I think this should be SetInstanceID(instance_id) or Init(instance_id), rather
than being a message handler.

[Fady Samuel, 2013-01-18 15:26:25]

PTAL

https://codereview.chromium.org/11956022/diff/32001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/11956022/diff/32001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.h:63: // |routing_id|.
On 2013/01/18 05:36:45, creis wrote:
> Please update this comment to say that it does the navigation as well if
> params.src is present.  (I think that got missed in my last round of
comments.)

Done.

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:325: instance_id_ =
instance_id;
On 2013/01/18 05:36:45, creis wrote:
> This should set allocate_instance_id_in_flight_ to false, right?  (That or
> change in_flight_ to sent_.)

Changed to allocate_instance_id_sent_. There's really no need at this point to
reset this bool.

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:246: void
OnAllocateInstanceIDACK(int request_id, int instance_id);
On 2013/01/18 05:36:45, creis wrote:
> I think this should be SetInstanceID(instance_id) or Init(instance_id), rather
> than being a message handler.


Fair enough. I had originally made it a message to allow BrowserPluginObservers
to see this message and perhaps do additional initialization when/if they need
to, but since BrowserPluginObservers have not yet landed and there is no
particular use case where we might want to add more initialization state (yet),
I'll switch this to SetInstanceID.

[Fady Samuel, 2013-01-18 15:26:25]

PTAL

https://codereview.chromium.org/11956022/diff/32001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/11956022/diff/32001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.h:63: // |routing_id|.
On 2013/01/18 05:36:45, creis wrote:
> Please update this comment to say that it does the navigation as well if
> params.src is present.  (I think that got missed in my last round of
comments.)

Done.

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:325: instance_id_ =
instance_id;
On 2013/01/18 05:36:45, creis wrote:
> This should set allocate_instance_id_in_flight_ to false, right?  (That or
> change in_flight_ to sent_.)

Changed to allocate_instance_id_sent_. There's really no need at this point to
reset this bool.

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11956022/diff/32001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:246: void
OnAllocateInstanceIDACK(int request_id, int instance_id);
On 2013/01/18 05:36:45, creis wrote:
> I think this should be SetInstanceID(instance_id) or Init(instance_id), rather
> than being a message handler.


Fair enough. I had originally made it a message to allow BrowserPluginObservers
to see this message and perhaps do additional initialization when/if they need
to, but since BrowserPluginObservers have not yet landed and there is no
particular use case where we might want to add more initialization state (yet),
I'll switch this to SetInstanceID.

[Charlie Reis, 2013-01-18 17:24:28]

Thanks, LGTM.

https://codereview.chromium.org/11956022/diff/41001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11956022/diff/41001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:346: 
nit: No need for second extra line here.

[Fady Samuel, 2013-01-18 17:27:13]

Thanks.

https://codereview.chromium.org/11956022/diff/41001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11956022/diff/41001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:346: 
On 2013/01/18 17:24:28, creis wrote:
> nit: No need for second extra line here.

Done.

[Fady Samuel, 2013-01-18 17:40:22]

+cdn@ for browser_plugin_messages security audit.

[Cris Neckar, 2013-01-22 20:40:02]

IPC security audit LGTM

[commit-bot: I haz the power, 2013-01-22 20:41:47]

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

[commit-bot: I haz the power, 2013-01-23 00:07:09]

Change committed as 178167