browser-plugin: Refactor the code for binding methods on the plugin.

browser-plugin: Refactor the code for binding methods on the plugin.

The hope for this refactor is that adding new bindings will be
easier since the code will be isolated in one place. Stylewise,
this is consistent with other parts of chrome too (chrome
extension functions are somewhat similarly defined, for example).

BUG=none

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

[sadrul, 2012-10-12 00:48:59]

Hi! This is a refactoring CL that ends up increasing the line-count. But I think
this makes the code a bit easier to read and maintain (e.g. it should be easier
to figure out how to add a new binding etc).

I am not overly attached to this change though, so feel free to disagree :)

[Charlie Reis, 2012-10-12 03:57:58]

On 2012/10/12 00:48:59, sadrul wrote:
> Hi! This is a refactoring CL that ends up increasing the line-count. But I
think
> this makes the code a bit easier to read and maintain (e.g. it should be
easier
> to figure out how to add a new binding etc).
> 
> I am not overly attached to this change though, so feel free to disagree :)

(I'm OOO until Monday.  I can review it when I get back unless you find someone
to take a look before then.)

[Charlie Reis, 2012-10-15 22:19:33]

I like it!  Just a few nits.

http://codereview.chromium.org/11116003/diff/4004/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

http://codereview.chromium.org/11116003/diff/4004/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_bindings.cc:94: }
nit: Why move these?  Nice that a lot of this was removed, but the move seems
like unnecessary code churn.

http://codereview.chromium.org/11116003/diff/4004/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_bindings.cc:233: class
BrowserPluginBindingInterface {
nit: Interface suggests a class with only pure virtual methods.  Maybe
BrowserPluginMethodBinding, since they all appear to be methods?

http://codereview.chromium.org/11116003/diff/4004/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_bindings.cc:249: virtual bool
Invoked(BrowserPluginBindings* bindings,
Why past tense?  Invoke seems clearer to me.

http://codereview.chromium.org/11116003/diff/4004/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_bindings.cc:511: iter !=
method_bindings_.end();
nit: wrong indent.  (Should have one more space, to line up with BindingsList.)
Same below in InvokeMethod.

[sadrul, 2012-10-15 22:37:12]

http://codereview.chromium.org/11116003/diff/4004/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

http://codereview.chromium.org/11116003/diff/4004/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_bindings.cc:94: }
On 2012/10/15 22:19:33, creis wrote:
> nit: Why move these?  Nice that a lot of this was removed, but the move seems
> like unnecessary code churn.

Done.

http://codereview.chromium.org/11116003/diff/4004/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_bindings.cc:233: class
BrowserPluginBindingInterface {
On 2012/10/15 22:19:33, creis wrote:
> nit: Interface suggests a class with only pure virtual methods.  Maybe
> BrowserPluginMethodBinding, since they all appear to be methods?

Good point. Done.

http://codereview.chromium.org/11116003/diff/4004/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_bindings.cc:249: virtual bool
Invoked(BrowserPluginBindings* bindings,
On 2012/10/15 22:19:33, creis wrote:
> Why past tense?  Invoke seems clearer to me.

Done.

http://codereview.chromium.org/11116003/diff/4004/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_bindings.cc:511: iter !=
method_bindings_.end();
On 2012/10/15 22:19:33, creis wrote:
> nit: wrong indent.  (Should have one more space, to line up with
BindingsList.)
> Same below in InvokeMethod.

Done.

[Charlie Reis, 2012-10-15 23:34:19]

LGTM.  Probably worthwhile to have Fady take a look as well, since this will
affect a lot of his in-flight CLs.

[sadrul, 2012-10-15 23:38:41]

On 2012/10/15 23:34:19, creis wrote:
> LGTM.  Probably worthwhile to have Fady take a look as well, since this will
> affect a lot of his in-flight CLs.

Looks like http://codereview.chromium.org/10829225/ introduces a contentWindow
property, but doesn't look like any of the CLs introduce a new method. But I
will wait for his review.

[Fady Samuel, 2012-10-16 00:06:35]

Sadrul and I discussed this CL in person the other day. It's great! Thanks!
LGTM.