Browser Plugin: Implement BrowserPluginObserver.

Browser Plugin: Implement BrowserPluginObserver.

The major changes in this CL are:
- Refactors BrowserPlugin to BrowserPlugin in content/public and BrowserPluginImpl.
- Refactors BrowserPluginBindings, moving out method and property bindings to content/public.
- Introduces BrowserPluginObserver, and adds an observer list to BrowserPluginImpl.
- Messages sent to BrowserPlugin from outside are encapsulated in another message that takes care of the instance ID routing BrowserPlugin*Msg_ForwardMessage.

BUG=166165
Test=BrowserPluginHostTest.*, BrowserPluginTest.*

[Fady Samuel, 2013-01-08 23:50:13]

+jam@ for content/public changes.

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

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
File content/public/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin.h:20: virtual void
AddMethodBinding(BrowserPluginMethodBinding* method_binding) = 0;
doc the methods, and comment on ownership/lifetime for the method/property
bindings

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
File content/public/renderer/browser_plugin/browser_plugin_observer.cc (right):

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.cc:16: if
(browser_plugin) {
Once a BPO is created, it doesn't look like |browser_plugin_| can be changed at
a later time. If that is the case, should it ever be created with a null
browser-plugin? i.e. should CHECK here for a non-null browser-plugin?

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.cc:51:
browser_plugin_ = NULL;
Or perhaps allow setting the |browser_plugin_| of an observer (e.g. if an
observer overrides OnDestruct, and wants to be reused as an observer)?

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
File content/public/renderer/browser_plugin/browser_plugin_observer.h (right):

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.h:18: class
CONTENT_EXPORT BrowserPluginObserver : public IPC::Listener,
It looks like a BPO can be an observer for a single browser-plugin at a time? I
think in chromium code, some kind of observers can observe any number of items
(aura::WindowObserver), while some observers can observer only a single item at
a time (content::RenderViewObserver). So this is OK (although I prefer the
former version of observers). But please add doc.

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.h:36: int
instance_id() { return instance_id_; }
const this too?

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.h:50: 
remove blank line

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
File content/public/renderer/browser_plugin/browser_plugin_property_binding.h
(right):

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_property_binding.h:23:
virtual bool MatchesName(NPIdentifier name) const = 0;
Do you need both GetName and MatchesName? Perhaps use GetName from where you use
MatchesName instead?

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_bindings.cc:384:
static_cast<BrowserPluginImpl*>(browser_plugin)->TerminateGuest();
no-op comment: All these static_casts seem a bit awkward. But I can't think of
anything better either.

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_bindings.h:52: void
AddMethodBinding(BrowserPluginMethodBinding* method_binding);
Comment on ownership/lifetime

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.cc:155:
ObserverListBase<BrowserPluginObserver>::Iterator it(observers_);
Check observers_.might_have_observers() first.

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.h:125: 
remove newline

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.h:130: virtual bool
Send(IPC::Message* message) OVERRIDE;
It might make more sense to group the IPC::Sender override seperately, under a
'IPC::Sender implementation.' comment (this approach is followed in the ui code,
but I am not sure about non-ui code)

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.h:181: // A BrowserPlugin
object is a controller that represents an instance of a
Update comment

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.h:195: // Add and remove
observers for filtering IPC messages.  Clients must be sure
single space after the dot

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.h:331: // A list of
observers that filter messages.  Weak references.
ditto

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager.cc:81: case
BrowserPluginMsg_UpdateRect::ID:
Can this default set of messages be added to browser_plugin_messages_ instead
(in the constructor, for example)?

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager.h:58: void
RequestMessage(uint32 message_id);
doc

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager.h:75: 
too many new lines

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager.h:88: };
You should have a DISALLOW_COPY_AND_ASSIGN inside a private block here.

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:9: #include
"content/public/renderer/content_renderer_client.h"
These includes aren't used?

[sadrul, 2013-01-09 15:23:53]

Also, you could change the CL description (to something like:

The major changes in this CL are:
 * Refactor ...
 * Refactor ...
 * Introduce ...
 * BrowserPlugin exposes ...

A subsequent patch will use these to implement executeScript)

[Fady Samuel, 2013-01-09 17:41:24]

PTAL

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
File content/public/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin.h:20: virtual void
AddMethodBinding(BrowserPluginMethodBinding* method_binding) = 0;
On 2013/01/09 15:21:54, sadrul wrote:
> doc the methods, and comment on ownership/lifetime for the method/property
> bindings

