Move the plugin placeholder from CppBoundClass to gin::Wrappable

Move the plugin placeholder from CppBoundClass to gin::Wrappable

BUG=297480
R=aa@chromium.org,bauerb@chromium.org

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

[jochen (gone - plz use gerrit), 2013-12-18 14:45:46]

this is still somewhat WIP, but I wanted to get some first comments.

Aaron, any idea how to solve this in a nicer way? Wrappable<> doesn't really
support inheritance...

[Bernhard Bauer, 2013-12-18 15:31:04]

Could you extract the gin::Wrappable into a separate object that calls back to
PluginPlaceholder?

https://codereview.chromium.org/116163008/diff/1/components/plugins/renderer/...
File components/plugins/renderer/webview_plugin.cc (left):

https://codereview.chromium.org/116163008/diff/1/components/plugins/renderer/...
components/plugins/renderer/webview_plugin.cc:105:
delegate_->WillDestroyPlugin();
How does the delegate get destroyed now?

[Aaron Boodman, 2013-12-18 18:23:53]

Hm, I think this works nicer if we make GetObjectTemplate(Builder?) virtual
after all. Since it is only called the first time the object is needed now, I
think it is fine to be virtual.

I am hacking it up to see if it works.

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

https://codereview.chromium.org/116163008/diff/1/components/plugins/renderer/...
components/plugins/renderer/plugin_placeholder.h:74: static
gin::ObjectTemplateBuilder& GetObjectTemplateBuilder(
Hm, maybe we should just make the contract in Wrappable be
GetObjectTemplateBuilder() so that these can conveniently call each other. The
base class could be the one to finally call Build().

[jochen (gone - plz use gerrit), 2013-12-18 18:29:56]

Another problem is that the base class doesn't have a WrapperInfo so we
can't safely convert the holder to a PluginPlaceholder object - see my
custom FromV8 which conveniently ships the type check.

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

[Aaron Boodman, 2013-12-18 19:12:17]

I'm not sure how to test this, but it at least links.

https://codereview.chromium.org/116163008/diff/20001/gin/wrappable.cc
File gin/wrappable.cc (left):

https://codereview.chromium.org/116163008/diff/20001/gin/wrappable.cc#oldcode41
gin/wrappable.cc:41: v8::Handle<v8::Object> WrappableBase::CreateWrapper(
There was no caller for CreateWrapper other than GetWrapperImpl, so I just
combined them.

https://codereview.chromium.org/116163008/diff/20001/gin/wrappable.h
File gin/wrappable.h (left):

https://codereview.chromium.org/116163008/diff/20001/gin/wrappable.h#oldcode110
gin/wrappable.h:110: base::is_convertible<T*, Wrappable<T>*>::value>::type> {
This was making it so that only direct descendants of WrappableBase could be
converted, because in the case of:

Bar : Foo : Wrappable<Foo> : WrappableBase

Wrappable<Bar> is undefined

[jochen (gone - plz use gerrit), 2013-12-18 19:15:12]

nice

You can test by enabling click to play in content settings and going to
youtube.com

it shouldn't crash (and videos should play when you click them)

[Aaron Boodman, 2013-12-18 19:47:06]

I added a unittest for wrapping, unwrapping and calling methods on multiple
levels of inheritance from Wrappable<T>. I am pretty sure your plugin thing
should work, but I can't trigger it on Linux using your instructions (I get
something about needing to install flash instead).

Can you take the patch from here?

[jochen (gone - plz use gerrit), 2013-12-19 13:28:29]

gin::Wrappable know owns the placeholder. It'll disappear as soon as it's GC'd
(which will only happen after the WebFrame was detroyed).

https://codereview.chromium.org/116163008/diff/60001/chrome/renderer/plugins/...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/116163008/diff/60001/chrome/renderer/plugins/...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:269: if (!GetFrame() ||
!plugin())
GetFrame() never returned NULL previously.

However, if we destroyed our old plugin, we can't replace it anymore

[jochen (gone - plz use gerrit), 2013-12-19 13:51:17]

and yes, I could pull out a helper class that is gin::Wrappable<> and have that
keep a weak pointer to the placeholder, but that's more or less ruining the idea
behind gin::Wrappable<> and how js works.

[Bernhard Bauer, 2014-01-07 09:35:39]

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

https://codereview.chromium.org/116163008/diff/60001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.cc:88: // (which will destroy
us).
I think this comment needs updating now.

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

https://codereview.chromium.org/116163008/diff/60001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.h:25: public
gin::Wrappable<PluginPlaceholder> {
Nit: I like listing the "real" superclass first (i.e. the one that actually
contains code and controls the lifetime, whereas the other ones are just
interfaces).

https://codereview.chromium.org/116163008/diff/60001/components/plugins/rende...
File components/plugins/renderer/webview_plugin.h (right):

https://codereview.chromium.org/116163008/diff/60001/components/plugins/rende...
components/plugins/renderer/webview_plugin.h:136: // Destroys itself.
I think this comment refers to |delegate_| and somehow moved down a line. It's
also not quite accurate anymore (okay, technically the delegate does destroy
itself, but it's garbage-collected).

Also, could you insert blank lines before the comments here so it's clear in the
future what they refer to?

[jochen (gone - plz use gerrit), 2014-01-08 19:18:23]

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

https://codereview.chromium.org/116163008/diff/60001/components/plugins/rende...
components/plugins/renderer/plugin_placeholder.cc:88: // (which will destroy
us).
On 2014/01/07 09:35:42, Bernhard Bauer wrote:
> I think this comment needs updating now.

Done.

https://codereview.chromium.org/116163008/diff/60001/components/plugins/rende...
File components/plugins/renderer/webview_plugin.h (right):

https://codereview.chromium.org/116163008/diff/60001/components/plugins/rende...
components/plugins/renderer/webview_plugin.h:136: // Destroys itself.
On 2014/01/07 09:35:42, Bernhard Bauer wrote:
> I think this comment refers to |delegate_| and somehow moved down a line. It's
> also not quite accurate anymore (okay, technically the delegate does destroy
> itself, but it's garbage-collected).
> 
> Also, could you insert blank lines before the comments here so it's clear in
the
> future what they refer to?

Done.

[Bernhard Bauer, 2014-01-08 19:56:02]

lgtm

[commit-bot: I haz the power, 2014-01-08 20:01:36]

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

[commit-bot: I haz the power, 2014-01-08 22:55:25]

Change committed as 243682