Track NPObject ownership by the originating plugins' NPP identifier. [2/3] (Chrome)

Track NPObject ownership by the originating plugins' NPP identifier. [2/3] (Chrome)

This CL updates Chrome to return plugin NPP identifiers for NPAPI, PPAPI and browser plugins, and to make the necessary calls into Blink to support object ownership tracking.

This CL requires Blink CL crrev.com/14989014, and is itself required by Blink CL crrev.com/14019005.

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

[Wez, 2013-05-13 03:01:21]

Chrome side of the NPObject ownership tracking patch.

[abarth-chromium, 2013-05-13 18:18:28]

I'm not an OWNER for any of this code, so I'm not sure whether I should review
this part of the CL.

[Wez, 2013-05-13 18:41:52]

John, could you take a look at this, please?

Adam, leaving you CC'd, since this is effectively part of the Blink-side CL.

[Wez, 2013-05-15 22:25:47]

Sorry guys, hard to find one reviewer for this particular combination of files.
:-/

For context see crbug.com/152006.

jamesr: webkit/*
bauerb: chrome/renderer/plugins/*
fsamuel: content/renderer/browser_plugin/*
estade: components/autofill/*

Thanks!

[jamesr, 2013-05-15 22:32:15]

https://codereview.chromium.org/15007012/diff/15001/webkit/plugins/npapi/webp...
File webkit/plugins/npapi/webplugin_impl.cc (right):

https://codereview.chromium.org/15007012/diff/15001/webkit/plugins/npapi/webp...
webkit/plugins/npapi/webplugin_impl.cc:293: return npp_; //
delegate_->GetPluginNPP();
hmm? what's up with the commented-out code here?

https://codereview.chromium.org/15007012/diff/15001/webkit/plugins/ppapi/ppap...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

https://codereview.chromium.org/15007012/diff/15001/webkit/plugins/ppapi/ppap...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:153: // calls will contain. WebKit
will treat it as opaque, though, so it needn't
if it's opaque anyway, can we fix the type to not be an NPAPI type?

https://codereview.chromium.org/15007012/diff/15001/webkit/plugins/ppapi/ppap...
File webkit/plugins/ppapi/ppapi_webplugin_impl.h (right):

https://codereview.chromium.org/15007012/diff/15001/webkit/plugins/ppapi/ppap...
webkit/plugins/ppapi/ppapi_webplugin_impl.h:47: virtual NPP pluginNPP();
Yuck - the ppapi bindings shouldn't depend on npapi at all.  Can you refactor
this in a way that NPAPI-agnostic?

https://codereview.chromium.org/15007012/diff/15001/webkit/support/webkit_sup...
File webkit/support/webkit_support.gypi (right):

https://codereview.chromium.org/15007012/diff/15001/webkit/support/webkit_sup...
webkit/support/webkit_support.gypi:24:
'<(DEPTH)/third_party/npapi/npapi.gyp:npapi',
why does webkit_support need to depend on npapi?

[Wez, 2013-05-15 23:14:16]

https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/npa...
File webkit/plugins/npapi/webplugin_impl.cc (right):

https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/npa...
webkit/plugins/npapi/webplugin_impl.cc:293: return npp_; //
delegate_->GetPluginNPP();
On 2013/05/15 22:32:15, jamesr wrote:
> hmm? what's up with the commented-out code here?

Sorry, looks like this patch-set is stale. :(

https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:153: // calls will contain. WebKit
will treat it as opaque, though, so it needn't
On 2013/05/15 22:32:15, jamesr wrote:
> if it's opaque anyway, can we fix the type to not be an NPAPI type?

There's not really any point in doing that right now (see scriptableObject()
above), but yes in principle this could be simply void*, or a dedicated
WebPluginId typedef, for example.

https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppapi_webplugin_impl.h (right):

https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
webkit/plugins/ppapi/ppapi_webplugin_impl.h:47: virtual NPP pluginNPP();
On 2013/05/15 22:32:15, jamesr wrote:
> Yuck - the ppapi bindings shouldn't depend on npapi at all.  Can you refactor
> this in a way that NPAPI-agnostic?

The PPAPI bindings already depend on NPAPI for scriptableObject, so this isn't
adding a new dependency as such.

Right now PPAPI still has legacy support for synchronous-scriptable plugins,
IIRC. Once that goes away these APIs can be made to return NULL or moved to e.g.
an WebPluginWithNPAPI interface.

Technically it's possible for pluginNPP() to return void*, since the calling
code never looks inside the NPP, but that ends up introducing messy re-interpret
casts and doesn't help with the NPObject dependency on the line above.

https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/support/web...
File webkit/support/webkit_support.gypi (right):

https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/support/web...
webkit/support/webkit_support.gypi:24:
'<(DEPTH)/third_party/npapi/npapi.gyp:npapi',
On 2013/05/15 22:32:15, jamesr wrote:
> why does webkit_support need to depend on npapi?

It pulls in WebFrame.h, IIRC, which in turn will need to pull in npruntime.h,
since NPP can't be forward-defined. :(

[jamesr, 2013-05-15 23:18:38]

On 2013/05/15 23:14:16, Wez wrote:
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/support/web...
> webkit/support/webkit_support.gypi:24:
> '<(DEPTH)/third_party/npapi/npapi.gyp:npapi',
> On 2013/05/15 22:32:15, jamesr wrote:
> > why does webkit_support need to depend on npapi?
> 
> It pulls in WebFrame.h, IIRC, which in turn will need to pull in npruntime.h,
> since NPP can't be forward-defined. :(

NPP is a typedef for NPP_t*.  Can't you forward declare the struct NPP_t and
then have a pointer to that?

Pulling the npapi headers in has lots of very surprising side effects - for
instance on mac it pulls in the OpenGL headers (because I'm lazy and haven't
fixed that yet), so I'm very skittish about pulling it in to more places if we
can in any way avoid it.

[Fady Samuel, 2013-05-15 23:21:15]

https://chromiumcodereview.appspot.com/15007012/diff/15001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/15007012/diff/15001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:1140: // Tell |container| to
allow this plugin to use script objects.
Does webview.contentWindow work without this workaround?
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...

[Wez, 2013-05-15 23:22:03]

On 2013/05/15 23:18:38, jamesr wrote:
> On 2013/05/15 23:14:16, Wez wrote:
> >
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/support/web...
> > webkit/support/webkit_support.gypi:24:
> > '<(DEPTH)/third_party/npapi/npapi.gyp:npapi',
> > On 2013/05/15 22:32:15, jamesr wrote:
> > > why does webkit_support need to depend on npapi?
> > 
> > It pulls in WebFrame.h, IIRC, which in turn will need to pull in
npruntime.h,
> > since NPP can't be forward-defined. :(
> 
> NPP is a typedef for NPP_t*.  Can't you forward declare the struct NPP_t and
> then have a pointer to that?
> 
> Pulling the npapi headers in has lots of very surprising side effects - for
> instance on mac it pulls in the OpenGL headers (because I'm lazy and haven't
> fixed that yet), so I'm very skittish about pulling it in to more places if we
> can in any way avoid it.

I'm fine with switching to using struct NPP_t* for this, so long as Adam is
amenable - the risk is simply that it's not strictly a "public" name for that
type.

[jamesr, 2013-05-15 23:39:24]

On 2013/05/15 23:21:15, Fady Samuel wrote:
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/content/renderer/b...
> File content/renderer/browser_plugin/browser_plugin.cc (right):
> 
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/content/renderer/b...
> content/renderer/browser_plugin/browser_plugin.cc:1140: // Tell |container| to
> allow this plugin to use script objects.
> Does webview.contentWindow work without this workaround?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...

What does webview.contentWindow do?

[Fady Samuel, 2013-05-15 23:48:35]

On 2013/05/15 23:39:24, jamesr wrote:
> On 2013/05/15 23:21:15, Fady Samuel wrote:
> >
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/content/renderer/b...
> > File content/renderer/browser_plugin/browser_plugin.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/content/renderer/b...
> > content/renderer/browser_plugin/browser_plugin.cc:1140: // Tell |container|
to
> > allow this plugin to use script objects.
> > Does webview.contentWindow work without this workaround?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
> 
> What does webview.contentWindow do?

webview.contentWindow returns the DOMWindow of the swapped out RenderView for
the guest's RenderView in the embedder's render process.

This permits cross-process postMessage, and close() and other operations (focus,
etc although these aren't fully supported yet).

[Bernhard Bauer, 2013-05-16 09:26:55]

https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppapi_webplugin_impl.h (right):

https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
webkit/plugins/ppapi/ppapi_webplugin_impl.h:47: virtual NPP pluginNPP();
On 2013/05/15 23:14:16, Wez wrote:
> On 2013/05/15 22:32:15, jamesr wrote:
> > Yuck - the ppapi bindings shouldn't depend on npapi at all.  Can you
refactor
> > this in a way that NPAPI-agnostic?
> 
> The PPAPI bindings already depend on NPAPI for scriptableObject, so this isn't
> adding a new dependency as such.
> 
> Right now PPAPI still has legacy support for synchronous-scriptable plugins,
> IIRC. Once that goes away these APIs can be made to return NULL or moved to
e.g.
> an WebPluginWithNPAPI interface.
> 
> Technically it's possible for pluginNPP() to return void*, since the calling
> code never looks inside the NPP, but that ends up introducing messy
re-interpret
> casts and doesn't help with the NPObject dependency on the line above.

But you have reinterpret casts right now too!

I would prefer a NPAPI-neutral return value too (and maybe a neutral method name
as well?).

[Wez, 2013-05-16 18:00:53]

On 2013/05/16 09:26:55, Bernhard Bauer wrote:
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
> File webkit/plugins/ppapi/ppapi_webplugin_impl.h (right):
> 
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
> webkit/plugins/ppapi/ppapi_webplugin_impl.h:47: virtual NPP pluginNPP();
> On 2013/05/15 23:14:16, Wez wrote:
> > On 2013/05/15 22:32:15, jamesr wrote:
> > > Yuck - the ppapi bindings shouldn't depend on npapi at all.  Can you
> refactor
> > > this in a way that NPAPI-agnostic?
> > 
> > The PPAPI bindings already depend on NPAPI for scriptableObject, so this
isn't
> > adding a new dependency as such.
> > 
> > Right now PPAPI still has legacy support for synchronous-scriptable plugins,
> > IIRC. Once that goes away these APIs can be made to return NULL or moved to
> e.g.
> > an WebPluginWithNPAPI interface.
> > 
> > Technically it's possible for pluginNPP() to return void*, since the calling
> > code never looks inside the NPP, but that ends up introducing messy
> re-interpret
> > casts and doesn't help with the NPObject dependency on the line above.
> 
> But you have reinterpret casts right now too!

Yes, there are two of those. Using e.g. void* instead of NPP requires casts in
many more places.

> I would prefer a NPAPI-neutral return value too (and maybe a neutral method
name
> as well?).

I don't think that makes sense, since the value is only used to track NPObject
ownership, so is inherently tied to NPAPI, and since the value *must* be the
same as the NPP value passed when calling to NPAPI bindings (e.g.
_NPN_CreateObject).

[Wez, 2013-05-16 18:27:16]

https://chromiumcodereview.appspot.com/15007012/diff/15001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/15007012/diff/15001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:1140: // Tell |container| to
allow this plugin to use script objects.
On 2013/05/15 23:21:15, Fady Samuel wrote:
> Does webview.contentWindow work without this workaround?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...

It will do once patch [3/3] has landed in Blink, yes.

[Bernhard Bauer, 2013-05-17 13:09:51]

On 2013/05/16 18:00:53, Wez wrote:
> On 2013/05/16 09:26:55, Bernhard Bauer wrote:
> >
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
> > File webkit/plugins/ppapi/ppapi_webplugin_impl.h (right):
> >
> >
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
> > webkit/plugins/ppapi/ppapi_webplugin_impl.h:47: virtual NPP pluginNPP();
> > On 2013/05/15 23:14:16, Wez wrote:
> > > On 2013/05/15 22:32:15, jamesr wrote:
> > > > Yuck - the ppapi bindings shouldn't depend on npapi at all.  Can you
> > refactor
> > > > this in a way that NPAPI-agnostic?
> > >
> > > The PPAPI bindings already depend on NPAPI for scriptableObject, so
this
> isn't
> > > adding a new dependency as such.
> > >
> > > Right now PPAPI still has legacy support for synchronous-scriptable
plugins,
> > > IIRC. Once that goes away these APIs can be made to return NULL or
moved to
> > e.g.
> > > an WebPluginWithNPAPI interface.
> > >
> > > Technically it's possible for pluginNPP() to return void*, since the
calling
> > > code never looks inside the NPP, but that ends up introducing messy
> > re-interpret
> > > casts and doesn't help with the NPObject dependency on the line above.
> >
> > But you have reinterpret casts right now too!
 >
> Yes, there are two of those. Using e.g. void* instead of NPP requires
casts in
> many more places.

Really? The only references I could find are in ScriptController.cpp and in
browser_plugin_bindings.cc.

But okay, if the others are okay with NPP_t* as the return type, I'm not
going to block it, so LGTM.

> > I would prefer a NPAPI-neutral return value too (and maybe a neutral
method
> name
> > as well?).
>
> I don't think that makes sense, since the value is only used to track
NPObject
> ownership, so is inherently tied to NPAPI, and since the value *must* be
the
> same as the NPP value passed when calling to NPAPI bindings (e.g.
> _NPN_CreateObject).

[Wez, 2013-05-17 16:52:34]

All of the NPAPI bindings (see WebBindings.h in Blink) take NPP values as
parameters, having plugins return void* means casts wherever code is taking
the plugin-supplied Id value and passing it to a binding.  As I said, in
principle we can do that, but I'd prefer not to do it in this CL.


On 17 May 2013 06:09, Bernhard Bauer <bauerb@chromium.org> wrote:

> On 2013/05/16 18:00:53, Wez wrote:
> > On 2013/05/16 09:26:55, Bernhard Bauer wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
> > > File webkit/plugins/ppapi/ppapi_webplugin_impl.h (right):
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15007012/diff/15001/webkit/plugins/ppa...
> > > webkit/plugins/ppapi/ppapi_webplugin_impl.h:47: virtual NPP
> pluginNPP();
> > > On 2013/05/15 23:14:16, Wez wrote:
> > > > On 2013/05/15 22:32:15, jamesr wrote:
> > > > > Yuck - the ppapi bindings shouldn't depend on npapi at all.  Can
> you
> > > refactor
> > > > > this in a way that NPAPI-agnostic?
> > > >
> > > > The PPAPI bindings already depend on NPAPI for scriptableObject, so
> this
> > isn't
> > > > adding a new dependency as such.
> > > >
> > > > Right now PPAPI still has legacy support for synchronous-scriptable
> plugins,
> > > > IIRC. Once that goes away these APIs can be made to return NULL or
> moved to
> > > e.g.
> > > > an WebPluginWithNPAPI interface.
> > > >
> > > > Technically it's possible for pluginNPP() to return void*, since the
> calling
> > > > code never looks inside the NPP, but that ends up introducing messy
> > > re-interpret
> > > > casts and doesn't help with the NPObject dependency on the line
> above.
> > >
> > > But you have reinterpret casts right now too!
>  >
> > Yes, there are two of those. Using e.g. void* instead of NPP requires
> casts in
> > many more places.
>
> Really? The only references I could find are in ScriptController.cpp and
> in browser_plugin_bindings.cc.
>
> But okay, if the others are okay with NPP_t* as the return type, I'm not
> going to block it, so LGTM.
>
> > > I would prefer a NPAPI-neutral return value too (and maybe a neutral
> method
> > name
> > > as well?).
> >
> > I don't think that makes sense, since the value is only used to track
> NPObject
> > ownership, so is inherently tied to NPAPI, and since the value *must* be
> the
> > same as the NPP value passed when calling to NPAPI bindings (e.g.
> > _NPN_CreateObject).
>
>

[Wez, 2013-05-18 06:59:09]

+darin

[jamesr, 2013-05-20 23:15:29]

On 2013/05/17 16:52:34, Wez wrote:
> All of the NPAPI bindings (see WebBindings.h in Blink) take NPP values as
> parameters, having plugins return void* means casts wherever code is taking
> the plugin-supplied Id value and passing it to a binding.  As I said, in
> principle we can do that, but I'd prefer not to do it in this CL.
> 

What do the WebBindings entry points have to do with the type of the owner
value?  In other words, if you consider the owner an opaque handle of
indeterminate type, when is it important that it be treated as an NPP instead of
just an opaque thing that can be compared for equality against itself and
(possibly) NPP instances?

[Wez, 2013-05-21 01:01:39]

On 2013/05/20 23:15:29, jamesr wrote:
> On 2013/05/17 16:52:34, Wez wrote:
> > All of the NPAPI bindings (see WebBindings.h in Blink) take NPP values as
> > parameters, having plugins return void* means casts wherever code is taking
> > the plugin-supplied Id value and passing it to a binding.  As I said, in
> > principle we can do that, but I'd prefer not to do it in this CL.
> > 
> 
> What do the WebBindings entry points have to do with the type of the owner
> value?  In other words, if you consider the owner an opaque handle of
> indeterminate type, when is it important that it be treated as an NPP instead
of
> just an opaque thing that can be compared for equality against itself and
> (possibly) NPP instances?

It's just tested for equality.

Your objection seems to be that this adds a tie between WebPlugin and NPAPI.
Firstly, WebPlugin is already tied to NPAPI (see scriptableObject()). Secondly,
this identifier is only necessary for plugins that generate NPObjects. So we can
in future move scriptableObject() and this API off WebPlugin to a
WebPluginScriptable, for example, to make a "clean" WebPlugin.

The downsides to using an opaque identifier are:
1. It makes the link between that and the NPP parameter to WebBindings less
obvious.
2. When I originally drafted the Blink-side CL that way, it ended up requiring
numerous re-interpret casts.
3. It requires defining a new NPAPI-agnotic opaque plugin identifier type that
will only ever be used with NPAPI, which seems unnecessary.

If you still object to using struct _NPP here then I'd prefer to land this CL,
since this is a high-priority fix to Blink (see the bug), and follow-up with the
API change separately.

[jamesr, 2013-05-21 17:43:13]

On 2013/05/21 01:01:39, Wez wrote:
> On 2013/05/20 23:15:29, jamesr wrote:
> > On 2013/05/17 16:52:34, Wez wrote:
> > > All of the NPAPI bindings (see WebBindings.h in Blink) take NPP values as
> > > parameters, having plugins return void* means casts wherever code is
taking
> > > the plugin-supplied Id value and passing it to a binding.  As I said, in
> > > principle we can do that, but I'd prefer not to do it in this CL.
> > > 
> > 
> > What do the WebBindings entry points have to do with the type of the owner
> > value?  In other words, if you consider the owner an opaque handle of
> > indeterminate type, when is it important that it be treated as an NPP
instead
> of
> > just an opaque thing that can be compared for equality against itself and
> > (possibly) NPP instances?
> 
> It's just tested for equality.
> 
> Your objection seems to be that this adds a tie between WebPlugin and NPAPI.
> Firstly, WebPlugin is already tied to NPAPI (see scriptableObject()).
Secondly,
> this identifier is only necessary for plugins that generate NPObjects. So we
can
> in future move scriptableObject() and this API off WebPlugin to a
> WebPluginScriptable, for example, to make a "clean" WebPlugin.
> 
> The downsides to using an opaque identifier are:
> 1. It makes the link between that and the NPP parameter to WebBindings less
> obvious.
> 2. When I originally drafted the Blink-side CL that way, it ended up requiring
> numerous re-interpret casts.

Where?  Do you need to reinterpret_cast<> to use operator== between a void* and
an NPP (which is a NNP_t*) ?


> 3. It requires defining a new NPAPI-agnotic opaque plugin identifier type that
> will only ever be used with NPAPI, which seems unnecessary.

But you're using it for PPAPI as well.

> 
> If you still object to using struct _NPP here then I'd prefer to land this CL,
> since this is a high-priority fix to Blink (see the bug), and follow-up with
the
> API change separately.

[jamesr, 2013-05-21 18:20:21]

https://codereview.chromium.org/15007012/diff/38001/chrome/renderer/plugins/w...
File chrome/renderer/plugins/webview_plugin.cc (right):

https://codereview.chromium.org/15007012/diff/38001/chrome/renderer/plugins/w...
chrome/renderer/plugins/webview_plugin.cc:118: return NULL;
how is this OK?

https://codereview.chromium.org/15007012/diff/38001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/15007012/diff/38001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:1215: return
reinterpret_cast<NPP>(this);
nak, if you're going to leave the type NPP_t*, return a pointer to an actual
NPP_t

https://codereview.chromium.org/15007012/diff/38001/webkit/glue/cpp_bound_cla...
File webkit/glue/cpp_bound_class.cc (right):

https://codereview.chromium.org/15007012/diff/38001/webkit/glue/cpp_bound_cla...
webkit/glue/cpp_bound_class.cc:303: reinterpret_cast<NPP>(this),
&CppNPObject::np_class_);
why isn't this returning npp_.get() ?

https://codereview.chromium.org/15007012/diff/38001/webkit/plugins/npapi/webp...
File webkit/plugins/npapi/webplugin_impl.h (right):

https://codereview.chromium.org/15007012/diff/38001/webkit/plugins/npapi/webp...
webkit/plugins/npapi/webplugin_impl.h:23: #include
"third_party/npapi/bindings/npruntime.h"
the NPP type is in npapi.h, please just include that instead of the runtime.

https://codereview.chromium.org/15007012/diff/38001/webkit/plugins/ppapi/ppap...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

https://codereview.chromium.org/15007012/diff/38001/webkit/plugins/ppapi/ppap...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:154: // be an actual NPP_t
instance.
this is too crazy. if the type is NPP_t*, you really should return a pointer to
an NPP_t.  If you really want an opaque handle, make that the API

[Wez, 2013-05-22 06:55:25]

https://codereview.chromium.org/15007012/diff/38001/chrome/renderer/plugins/w...
File chrome/renderer/plugins/webview_plugin.cc (right):

https://codereview.chromium.org/15007012/diff/38001/chrome/renderer/plugins/w...
chrome/renderer/plugins/webview_plugin.cc:118: return NULL;
On 2013/05/21 18:20:21, jamesr wrote:
> how is this OK?

WebViewPlugin doesn't appear to do any scripting via NPObjects, does it?

https://codereview.chromium.org/15007012/diff/38001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/15007012/diff/38001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:1215: return
reinterpret_cast<NPP>(this);
On 2013/05/21 18:20:21, jamesr wrote:
> nak, if you're going to leave the type NPP_t*, return a pointer to an actual
> NPP_t

Done.

https://codereview.chromium.org/15007012/diff/38001/webkit/glue/cpp_bound_cla...
File webkit/glue/cpp_bound_class.cc (right):

https://codereview.chromium.org/15007012/diff/38001/webkit/glue/cpp_bound_cla...
webkit/glue/cpp_bound_class.cc:303: reinterpret_cast<NPP>(this),
&CppNPObject::np_class_);
On 2013/05/21 18:20:21, jamesr wrote:
> why isn't this returning npp_.get() ?

Done.

https://codereview.chromium.org/15007012/diff/38001/webkit/plugins/npapi/webp...
File webkit/plugins/npapi/webplugin_impl.h (right):

https://codereview.chromium.org/15007012/diff/38001/webkit/plugins/npapi/webp...
webkit/plugins/npapi/webplugin_impl.h:23: #include
"third_party/npapi/bindings/npruntime.h"
On 2013/05/21 18:20:21, jamesr wrote:
> the NPP type is in npapi.h, please just include that instead of the runtime.

Done.

https://codereview.chromium.org/15007012/diff/38001/webkit/plugins/ppapi/ppap...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

https://codereview.chromium.org/15007012/diff/38001/webkit/plugins/ppapi/ppap...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:154: // be an actual NPP_t
instance.
On 2013/05/21 18:20:21, jamesr wrote:
> this is too crazy. if the type is NPP_t*, you really should return a pointer
to
> an NPP_t.  If you really want an opaque handle, make that the API

Done.

[jamesr, 2013-05-22 07:01:36]

lgtm

[commit-bot: I haz the power, 2013-05-25 06:40:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/15007012/78001

[commit-bot: I haz the power, 2013-05-25 08:48:06]

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

[commit-bot: I haz the power, 2013-05-26 06:28:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/15007012/78001

[commit-bot: I haz the power, 2013-05-26 08:26:43]

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

[commit-bot: I haz the power, 2013-05-26 16:29:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/15007012/78001

[commit-bot: I haz the power, 2013-05-26 18:24:34]

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

[commit-bot: I haz the power, 2013-05-26 23:29:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/15007012/111001

[commit-bot: I haz the power, 2013-05-27 03:17:13]

Change committed as 202364