Done.

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
File content/public/renderer/browser_plugin/browser_plugin_observer.cc (right):

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.cc:16: if
(browser_plugin) {
On 2013/01/09 15:21:54, sadrul wrote:
> Once a BPO is created, it doesn't look like |browser_plugin_| can be changed
at
> a later time. If that is the case, should it ever be created with a null
> browser-plugin? i.e. should CHECK here for a non-null browser-plugin?

It should never be null. Changed to a CHECK.

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.cc:51:
browser_plugin_ = NULL;
On 2013/01/09 15:21:54, sadrul wrote:
> Or perhaps allow setting the |browser_plugin_| of an observer (e.g. if an
> observer overrides OnDestruct, and wants to be reused as an observer)?

I'd rather not, a BrowserPluginObserver can create
BrowserPluginMethod/PropertyBindings whose lifetime is tied to a particular
BrowserPlugin and refer back to a particular BrowserPluginObserver. I'm worried
that if we make it possible for an observer to change owner BrowserPlugins,
lifetime management might get confusing. I'd prefer to leave this as is, and add
additional functionality when necessary.

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
File content/public/renderer/browser_plugin/browser_plugin_observer.h (right):

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.h:18: class
CONTENT_EXPORT BrowserPluginObserver : public IPC::Listener,
On 2013/01/09 15:21:54, sadrul wrote:
> It looks like a BPO can be an observer for a single browser-plugin at a time?
I
> think in chromium code, some kind of observers can observe any number of items
> (aura::WindowObserver), while some observers can observer only a single item
at
> a time (content::RenderViewObserver). So this is OK (although I prefer the
> former version of observers). But please add doc.

Done.

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.h:36: int
instance_id() { return instance_id_; }
On 2013/01/09 15:21:54, sadrul wrote:
> const this too?

Done.

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.h:50: 
On 2013/01/09 15:21:54, sadrul wrote:
> remove blank line

Done.

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
File content/public/renderer/browser_plugin/browser_plugin_property_binding.h
(right):

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_property_binding.h:23:
virtual bool MatchesName(NPIdentifier name) const = 0;
On 2013/01/09 15:21:54, sadrul wrote:
> Do you need both GetName and MatchesName? Perhaps use GetName from where you
use
> MatchesName instead?

Done.

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_bindings.cc:384:
static_cast<BrowserPluginImpl*>(browser_plugin)->TerminateGuest();
On 2013/01/09 15:21:54, sadrul wrote:
> no-op comment: All these static_casts seem a bit awkward. But I can't think of
> anything better either.

Sigh, neither can I :-(

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_bindings.h:52: void
AddMethodBinding(BrowserPluginMethodBinding* method_binding);
On 2013/01/09 15:21:54, sadrul wrote:
> Comment on ownership/lifetime

Done.

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.cc:155:
ObserverListBase<BrowserPluginObserver>::Iterator it(observers_);
On 2013/01/09 15:21:54, sadrul wrote:
> Check observers_.might_have_observers() first.

Done.

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.h:125: 
On 2013/01/09 15:21:54, sadrul wrote:
> remove newline

Done.

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.h:130: virtual bool
Send(IPC::Message* message) OVERRIDE;
On 2013/01/09 15:21:54, sadrul wrote:
> It might make more sense to group the IPC::Sender override seperately, under a
> 'IPC::Sender implementation.' comment (this approach is followed in the ui
code,
> but I am not sure about non-ui code)

Done.

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.h:181: // A BrowserPlugin
object is a controller that represents an instance of a
On 2013/01/09 15:21:54, sadrul wrote:
> Update comment

Done.

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.h:195: // Add and remove
observers for filtering IPC messages.  Clients must be sure
On 2013/01/09 15:21:54, sadrul wrote:
> single space after the dot

Done.

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_impl.h:331: // A list of
observers that filter messages.  Weak references.
On 2013/01/09 15:21:54, sadrul wrote:
> ditto

Done.

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager.cc:81: case
BrowserPluginMsg_UpdateRect::ID:
On 2013/01/09 15:21:54, sadrul wrote:
> Can this default set of messages be added to browser_plugin_messages_ instead
> (in the constructor, for example)?

Done.

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager.h:58: void
RequestMessage(uint32 message_id);
On 2013/01/09 15:21:54, sadrul wrote:
> doc

Done.

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager.h:75: 
On 2013/01/09 15:21:54, sadrul wrote:
> too many new lines

Done.

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager.h:88: };
On 2013/01/09 15:21:54, sadrul wrote:
> You should have a DISALLOW_COPY_AND_ASSIGN inside a private block here.

Done.

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

https://codereview.chromium.org/11826005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:9: #include
"content/public/renderer/content_renderer_client.h"
On 2013/01/09 15:21:54, sadrul wrote:
> These includes aren't used?

Done.

[sadrul, 2013-01-09 17:58:07]

LGTM

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
File content/public/renderer/browser_plugin/browser_plugin_observer.cc (right):

https://codereview.chromium.org/11826005/diff/1/content/public/renderer/brows...
content/public/renderer/browser_plugin/browser_plugin_observer.cc:51:
browser_plugin_ = NULL;
On 2013/01/09 17:41:24, Fady Samuel wrote:
> On 2013/01/09 15:21:54, sadrul wrote:
> > Or perhaps allow setting the |browser_plugin_| of an observer (e.g. if an
> > observer overrides OnDestruct, and wants to be reused as an observer)?
> 
> I'd rather not, a BrowserPluginObserver can create
> BrowserPluginMethod/PropertyBindings whose lifetime is tied to a particular
> BrowserPlugin and refer back to a particular BrowserPluginObserver. I'm
worried
> that if we make it possible for an observer to change owner BrowserPlugins,
> lifetime management might get confusing. I'd prefer to leave this as is, and
add
> additional functionality when necessary.

Sounds good.

[commit-bot: I haz the power, 2013-01-09 17:59:46]

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

[commit-bot: I haz the power, 2013-01-09 17:59:56]

Presubmit check for 11826005-6002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content
    content/public
    content/renderer

Presubmit checks took 2.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[jam, 2013-01-09 19:10:03]

I need to find a block of time to understand all this. my first impression is
can you simplify the public parts? i.e. why do these bindings interfaces need to
be there? can they be folded into the registration part or do they really need
to be dynamic?

[Fady Samuel, 2013-01-09 19:16:07]

On 2013/01/09 19:10:03, John Abd-El-Malek wrote:
> I need to find a block of time to understand all this. my first impression is
> can you simplify the public parts? i.e. why do these bindings interfaces need
to
> be there? can they be folded into the registration part or do they really need
> to be dynamic?

Hmm, perhaps argcount and name can be moved into the registration process, but
the other stuff might access state outside of the core BrowserPluginImpl, and so
it needs custom implementations.

[jam, 2013-01-09 22:07:12]

On 2013/01/09 19:16:07, Fady Samuel wrote:
> On 2013/01/09 19:10:03, John Abd-El-Malek wrote:
> > I need to find a block of time to understand all this. my first impression
is
> > can you simplify the public parts? i.e. why do these bindings interfaces
need
> to
> > be there? can they be folded into the registration part or do they really
need
> > to be dynamic?
> 
> Hmm, perhaps argcount and name can be moved into the registration process, but
> the other stuff might access state outside of the core BrowserPluginImpl, and
so
> it needs custom implementations.

It's a bit hard for me to see this because I don't see the chrome side
implementation. Can you point me to what that looks like? or add it to this
change?

also re RequestMessage, usually the model used here is that the observer object
is an IPC::Listener which can filter messages itself.

[Fady Samuel, 2013-01-09 22:10:48]

On 2013/01/09 22:07:12, John Abd-El-Malek wrote:
> On 2013/01/09 19:16:07, Fady Samuel wrote:
> > On 2013/01/09 19:10:03, John Abd-El-Malek wrote:
> > > I need to find a block of time to understand all this. my first impression
> is
> > > can you simplify the public parts? i.e. why do these bindings interfaces
> need
> > to
> > > be there? can they be folded into the registration part or do they really
> need
> > > to be dynamic?
> > 
> > Hmm, perhaps argcount and name can be moved into the registration process,
but
> > the other stuff might access state outside of the core BrowserPluginImpl,
and
> so
> > it needs custom implementations.
> 
> It's a bit hard for me to see this because I don't see the chrome side
> implementation. Can you point me to what that looks like? or add it to this
> change?
> 
> also re RequestMessage, usually the model used here is that the observer
object
> is an IPC::Listener which can filter messages itself.

This code is not quite ready for review but here is a link:

https://codereview.chromium.org/11693009/patch/1/11

[Fady Samuel, 2013-01-09 23:02:53]

On 2013/01/09 22:10:48, Fady Samuel wrote:
> On 2013/01/09 22:07:12, John Abd-El-Malek wrote:
> > On 2013/01/09 19:16:07, Fady Samuel wrote:
> > > On 2013/01/09 19:10:03, John Abd-El-Malek wrote:
> > > > I need to find a block of time to understand all this. my first
impression
> > is
> > > > can you simplify the public parts? i.e. why do these bindings interfaces
> > need
> > > to
> > > > be there? can they be folded into the registration part or do they
really
> > need
> > > > to be dynamic?
> > > 
> > > Hmm, perhaps argcount and name can be moved into the registration process,
> but
> > > the other stuff might access state outside of the core BrowserPluginImpl,
> and
> > so
> > > it needs custom implementations.
> > 
> > It's a bit hard for me to see this because I don't see the chrome side
> > implementation. Can you point me to what that looks like? or add it to this
> > change?
> > 
> > also re RequestMessage, usually the model used here is that the observer
> object
> > is an IPC::Listener which can filter messages itself.
> 
> This code is not quite ready for review but here is a link:
> 
> https://codereview.chromium.org/11693009/patch/1/11

The purpose of RequestMessage has to do with the way we read and route messages.

For BrowserPlugin, routing happens based on the first parameter in the payload,
the |instance_id|. We can't read the instance_id of ALL messages because not all
messages have that parameter, because they're not all browser_plugin messages.
Thus, we add BrowserPlugin message types to a set. If they're in the set then we
know we can read the first parameter, |instance_id| and route based on that
parameter.

[Fady Samuel, 2013-01-09 23:02:53]

On 2013/01/09 22:10:48, Fady Samuel wrote:
> On 2013/01/09 22:07:12, John Abd-El-Malek wrote:
> > On 2013/01/09 19:16:07, Fady Samuel wrote:
> > > On 2013/01/09 19:10:03, John Abd-El-Malek wrote:
> > > > I need to find a block of time to understand all this. my first
impression
> > is
> > > > can you simplify the public parts? i.e. why do these bindings interfaces
> > need
> > > to
> > > > be there? can they be folded into the registration part or do they
really
> > need
> > > > to be dynamic?
> > > 
> > > Hmm, perhaps argcount and name can be moved into the registration process,
> but
> > > the other stuff might access state outside of the core BrowserPluginImpl,
> and
> > so
> > > it needs custom implementations.
> > 
> > It's a bit hard for me to see this because I don't see the chrome side
> > implementation. Can you point me to what that looks like? or add it to this
> > change?
> > 
> > also re RequestMessage, usually the model used here is that the observer
> object
> > is an IPC::Listener which can filter messages itself.
> 
> This code is not quite ready for review but here is a link:
> 
> https://codereview.chromium.org/11693009/patch/1/11

The purpose of RequestMessage has to do with the way we read and route messages.

For BrowserPlugin, routing happens based on the first parameter in the payload,
the |instance_id|. We can't read the instance_id of ALL messages because not all
messages have that parameter, because they're not all browser_plugin messages.
Thus, we add BrowserPlugin message types to a set. If they're in the set then we
know we can read the first parameter, |instance_id| and route based on that
parameter.

[Fady Samuel, 2013-01-11 16:44:11]

After our chat John, I've removed the need for RequestMessage inside observers.
I wrap all messages from observers in either
BrowserPluginMsg_ForwardMessage/BrowserPluginHostMsg_ForwardMessage. Thanks for
your help there! This is very cool! Observers don't seem to need to worry about
instance_ids anymore!

I've added mpcomplete@ in CC for this review. We will be chatting later today
about executeScript, and injection of bindings from content embedders.

[Fady Samuel, 2013-02-21 20:10:12]

+creis for reference after a VC conversation:

This is the embedder-side refactor that will permit drop in replacement APIs for
the BrowserPlugin core functionality.

[Fady Samuel, 2013-02-26 20:31:45]

+creis [-everyone else]:

Hi Charlie,

This is the first step towards moving out all the API in content to chrome.

This is the embedder-side BrowserPluginObserver. Once we fully implement
observers in both here and the browser process, we can begin moving the API into
the chrome layer.

Thanks,

Fady

[Fady Samuel, 2013-02-26 22:43:46]

+lazyboy@ for a look at this as well.

[Charlie Reis, 2013-02-28 02:12:03]

I'm very happy about where this is headed.  It will be nice to move the API
implementations into the chrome module.

https://codereview.chromium.org/11826005/diff/37026/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11826005/diff/37026/content/common/browser_pl...
content/common/browser_plugin_messages.h:136: // BrowserPluginGuestObservers.
I'm not sure if this is ok from a security perspective, but I suppose you'll
need a security reviewer for this file anyway.

Why do you need to forward an entire IPC message?  What's an example?

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
File content/public/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
2013 for new files.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin.h:28: class CONTENT_EXPORT
BrowserPlugin : public IPC::Sender {
Please add class-level documentation for this concept.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin.h:46: virtual RenderView*
GetRenderView() const = 0;
GetRenderView and GetContainer seem like a different category than the others. 
Can you move them above AddMethodBinding?

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
File content/public/renderer/browser_plugin/browser_plugin_method_binding.h
(right):

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin_method_binding.h:17: class
CONTENT_EXPORT BrowserPluginMethodBinding {
Class-level documentation.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
File content/public/renderer/browser_plugin/browser_plugin_observer.h (right):

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin_observer.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
2013 for new files.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin_observer.h:18: // Base
class for objects taht want to filter incoming IPCs.
nit: that

I'm a bit surprised that this is about IPC messages.  I was expecting something
more like RenderViewObserver, which exposes semantic events.  Then again, I'm
not sure how you're expecting to use this.

I'll defer to John's opinion on this, though.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
File content/public/renderer/browser_plugin/browser_plugin_property_binding.h
(right):

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin_property_binding.h:17:
class CONTENT_EXPORT BrowserPluginPropertyBinding {
Class-level documentation.

[Fady Samuel, 2013-02-28 22:56:21]

Addressed comments

+jam for thoughts

https://codereview.chromium.org/11826005/diff/37026/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11826005/diff/37026/content/common/browser_pl...
content/common/browser_plugin_messages.h:136: // BrowserPluginGuestObservers.
On 2013/02/28 02:12:03, creis wrote:
> I'm not sure if this is ok from a security perspective, but I suppose you'll
> need a security reviewer for this file anyway.
> 
> Why do you need to forward an entire IPC message?  What's an example?

See https://chromiumcodereview.appspot.com/12326168/

This idea was suggested to me by John. I believe this is done elsewhere in the
code. This hides the notion of instance ID from the chrome layer and
BrowserPluginObserver. Messages automagically get to the right place without
needing to know anything about how they're routed there.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
File content/public/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2013/02/28 02:12:03, creis wrote:
> 2013 for new files.

Done.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin.h:28: class CONTENT_EXPORT
BrowserPlugin : public IPC::Sender {
On 2013/02/28 02:12:03, creis wrote:
> Please add class-level documentation for this concept.

Done.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin.h:46: virtual RenderView*
GetRenderView() const = 0;
On 2013/02/28 02:12:03, creis wrote:
> GetRenderView and GetContainer seem like a different category than the others.

> Can you move them above AddMethodBinding?

Done.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
File content/public/renderer/browser_plugin/browser_plugin_method_binding.h
(right):

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin_method_binding.h:17: class
CONTENT_EXPORT BrowserPluginMethodBinding {
On 2013/02/28 02:12:03, creis wrote:
> Class-level documentation.

Done.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
File content/public/renderer/browser_plugin/browser_plugin_observer.h (right):

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin_observer.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/02/28 02:12:03, creis wrote:
> 2013 for new files.

Done.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin_observer.h:18: // Base
class for objects taht want to filter incoming IPCs.
On 2013/02/28 02:12:03, creis wrote:
> nit: that
> 
> I'm a bit surprised that this is about IPC messages.  I was expecting
something
> more like RenderViewObserver, which exposes semantic events.  Then again, I'm
> not sure how you're expecting to use this.
> 
> I'll defer to John's opinion on this, though.

Fixed nit.

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
File content/public/renderer/browser_plugin/browser_plugin_property_binding.h
(right):

https://codereview.chromium.org/11826005/diff/37026/content/public/renderer/b...
content/public/renderer/browser_plugin/browser_plugin_property_binding.h:17:
class CONTENT_EXPORT BrowserPluginPropertyBinding {
On 2013/02/28 02:12:03, creis wrote:
> Class-level documentation.

Done.