Browser Plugin: Reduce code repetition in BrowserPluginManager.

Browser Plugin: Reduce code repetition in BrowserPluginManager.

Move the bulk of message handling in BrowserPluginmanager to BrowserPlugin.

This is not just code simplification. This change will enable a more extensible
BrowserPlugin by allowing observers to intercept messages on specific instances
of BrowserPlugin and perform custom behavior or simply add more behavior
to specific instances of BrowserPlugin.

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

[Fady Samuel, 2012-12-14 00:01:46]

No change in functionality yet. This is simply a standalone code simplification
with the intent of using it to enable BrowserPluginObservers and specifically a
ChromeBrowserPluginObserver in the future to allow for Chrome-layer only API in
BrowserPlugin. This change may also be useful for OOP iframes.

[sadrul, 2012-12-14 16:08:39]

Looks good. A few comments:

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

https://codereview.chromium.org/11565024/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:411: // Construct the loadStop
event object.
This comment doesn't seem relevant anymore.

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

https://codereview.chromium.org/11565024/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.h:34: public IPC::Listener {
The plugin doesn't need to be an IPC::Listener, right? From what I can see, it
receives messages only from the BrowserPluginManager. So it doesn't actually
need to be a listener?

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

https://codereview.chromium.org/11565024/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:160: new
BrowserPluginMsg_UpdateRect(0, instance_id, 0, update_rect_params));
Are these messages leaking? Can you just create the messages on the stack?

https://codereview.chromium.org/11565024/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:160: new
BrowserPluginMsg_UpdateRect(0, instance_id, 0, update_rect_params));
I see that the messages aren't leaking because the mock plugin deletes these.
But if these messages can be on the stack, that would probably be better.

[Fady Samuel, 2012-12-14 17:05:54]

Addressed your comments sadrul. I've snuck in a small change in
BrowserPluginManager, where instance_ids now start at 1, instead of 0. This will
be needed for window.open, and I thought I'd make this innocuous change now
while I'm in this code. :-)

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

https://codereview.chromium.org/11565024/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:411: // Construct the loadStop
event object.
On 2012/12/14 16:08:39, sadrul wrote:
> This comment doesn't seem relevant anymore.

Done.

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

https://codereview.chromium.org/11565024/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.h:34: public IPC::Listener {
On 2012/12/14 16:08:39, sadrul wrote:
> The plugin doesn't need to be an IPC::Listener, right? From what I can see, it
> receives messages only from the BrowserPluginManager. So it doesn't actually
> need to be a listener?

It does not and I don't have immediate plans to pass it in to methods that
expect an IPC::Listener. Removed. Thanks!

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

https://codereview.chromium.org/11565024/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:160: new
BrowserPluginMsg_UpdateRect(0, instance_id, 0, update_rect_params));
On 2012/12/14 16:08:39, sadrul wrote:
> I see that the messages aren't leaking because the mock plugin deletes these.
> But if these messages can be on the stack, that would probably be better.

Done.

[sadrul, 2012-12-14 17:09:41]

Cool. LGTM

[Charlie Reis, 2012-12-14 18:24:02]

I like where this is heading, but the IPC stuff worries me.  Do you think we can
go a little further and eliminate the need for BrowserPluginManager entirely?

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

