Bind plugin placeholder directly to v8 instead of over CppBoundClass

Bind plugin placeholder directly to v8 instead of over CppBoundClass

BUG=297480
R=bauerb@chromium.org, svenpanne@chromium.org
TBR=svenpanne@chromium.org

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

[jochen (gone - plz use gerrit), 2013-11-12 16:29:07]

plz review

[Bernhard Bauer, 2013-11-12 16:37:19]

https://codereview.chromium.org/69953006/diff/1/chrome/renderer/plugins/chrom...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/69953006/diff/1/chrome/renderer/plugins/chrom...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:219: void
ChromePluginPlaceholder::OpenAboutPluginsCallback(
If this is a static method now, you can move it to an anonymous namespace (same
goes for the other private static methods below).

https://codereview.chromium.org/69953006/diff/1/components/plugins/renderer/m...
File components/plugins/renderer/mobile_youtube_plugin.cc (right):

https://codereview.chromium.org/69953006/diff/1/components/plugins/renderer/m...
components/plugins/renderer/mobile_youtube_plugin.cc:97: const
v8::FunctionCallbackInfo<v8::Value>& args) {
Move to anonymous namespace

https://codereview.chromium.org/69953006/diff/1/components/plugins/renderer/p...
File components/plugins/renderer/plugin_placeholder.h (right):

https://codereview.chromium.org/69953006/diff/1/components/plugins/renderer/p...
components/plugins/renderer/plugin_placeholder.h:48: virtual void
InstallAdditionalCallbacks(v8::Handle<v8::Template> prototype);
Super-nit: From the description of the method it seems you can install arbitrary
properties on the window.plugin object, but the method name just mentions
callbacks. Update one or the other?

https://codereview.chromium.org/69953006/diff/60001/components/plugins/render...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/69953006/diff/60001/components/plugins/render...
components/plugins/renderer/plugin_placeholder.cc:232: PluginPlaceholder* plugin
= reinterpret_cast<PluginPlaceholder*>(
This reinterpret_cast business looks kinda messy. Do you think you could extract
this into a shared base class (templatized) that can register itself for
callbacks?

Also, if we can register arbitrary base::Closures, we could bind them with weak
pointers to make sure that the object is still alive when we call the method on
it. I don't think we have that guarantee now, do we?

[jochen (gone - plz use gerrit), 2013-11-14 15:59:23]

PTAL

WeakPtrs in one direction, weak handles in the other direction, simplified API
for derived classes.

[Bernhard Bauer, 2013-11-14 16:25:43]

Nice! LGTM with some nits, and a question:

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.cc:47: V8ClosureWrapper(const
base::Closure& closure) : closure_(closure) {}
Nit: add `explicit` to constructor

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.cc:150: new
V8ClosureWrapper(base::Bind(&PluginPlaceholder::InternalObserve,
Out of curiosity, why do you do this instead of having the subclass directly
register an arbitrary callback?

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
File components/plugins/renderer/plugin_placeholder.h (right):

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.h:51: // on the window.plugin
object. When the callbacks are invoked, the Observe()
Nit: This sentence reads a bit weird. How about, "When a callback is invoked, it
will be forwarded to Observe()"?

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.h:55: virtual void Observe(const
std::string& callback);
Nit: add a comment

[jochen (gone - plz use gerrit), 2013-11-15 10:09:39]

TBR'ing sven because of the stupid DEPS rule.

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.cc:47: V8ClosureWrapper(const
base::Closure& closure) : closure_(closure) {}
On 2013/11/14 16:25:43, Bernhard Bauer wrote:
> Nit: add `explicit` to constructor

Done.

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.cc:150: new
V8ClosureWrapper(base::Bind(&PluginPlaceholder::InternalObserve,
On 2013/11/14 16:25:43, Bernhard Bauer wrote:
> Out of curiosity, why do you do this instead of having the subclass directly
> register an arbitrary callback?

It strikes me as a safer to use API

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
File components/plugins/renderer/plugin_placeholder.h (right):

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.h:51: // on the window.plugin
object. When the callbacks are invoked, the Observe()
On 2013/11/14 16:25:43, Bernhard Bauer wrote:
> Nit: This sentence reads a bit weird. How about, "When a callback is invoked,
it
> will be forwarded to Observe()"?

Done.

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.h:55: virtual void Observe(const
std::string& callback);
On 2013/11/14 16:25:43, Bernhard Bauer wrote:
> Nit: add a comment

Done.

[commit-bot: I haz the power, 2013-11-15 10:10:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/69953006/310001

[Sven Panne, 2013-11-15 10:12:41]

LGTM (although I have no clue about the details :-)

[Bernhard Bauer, 2013-11-15 10:24:03]

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.cc:150: new
V8ClosureWrapper(base::Bind(&PluginPlaceholder::InternalObserve,
On 2013/11/15 10:09:39, jochen wrote:
> On 2013/11/14 16:25:43, Bernhard Bauer wrote:
> > Out of curiosity, why do you do this instead of having the subclass directly
> > register an arbitrary callback?
> 
> It strikes me as a safer to use API

Why is that? It means that the subclass has to find out which callback was
called, which is somewhat prone to typos if you compare the callback name to a
string constant.

[jochen (gone - plz use gerrit), 2013-11-15 11:55:32]

On 2013/11/15 10:24:03, Bernhard Bauer wrote:
>
https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
> File components/plugins/renderer/plugin_placeholder.cc (right):
> 
>
https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
> components/plugins/renderer/plugin_placeholder.cc:150: new
> V8ClosureWrapper(base::Bind(&PluginPlaceholder::InternalObserve,
> On 2013/11/15 10:09:39, jochen wrote:
> > On 2013/11/14 16:25:43, Bernhard Bauer wrote:
> > > Out of curiosity, why do you do this instead of having the subclass
directly
> > > register an arbitrary callback?
> > 
> > It strikes me as a safer to use API
> 
> Why is that? It means that the subclass has to find out which callback was
> called, which is somewhat prone to typos if you compare the callback name to a
> string constant.

That's way better than having it screw up the entire v8 state

[Bernhard Bauer, 2013-11-15 12:02:47]

On Fri, Nov 15, 2013 at 11:55 AM, <jochen@chromium.org> wrote:

> On 2013/11/15 10:24:03, Bernhard Bauer wrote:
>
> https://codereview.chromium.org/69953006/diff/210001/
> components/plugins/renderer/plugin_placeholder.cc
>
>> File components/plugins/renderer/plugin_placeholder.cc (right):
>>
>
>
> https://codereview.chromium.org/69953006/diff/210001/
> components/plugins/renderer/plugin_placeholder.cc#newcode150
>
>> components/plugins/renderer/plugin_placeholder.cc:150: new
>> V8ClosureWrapper(base::Bind(&PluginPlaceholder::InternalObserve,
>> On 2013/11/15 10:09:39, jochen wrote:
>> > On 2013/11/14 16:25:43, Bernhard Bauer wrote:
>> > > Out of curiosity, why do you do this instead of having the subclass
>>
> directly
>
>> > > register an arbitrary callback?
>> >
>> > It strikes me as a safer to use API
>>
>
>  Why is that? It means that the subclass has to find out which callback was
>> called, which is somewhat prone to typos if you compare the callback name
>> to a
>> string constant.
>>
>
> That's way better than having it screw up the entire v8 state
>

Yes, I was not talking about exposing the v8 state to subclasses, but
letting them choose the callback that is stored instead of the one you're
binding in line 150.


> https://codereview.chromium.org/69953006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2013-11-15 12:03:51]

On 2013/11/15 11:55:32, jochen wrote:
> On 2013/11/15 10:24:03, Bernhard Bauer wrote:
> >
>
https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
> > File components/plugins/renderer/plugin_placeholder.cc (right):
> > 
> >
>
https://codereview.chromium.org/69953006/diff/210001/components/plugins/rende...
> > components/plugins/renderer/plugin_placeholder.cc:150: new
> > V8ClosureWrapper(base::Bind(&PluginPlaceholder::InternalObserve,
> > On 2013/11/15 10:09:39, jochen wrote:
> > > On 2013/11/14 16:25:43, Bernhard Bauer wrote:
> > > > Out of curiosity, why do you do this instead of having the subclass
> directly
> > > > register an arbitrary callback?
> > > 
> > > It strikes me as a safer to use API
> > 
> > Why is that? It means that the subclass has to find out which callback was
> > called, which is somewhat prone to typos if you compare the callback name to
a
> > string constant.
> 
> That's way better than having it screw up the entire v8 state

ah, you mean a base::Callback. makes sense

[jochen (gone - plz use gerrit), 2013-11-15 12:37:14]

PTAL

[Bernhard Bauer, 2013-11-15 12:42:36]

LGTM, thanks!

[commit-bot: I haz the power, 2013-11-15 12:44:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/69953006/310002

[commit-bot: I haz the power, 2013-11-15 13:29:50]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-11-15 14:05:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/69953006/710001

[jochen (gone - plz use gerrit), 2013-11-15 16:01:20]

Committed patchset #6 manually as r235336 (presubmit successful).