https://codereview.chromium.org/11565024/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:40: if
(browser_plugin_messages_.count(message.type()) > 0) {
This feels like a mis-use of the IPC logic.  All the macros are designed to
route appropriate messages to the right instances, so it would be better if we
didn't go outside the system.

Is it possible to use "routed" IPC messages with routing_ids to accomplish this
same thing?

And do we even need BrowserPluginManager anymore if we go that route?

[Fady Samuel, 2012-12-14 18:47:48]

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

https://codereview.chromium.org/11565024/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:40: if
(browser_plugin_messages_.count(message.type()) > 0) {
On 2012/12/14 18:24:02, creis wrote:
> This feels like a mis-use of the IPC logic.  All the macros are designed to
> route appropriate messages to the right instances, so it would be better if we
> didn't go outside the system.
> 
> Is it possible to use "routed" IPC messages with routing_ids to accomplish
this
> same thing?
> 
> And do we even need BrowserPluginManager anymore if we go that route?

Regardless of the direction we go, BrowserPluginManager is still useful for
handling "control" messages that aren't directed at a particular BrowserPlugin:
see  OnPluginAtPositionRequest.

window.open CL also does some work in BrowserPluginManager that doesn't fit into
a particular BrowserPlugin: OnNewWindow.

We need to allocate route IDs from somewhere. My concern is what you're
suggesting will require some non-trivial changes to RenderThreadImpl and browser
process side changes (RenderProcessHostImpl::OnMessageReceived assumes a route
ID is a handle to a particular RenderWidgetHost)to ensure that a RenderView
doesn't end up with the same route ID as a BrowserPlugin.

Also, with the introduction of window.open, some instance_ids are allocated by
the browser process (negative IDs) and some by the BrowserPluginManager
(positive IDs). All route IDs are currently allocated by the browser process
IIRC (RenderWidgetHelper::GetNextRoutingID).

We have a BrowserPluginManager per RenderViewImpl, so BrowserPluginMsg_* are
routed to a particular BrowserPluginManager, which, from there, routes on
instance_id to a particular BrowserPlugin.

What your suggesting is certainly possible, but definitely not an easy task to
get right. Are you OK with my landing this as is?

[Charlie Reis, 2012-12-14 19:16:36]

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

https://codereview.chromium.org/11565024/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:40: if
(browser_plugin_messages_.count(message.type()) > 0) {
On 2012/12/14 18:47:48, Fady Samuel wrote:
> Regardless of the direction we go, BrowserPluginManager is still useful for
> handling "control" messages that aren't directed at a particular
BrowserPlugin:
> see  OnPluginAtPositionRequest.

Perhaps the class is still needed.
 
> window.open CL also does some work in BrowserPluginManager that doesn't fit
into
> a particular BrowserPlugin: OnNewWindow.
> 
> We need to allocate route IDs from somewhere. My concern is what you're
> suggesting will require some non-trivial changes to RenderThreadImpl and
browser
> process side changes (RenderProcessHostImpl::OnMessageReceived assumes a route
> ID is a handle to a particular RenderWidgetHost)to ensure that a RenderView
> doesn't end up with the same route ID as a BrowserPlugin.

No, I was picturing a different ID space than RenderWidgets, and thus not
routing these through RenderThreadImpl.  Perhaps that's something that can be
discussed separately, including someone like John.

> Also, with the introduction of window.open, some instance_ids are allocated by
> the browser process (negative IDs) and some by the BrowserPluginManager
> (positive IDs). All route IDs are currently allocated by the browser process
> IIRC (RenderWidgetHelper::GetNextRoutingID).

Negative IDs do not sound OK to me, but that's a topic for a different CL.

> We have a BrowserPluginManager per RenderViewImpl, so BrowserPluginMsg_* are
> routed to a particular BrowserPluginManager, which, from there, routes on
> instance_id to a particular BrowserPlugin.
> 
> What your suggesting is certainly possible, but definitely not an easy task to
> get right. Are you OK with my landing this as is?

We can skip the larger change here, but it's worth a discussion for later.

https://codereview.chromium.org/11565024/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:42:
PickleIterator iter(message);
Please document that all messages that we allow here must have instance_id as
their first parameter.

Also, we should get a security team reviewer on this code, as if it were in a
_messages.h file.

https://codereview.chromium.org/11565024/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:87: void
BrowserPluginManagerImpl::RegisterBrowserPluginMessages() {
Instead of holding a set, just use a switch statement, like:
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/swapped_ou...

[Fady Samuel, 2012-12-14 19:34:08]

+cdn@, Cris, could you please take a look at
BrowserPluginManagerImpl::OnMessageReceived. We're routing on the first
parameter of the message body.

These messages are coming from the browser process.

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

https://codereview.chromium.org/11565024/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:42:
PickleIterator iter(message);
On 2012/12/14 19:16:36, creis wrote:
> Please document that all messages that we allow here must have instance_id as
> their first parameter.
> 
> Also, we should get a security team reviewer on this code, as if it were in a
> _messages.h file.

Done.

https://codereview.chromium.org/11565024/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:87: void
BrowserPluginManagerImpl::RegisterBrowserPluginMessages() {
On 2012/12/14 19:16:36, creis wrote:
> Instead of holding a set, just use a switch statement, like:
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/swapped_ou...

I made this a set rather than a static switch statement because the set of
messages we're interested in might grow if a BrowserPluginObserver (coming in a
subsequent CL) requests more message types.

You can imagine a ChromeBrowserPlugin/ChromeBrowserPluginObserver that requests
the receipt of

ChromeBrowserPluginMsg_* messages.

[Charlie Reis, 2012-12-14 19:40:32]

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

https://codereview.chromium.org/11565024/diff/8001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:87: void
BrowserPluginManagerImpl::RegisterBrowserPluginMessages() {
On 2012/12/14 19:34:08, Fady Samuel wrote:
> On 2012/12/14 19:16:36, creis wrote:
> > Instead of holding a set, just use a switch statement, like:
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/swapped_ou...
> 
> I made this a set rather than a static switch statement because the set of
> messages we're interested in might grow if a BrowserPluginObserver (coming in
a
> subsequent CL) requests more message types.
> 
> You can imagine a ChromeBrowserPlugin/ChromeBrowserPluginObserver that
requests
> the receipt of
> 
> ChromeBrowserPluginMsg_* messages. 

That doesn't imply that you need a set at runtime, though. 
SwappedOutMessages::CanHandleWhileSwappedOut calls
ContentClient::CanHandleWhileSwappedOut to let the embedder provide more
supported message types.

[Fady Samuel, 2012-12-14 19:53:37]

On 2012/12/14 19:40:32, creis wrote:
>
https://codereview.chromium.org/11565024/diff/8001/content/renderer/browser_p...
> File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/11565024/diff/8001/content/renderer/browser_p...
> content/renderer/browser_plugin/browser_plugin_manager_impl.cc:87: void
> BrowserPluginManagerImpl::RegisterBrowserPluginMessages() {
> On 2012/12/14 19:34:08, Fady Samuel wrote:
> > On 2012/12/14 19:16:36, creis wrote:
> > > Instead of holding a set, just use a switch statement, like:
> > >
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/swapped_ou...
> > 
> > I made this a set rather than a static switch statement because the set of
> > messages we're interested in might grow if a BrowserPluginObserver (coming
in
> a
> > subsequent CL) requests more message types.
> > 
> > You can imagine a ChromeBrowserPlugin/ChromeBrowserPluginObserver that
> requests
> > the receipt of
> > 
> > ChromeBrowserPluginMsg_* messages. 
> 
> That doesn't imply that you need a set at runtime, though. 
> SwappedOutMessages::CanHandleWhileSwappedOut calls
> ContentClient::CanHandleWhileSwappedOut to let the embedder provide more
> supported message types.

Good point! Done. Thanks!

[Charlie Reis, 2012-12-14 20:51:28]

Thanks.  LGTM pending Cris's review.

[Cris Neckar, 2012-12-14 21:44:43]

Doesn't appear to introduce unpriv->priv ipcs so lgtm

[commit-bot: I haz the power, 2012-12-14 21:47:18]

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

[commit-bot: I haz the power, 2012-12-15 02:25:36]

Retried try job too often on win_rel for step(s) browser_tests