New Pepper API implementation.

Partially implement the new pepper API in Chrome. This is not actually hooked
up, which will require some changes in render_view as well as the plugin list.

TEST=none
BUG=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=46760

[brettw, 2010-05-03 19:43:17]

I think I need to stop working on this and get something checked in. 2D painting
*almost* works but currently shows black. It looks like something probably isn't
hooked up or getting painted correctly.

I would prefer it if most comments, especially about the future direction of the
API or some certain interface should work a little differently, could be filed
as bugs and fixed asynchronously. This is still fairly rough and preliminary,
and waiting until this is perfect to check anything in will just slow everything
down.

However, I am very interested in comments, just be clear what you think is the
most important to address before checkin, and what you think can be addressed in
future iterations.

[cpu_(ooo_6.6-7.5), 2010-05-03 21:55:09]

random bits, doing an actual review is too intimidating :)

http://codereview.chromium.org/1697008/diff/45001/46040
File third_party/ppapi/bindings/ppp.h (right):

http://codereview.chromium.org/1697008/diff/45001/46040#newcode20
third_party/ppapi/bindings/ppp.h:20: bool PPP_InitializeModule(PP_Module module,
is it crazy to return int and zero means success? or positive means success? 

I am thinking the case where the developer can tell an error code and we can
output the error code to some debug IO.

http://codereview.chromium.org/1697008/diff/45001/46040#newcode27
third_party/ppapi/bindings/ppp.h:27: // if the interface is not supported.
and interface_name is ascii not utf8, right?

http://codereview.chromium.org/1697008/diff/45001/46019
File third_party/ppapi/cpp/rect.h (right):

http://codereview.chromium.org/1697008/diff/45001/46019#newcode33
third_party/ppapi/cpp/rect.h:33: int width() const { return rect_.right -
rect_.left; }
any enforcement of left <= right needed? or shall we wait for the security
review? :)

http://codereview.chromium.org/1697008/diff/45001/46033
File third_party/ppapi/cpp/var.h (right):

http://codereview.chromium.org/1697008/diff/45001/46033#newcode19
third_party/ppapi/cpp/var.h:19: enum NullType { Null };  // Special value passed
to constructor to make NULL.
how about NullType being a struct like PassRef, because I think |Null| might
decay into int32_t

Also add explicit in some of the ctors

[brettw, 2010-05-03 22:16:39]

> Also add explicit in some of the ctors

I'll add explicit to Plugin, but I want to keep the Var constructors implicit. A
var is really just a wrapper around one of a number of simpler types, and I
think this will help readability a lot, for example:

var->Call("DoSomething", "/foo/bar/", 3);

vs

var->Call(pp::Var("DoSomething"), pp::Var("/foo/bar/"), pp:Var(3));

[sehr (please use chromium), 2010-05-03 22:55:18]

Sorry, I also found a full pass daunting at the moment.  Here are some comments.

http://codereview.chromium.org/1697008/diff/45001/46017
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/1697008/diff/45001/46017#newcode407
third_party/npapi/bindings/npapi_extensions.h:407: typedef void
(*NPDeleteFilePath)(NPFilePath* filePath);
Not sure where to put this, but for consistency with the existing, this function
should be NPDeleteFilePathPtr, or with the new stuff it should be
PPDeleteFilePath, I think.

http://codereview.chromium.org/1697008/diff/45001/46017#newcode432
third_party/npapi/bindings/npapi_extensions.h:432: typedef int
(*NPGetFileName)(const NPFilePath* filePath,
Ditto.

http://codereview.chromium.org/1697008/diff/45001/46045
File third_party/ppapi/bindings/pp_event.h (right):

http://codereview.chromium.org/1697008/diff/45001/46045#newcode74
third_party/ppapi/bindings/pp_event.h:74: /*typedef struct _NPDeviceEvent {
s/NP/PP/?

http://codereview.chromium.org/1697008/diff/45001/46039
File third_party/ppapi/bindings/pp_rect.h (right):

http://codereview.chromium.org/1697008/diff/45001/46039#newcode9
third_party/ppapi/bindings/pp_rect.h:9: int left;
Even though int is 32 basically everywhere, I think we should only use sized
types in the interface.

http://codereview.chromium.org/1697008/diff/45001/46042
File third_party/ppapi/bindings/pp_stdint.h (right):

http://codereview.chromium.org/1697008/diff/45001/46042#newcode1
third_party/ppapi/bindings/pp_stdint.h:1: 
Needs guards.

http://codereview.chromium.org/1697008/diff/45001/46052
File third_party/ppapi/bindings/pp_var.h (right):

http://codereview.chromium.org/1697008/diff/45001/46052#newcode7
third_party/ppapi/bindings/pp_var.h:7: 
We will want C++ guards on all the header files.

http://codereview.chromium.org/1697008/diff/45001/46052#newcode9
third_party/ppapi/bindings/pp_var.h:9: PP_VARTYPE_VOID,
The capitalization style is not consistent between this and the events file
above.

http://codereview.chromium.org/1697008/diff/45001/46052#newcode15
third_party/ppapi/bindings/pp_var.h:15: PP_VARTYPE_OBJECT,
Trailing commas are warnings/errors on NaCl and (I think) Chrome.

http://codereview.chromium.org/1697008/diff/45001/46052#newcode18
third_party/ppapi/bindings/pp_var.h:18: typedef struct _PP_Var {
Here and below.  Underscore followed by a capital letter is reserved for
compiler implementations.  Best not to use those.

http://codereview.chromium.org/1697008/diff/45001/46052#newcode27
third_party/ppapi/bindings/pp_var.h:27: };
I don't think C has anonymous unions.

http://codereview.chromium.org/1697008/diff/45001/46047
File third_party/ppapi/bindings/ppb.h (right):

http://codereview.chromium.org/1697008/diff/45001/46047#newcode10
third_party/ppapi/bindings/ppb.h:10: typedef const void*
(*PPB_GetInterface)(const char* interface_name);
Again, function names have Ptr in other contexts.  Be consistent.

http://codereview.chromium.org/1697008/diff/45001/46038
File third_party/ppapi/bindings/ppb_core.h (right):

http://codereview.chromium.org/1697008/diff/45001/46038#newcode18
third_party/ppapi/bindings/ppb_core.h:18: void (*AddRefVar)(PP_Var var);
Don't these need to take pointer to PP_Var rather than by-value?

http://codereview.chromium.org/1697008/diff/45001/46038#newcode60
third_party/ppapi/bindings/ppb_core.h:60: void (*AddRefResource)(PP_Resource
resource);
Likewise on PP_Resource*

http://codereview.chromium.org/1697008/diff/45001/46037
File third_party/ppapi/bindings/ppb_device_context_2d.h (right):

http://codereview.chromium.org/1697008/diff/45001/46037#newcode25
third_party/ppapi/bindings/ppb_device_context_2d.h:25: PP_Resource
(*Create)(PP_Module module, int width, int height);
Ptr, or remove from the other APIs.

http://codereview.chromium.org/1697008/diff/45001/46044
File third_party/ppapi/bindings/ppb_image_data.h (right):

http://codereview.chromium.org/1697008/diff/45001/46044#newcode22
third_party/ppapi/bindings/ppb_image_data.h:22: int width;
uint32_t?

http://codereview.chromium.org/1697008/diff/45001/46044#newcode35
third_party/ppapi/bindings/ppb_image_data.h:35: int width,
ditto.

http://codereview.chromium.org/1697008/diff/45001/46044#newcode50
third_party/ppapi/bindings/ppb_image_data.h:50: void* (*Map)(PP_Resource
resource);
By-reference.

http://codereview.chromium.org/1697008/diff/45001/46050
File third_party/ppapi/bindings/ppp_class.h (right):

http://codereview.chromium.org/1697008/diff/45001/46050#newcode16
third_party/ppapi/bindings/ppp_class.h:16: PP_Var name);
Since we're using the exception-oriented model here, this also needs to have an
exception (they're read/write, aren't they?) to guard against a bogus object
being passed in from a previously excepting call.

http://codereview.chromium.org/1697008/diff/45001/46050#newcode24
third_party/ppapi/bindings/ppp_class.h:24: PP_Var** properties);
Does this really get all properties, or all property names?  I don't think the
first meaning is terribly useful, and the second meaning would be better with
"GetAllPropertyNames".

And ditto with the exception above.

http://codereview.chromium.org/1697008/diff/45001/46050#newcode37
third_party/ppapi/bindings/ppp_class.h:37: // method_name is guaranteed to be
either a string or an integer.
Is an exception returned if it is not?  What exception?

http://codereview.chromium.org/1697008/diff/45001/46050#newcode40
third_party/ppapi/bindings/ppp_class.h:40: size_t argc,
Sometimes you've used size_t, other times ints.  One or the other.

http://codereview.chromium.org/1697008/diff/45001/46032
File third_party/ppapi/cpp/device_context_2d.h (right):

http://codereview.chromium.org/1697008/diff/45001/46032#newcode36
third_party/ppapi/cpp/device_context_2d.h:36: int x, int y,
again, explicit sizes seem preferable.

http://codereview.chromium.org/1697008/diff/45001/46022
File third_party/ppapi/cpp/image_data.h (right):

http://codereview.chromium.org/1697008/diff/45001/46022#newcode35
third_party/ppapi/cpp/image_data.h:35: int width() const { return desc_.width; }
uint32_t better conveys this I think.

http://codereview.chromium.org/1697008/diff/45001/46023
File third_party/ppapi/cpp/plugin_class.cc (right):

http://codereview.chromium.org/1697008/diff/45001/46023#newcode117
third_party/ppapi/cpp/plugin_class.cc:117: &Deallocate,
no trailing ,

http://codereview.chromium.org/1697008/diff/45001/46025
File third_party/ppapi/cpp/plugin_class.h (right):

http://codereview.chromium.org/1697008/diff/45001/46025#newcode27
third_party/ppapi/cpp/plugin_class.h:27: virtual void
GetAllProperties(std::vector<Var>* properties);
Ditto w.r.t. property names, I think.

http://codereview.chromium.org/1697008/diff/45001/46019
File third_party/ppapi/cpp/rect.h (right):

http://codereview.chromium.org/1697008/diff/45001/46019#newcode33
third_party/ppapi/cpp/rect.h:33: int width() const { return rect_.right -
rect_.left; }
uint32_t.

http://codereview.chromium.org/1697008/diff/45001/46011
File webkit/glue/plugins/pepper_image_data.h (right):

http://codereview.chromium.org/1697008/diff/45001/46011#newcode19
webkit/glue/plugins/pepper_image_data.h:19: class SkBitmap;
We should make sure the implementation of 2D is thread safe from the beginning.

http://codereview.chromium.org/1697008/diff/45001/46011#newcode55
webkit/glue/plugins/pepper_image_data.h:55: int width_;
uint32_t.

http://codereview.chromium.org/1697008/diff/45001/46015
File webkit/glue/plugins/pepper_plugin_module.cc (right):

http://codereview.chromium.org/1697008/diff/45001/46015#newcode77
webkit/glue/plugins/pepper_plugin_module.cc:77: DLOG(WARNING) << "AddRef()ing a
nonexistant resource";
nonexistent

http://codereview.chromium.org/1697008/diff/45001/46015#newcode86
webkit/glue/plugins/pepper_plugin_module.cc:86: DLOG(WARNING) << "Release()ing a
nonexistant resource";
nonexistent

[brettw, 2010-05-04 05:53:57]

New snap up.

http://codereview.chromium.org/1697008/diff/45001/46017
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/1697008/diff/45001/46017#newcode407
third_party/npapi/bindings/npapi_extensions.h:407: typedef void
(*NPDeleteFilePath)(NPFilePath* filePath);
Sorry, this file has an old change I was working on that got confused in this
CL. I'll remove this file.

http://codereview.chromium.org/1697008/diff/45001/46039
File third_party/ppapi/bindings/pp_rect.h (right):

http://codereview.chromium.org/1697008/diff/45001/46039#newcode9
third_party/ppapi/bindings/pp_rect.h:9: int left;
I agree, I did this for some of them but not consistently. I should have caught
them all now.

http://codereview.chromium.org/1697008/diff/45001/46052
File third_party/ppapi/bindings/pp_var.h (right):

http://codereview.chromium.org/1697008/diff/45001/46052#newcode7
third_party/ppapi/bindings/pp_var.h:7: 
On 2010/05/03 22:55:19, sehr wrote:
> We will want C++ guards on all the header files.

Why?

http://codereview.chromium.org/1697008/diff/45001/46052#newcode18
third_party/ppapi/bindings/pp_var.h:18: typedef struct _PP_Var {
I've heard of no "__" but this I've never heard of! Turns out you're right. I
would like to register my displeasure with the authorities. In the meantime,
I've changed the "struct types" to _pp

http://codereview.chromium.org/1697008/diff/45001/46047
File third_party/ppapi/bindings/ppb.h (right):

http://codereview.chromium.org/1697008/diff/45001/46047#newcode10
third_party/ppapi/bindings/ppb.h:10: typedef const void*
(*PPB_GetInterface)(const char* interface_name);
I haven't used Ptr anywhere.

http://codereview.chromium.org/1697008/diff/45001/46038
File third_party/ppapi/bindings/ppb_core.h (right):

http://codereview.chromium.org/1697008/diff/45001/46038#newcode18
third_party/ppapi/bindings/ppb_core.h:18: void (*AddRefVar)(PP_Var var);
On 2010/05/03 22:55:19, sehr wrote:
> Don't these need to take pointer to PP_Var rather than by-value?

Both PP_Var and PP_Resource can be passed by value. They are just a struct
wrapper around an integer "handle" value.

http://codereview.chromium.org/1697008/diff/45001/46040
File third_party/ppapi/bindings/ppp.h (right):

http://codereview.chromium.org/1697008/diff/45001/46040#newcode20
third_party/ppapi/bindings/ppp.h:20: bool PPP_InitializeModule(PP_Module module,
Sounds OK to me.

http://codereview.chromium.org/1697008/diff/45001/46040#newcode27
third_party/ppapi/bindings/ppp.h:27: // if the interface is not supported.
Yes, I clarified this.

http://codereview.chromium.org/1697008/diff/45001/46050
File third_party/ppapi/bindings/ppp_class.h (right):

http://codereview.chromium.org/1697008/diff/45001/46050#newcode16
third_party/ppapi/bindings/ppp_class.h:16: PP_Var name);
The only way to call these functions is through the browser, since we no longer
expose the "class" on Variant. So JS from the browser, or plugin code all has to
go through the PPB_Var code, which can provide uniform checking of the "input"
exceptions and not even call the plugin in that case.

This makes the code prettier and harder to screw up (not checking an exception
on the call would be super easy to forget). So I only added an exception in the
cases where the plugin would actually need to generate one. In the case of
HasProperty and GetAllProperties, the object is guaranteed valid and there is no
exception.

http://codereview.chromium.org/1697008/diff/45001/46050#newcode37
third_party/ppapi/bindings/ppp_class.h:37: // method_name is guaranteed to be
either a string or an integer.
On 2010/05/03 22:55:19, sehr wrote:
> Is an exception returned if it is not?  What exception?

As before, the browser code calling this will check the method name type. This
comment is really for the implementor, so there's no need to mention that rule.
That rule would be on PPB_Var.Call

http://codereview.chromium.org/1697008/diff/45001/46050#newcode40
third_party/ppapi/bindings/ppp_class.h:40: size_t argc,
I fixed all these to use sized values.

http://codereview.chromium.org/1697008/diff/45001/46022
File third_party/ppapi/cpp/image_data.h (right):

http://codereview.chromium.org/1697008/diff/45001/46022#newcode35
third_party/ppapi/cpp/image_data.h:35: int width() const { return desc_.width; }
I changed these to int32_t. Google style prohibits unsigned types in these
cases, and I additionally felt like using int32_t for all "coordinate" values
makes more sense.

http://codereview.chromium.org/1697008/diff/45001/46019
File third_party/ppapi/cpp/rect.h (right):

http://codereview.chromium.org/1697008/diff/45001/46019#newcode33
third_party/ppapi/cpp/rect.h:33: int width() const { return rect_.right -
rect_.left; }
If there is a security problem with the rect sizes, this is not the place for
that checking. This is just a thin wrapper around a C-style struct. Some plugins
won't use this wrapper and will write to the struct themselves.

So the browser layer than sends rects to and from the plugin will need to check
for this stuff. We could additionally add checks in the set_ functions in case a
plugin creates an invalid rect and then uses it from shooting itself in the
foot.

But given that we're left,right,top,bottom, it's hard to enforce this since it
could throw an error if you're setting a new rect and do the bottom before the
top, which seems confusing to me. gfx::Rect gets around this by using
x,y,width,height. I did l,t,r,b because that's what NPAPI did. I'm OK changing
this if you think it will be helpful.

[David Springer, 2010-05-04 15:54:11]

Can you please split this into at least 2 CLs?  One being the C-layer Pepper
API, and the other being the C++ PPAPI wrappers.  Then, would you consider
potentially having two sets of reviewers?  The current list seems ideal for
discussing the Pepper C API.  For the C++ layer, please consider the Boulder
team (who is working on a C++ library for programming Native Client modules).  
Today, that includes gwink@, dspringer@, sanga@ and alokp@.  Also, it might make
sense like to hold off on the C++ PPAPI layer until we have had some more
collaboration on it.

[brettw, 2010-05-04 17:53:47]

On Tue, May 4, 2010 at 8:54 AM,  <dspringer@google.com> wrote:
> Can you please split this into at least 2 CLs?  One being the C-layer Pepper
> API, and the other being the C++ PPAPI wrappers.  Then, would you consider
> potentially having two sets of reviewers?  The current list seems ideal for
> discussing the Pepper C API.  For the C++ layer, please consider the Boulder
> team (who is working on a C++ library for programming Native Client
> modules).
> Today, that includes gwink@, dspringer@, sanga@ and alokp@.  Also, it might
> make
> sense like to hold off on the C++ PPAPI layer until we have had some more
> collaboration on it.

I'm happy to break this off. I scheduled a meeting this afternoon to
sync up about the C++ stuff.

Brett

[brettw, 2010-05-04 18:02:44]

http://codereview.chromium.org/1697008/diff/45001/46011
File webkit/glue/plugins/pepper_image_data.h (right):

http://codereview.chromium.org/1697008/diff/45001/46011#newcode19
webkit/glue/plugins/pepper_image_data.h:19: class SkBitmap;
I'm currently just trying to get something checked in, followed by feature
parity with the existing Pepper so this work isn't blocking people. I think
threadsafety should come after that because it's not blocking anybody.

[cpu_(ooo_6.6-7.5), 2010-05-04 18:13:42]

On 2010/05/03 22:16:39, brettw wrote:
> > Also add explicit in some of the ctors
> 
> I'll add explicit to Plugin, but I want to keep the Var constructors implicit.
A
> var is really just a wrapper around one of a number of simpler types, and I
> think this will help readability a lot, for example:
> 
> var->Call("DoSomething", "/foo/bar/", 3);
> 
> vs
> 
> var->Call(pp::Var("DoSomething"), pp::Var("/foo/bar/"), pp:Var(3));

I am ok with that as long as it does the expected thing. For example is
var->Call(0) doing the right thing?

[brettw, 2010-05-04 18:18:37]

On Tue, May 4, 2010 at 11:13 AM,  <cpu@chromium.org> wrote:
> On 2010/05/03 22:16:39, brettw wrote:
>>
>> > Also add explicit in some of the ctors
>
>> I'll add explicit to Plugin, but I want to keep the Var constructors
>> implicit.
>
> A
>>
>> var is really just a wrapper around one of a number of simpler types, and
>> I
>> think this will help readability a lot, for example:
>
>> var->Call("DoSomething", "/foo/bar/", 3);
>
>> vs
>
>> var->Call(pp::Var("DoSomething"), pp::Var("/foo/bar/"), pp:Var(3));
>
> I am ok with that as long as it does the expected thing. For example is
> var->Call(0) doing the right thing?

Call(0) would be calling the "function" 0 which would be like JS array
access. That's the expected thing to me, but I don't know about
others.

Brett

[cpu_(ooo_6.6-7.5), 2010-05-04 18:29:29]

mor random thoughts

http://codereview.chromium.org/1697008/diff/113001/91019
File third_party/ppapi/cpp/image_data.cc (right):

http://codereview.chromium.org/1697008/diff/113001/91019#newcode7
third_party/ppapi/cpp/image_data.cc:7: #include <string.h>
seems unused

http://codereview.chromium.org/1697008/diff/113001/91019#newcode26
third_party/ppapi/cpp/image_data.cc:26: ImageData::ImageData() : Resource() {
it is somewhat strange to explicitly call the default ctor of the parent

Also how about data_(NULL) ?

http://codereview.chromium.org/1697008/diff/113001/91019#newcode31
third_party/ppapi/cpp/image_data.cc:31: desc_ = other.desc_;
only copies the descriptor?

http://codereview.chromium.org/1697008/diff/113001/91019#newcode51
third_party/ppapi/cpp/image_data.cc:51: }
no need to free data_ ?

[brettw, 2010-05-04 18:44:29]

http://codereview.chromium.org/1697008/diff/113001/91019
File third_party/ppapi/cpp/image_data.cc (right):

http://codereview.chromium.org/1697008/diff/113001/91019#newcode31
third_party/ppapi/cpp/image_data.cc:31: desc_ = other.desc_;
On 2010/05/04 18:29:29, cpu wrote:
> only copies the descriptor?

This calls Resource's copy constructor which will copy the resource handle.

http://codereview.chromium.org/1697008/diff/113001/91019#newcode51
third_party/ppapi/cpp/image_data.cc:51: }
On 2010/05/04 18:29:29, cpu wrote:
> no need to free data_ ?

This is managed by the resource handle which will get freed by the browser when
all refs go away.

[brettw, 2010-05-04 20:57:39]

I split the C++ bindings layer into issue 1945002 and added the people I thought
were most relevant. Please feel free to look at it even if I didn't add you.

[darin (slow to review), 2010-05-05 01:39:29]

some comments on the C API

http://codereview.chromium.org/1697008/diff/83005/133029
File third_party/ppapi/bindings/pp_event.h (right):

http://codereview.chromium.org/1697008/diff/83005/133029#newcode74
third_party/ppapi/bindings/pp_event.h:74: /*typedef struct _pp_Event_Device {
perhaps this commented out code should just be deleted?  or add a TODO?

http://codereview.chromium.org/1697008/diff/83005/133029#newcode98
third_party/ppapi/bindings/pp_event.h:98: //PP_Event_Minimize minimize;
ditto

http://codereview.chromium.org/1697008/diff/83005/133029#newcode105
third_party/ppapi/bindings/pp_event.h:105: 
nit: extra new line

http://codereview.chromium.org/1697008/diff/83005/133025
File third_party/ppapi/bindings/pp_instance.h (right):

http://codereview.chromium.org/1697008/diff/83005/133025#newcode10
third_party/ppapi/bindings/pp_instance.h:10: // A PP_Instances uniquely
identifies one plugin instance, which is one time
nit: PP_Instances -> PP_Instance

http://codereview.chromium.org/1697008/diff/83005/133032
File third_party/ppapi/bindings/pp_module.h (right):

http://codereview.chromium.org/1697008/diff/83005/133032#newcode16
third_party/ppapi/bindings/pp_module.h:16: 
nit: extra new line

http://codereview.chromium.org/1697008/diff/83005/133034
File third_party/ppapi/bindings/pp_resource.h (right):

http://codereview.chromium.org/1697008/diff/83005/133034#newcode8
third_party/ppapi/bindings/pp_resource.h:8: #include
"ppapi/bindings/pp_stdint.h"
is "bindings" the best directory name for the low-level headers?  i know we have
that for npapi, but that is just historical baggage from webkit which used to
store its npapi.h in a directory named bindings.  it no longer does!

i think it is nice to have a separate directory for the C++ layer, and it is
perhaps nice to have the C API in its own directory.  maybe ppapi/{c,cpp}/?

http://codereview.chromium.org/1697008/diff/83005/133030
File third_party/ppapi/bindings/ppb.h (right):

http://codereview.chromium.org/1697008/diff/83005/133030#newcode10
third_party/ppapi/bindings/ppb.h:10: typedef const void*
(*PPB_GetInterface)(const char* interface_name);
perhaps we should define a syntax for interface name that includes a version
number?

"PPB_Core" -> "PPB_Core;1" for version 1 of PPB_Core.

http://codereview.chromium.org/1697008/diff/83005/133022
File third_party/ppapi/bindings/ppb_core.h (right):

http://codereview.chromium.org/1697008/diff/83005/133022#newcode15
third_party/ppapi/bindings/ppb_core.h:15: typedef struct _ppb_Core {
it seems like there should be a PPB_Var module for dealing with PP_Var objects.

http://codereview.chromium.org/1697008/diff/83005/133022#newcode61
third_party/ppapi/bindings/ppb_core.h:61: void (*AddRefResource)(PP_Resource
resource);
and PPB_Resource for PP_Resource objects

http://codereview.chromium.org/1697008/diff/83005/133021
File third_party/ppapi/bindings/ppb_device_context_2d.h (right):

http://codereview.chromium.org/1697008/diff/83005/133021#newcode34
third_party/ppapi/bindings/ppb_device_context_2d.h:34: void
(*PaintImageData)(PP_Resource device_context,
I wonder if we shouldn't restructure this so that we have the following:

  void (*DrawImage)(PP_Resource device_context,
                    PP_Resource image,
                    const PP_Rect* image_rect,
                    int32_t x, int32_t y);
  void (*Scroll)(PP_Resource device_context,
                 const PP_Rect* clip_rect,
                 int32_t dx, int32_t dy);
  void (*Flush)(PP_Resource device_context,
                PPB_DeviceContext2D_FlushCallback callback,
                void* callback_data);

In this model, an application can use DrawImage to paint a region of the
retained backingstore from a region of a given image.  It can use Scroll to
shift the pixels of the retained backingstore within the designated clip_rect. 
Finally, an application can call Flush to wait for updates to propagate.

It would have to be noted that the image passed to DrawImage must not be
modified until the Flush callback has run.

I think DrawImage and Scroll better align with
WebWidgetClient::did{InvalidateRect,Scroll}Rect, which means that our
implementation should be fairly simple.

http://codereview.chromium.org/1697008/diff/83005/133027
File third_party/ppapi/bindings/ppb_instance.h (right):

http://codereview.chromium.org/1697008/diff/83005/133027#newcode29
third_party/ppapi/bindings/ppb_instance.h:29: bool
(*BindGraphicsDeviceContext)(PP_Instance instance,
BindDeviceContext seems sufficient since a PP_Instance refers to an <embed> tag,
but i can see that you wanted to eliminate confusion with audio devices, etc.

when we support camera devices, perhaps we'd be able to bind those to an
instance as well!  or maybe that is just crazy talk ;-)

http://codereview.chromium.org/1697008/diff/83005/133033
File third_party/ppapi/bindings/ppp_class.h (right):

http://codereview.chromium.org/1697008/diff/83005/133033#newcode52
third_party/ppapi/bindings/ppp_class.h:52: } PPP_Class;
I don't see any references to PPP_Class outside of this header file, so it is
not completely clear how this will be used.

http://codereview.chromium.org/1697008/diff/83005/133031
File third_party/ppapi/bindings/ppp_instance.h (right):

http://codereview.chromium.org/1697008/diff/83005/133031#newcode27
third_party/ppapi/bindings/ppp_instance.h:27: // Called immediately after
NewInstance. Plugins can do instance-specific
NewInstance -> New

[David Springer, 2010-05-05 14:42:37]

A few comments, I just started to dive into this.

http://codereview.chromium.org/1697008/diff/83005/133021
File third_party/ppapi/bindings/ppb_device_context_2d.h (right):

http://codereview.chromium.org/1697008/diff/83005/133021#newcode34
third_party/ppapi/bindings/ppb_device_context_2d.h:34: void
(*PaintImageData)(PP_Resource device_context,
On 2010/05/05 01:39:29, darin wrote:
> I wonder if we shouldn't restructure this so that we have the following:
> 
>   void (*DrawImage)(PP_Resource device_context,
>                     PP_Resource image,
>                     const PP_Rect* image_rect,
>                     int32_t x, int32_t y);
>   void (*Scroll)(PP_Resource device_context,
>                  const PP_Rect* clip_rect,
>                  int32_t dx, int32_t dy);
>   void (*Flush)(PP_Resource device_context,
>                 PPB_DeviceContext2D_FlushCallback callback,
>                 void* callback_data);
> 
> In this model, an application can use DrawImage to paint a region of the
> retained backingstore from a region of a given image.  It can use Scroll to
> shift the pixels of the retained backingstore within the designated clip_rect.

> Finally, an application can call Flush to wait for updates to propagate.
> 
> It would have to be noted that the image passed to DrawImage must not be
> modified until the Flush callback has run.
> 
> I think DrawImage and Scroll better align with
> WebWidgetClient::did{InvalidateRect,Scroll}Rect, which means that our
> implementation should be fairly simple.

Is there any plan to support flipped image/device coordinate systems?  If so,
then an is_flipped parameter to DrawImage might be good.

Do we plan to support any compositing at this level of the API?  In that case,
adding a composite op parameter would be good.  As for scrolling, this (and more
general image copy-with-scale, etc) can be accomplished with something like:
  void (*DrawInRectFromRect)(PP_Resource device_context,
                                                    const PP_Rect* dest_rect, //
in device coords
                                                    const PP_Rect* from_rect, //
in image coords
                                                    PP_CompositeOp op);

DrawImage() becomes a convenience wrapper on top of DrawInRectFromRect() where
the whole image is drawn to the point specified by dest_rect->origin with a
scale of 1.

See here for more ideas:
   
http://developer.apple.com/mac/library/documentation/cocoa/Reference/Applicat...:

http://codereview.chromium.org/1697008/diff/83005/133010
File webkit/glue/plugins/pepper_image_data.cc (right):

http://codereview.chromium.org/1697008/diff/83005/133010#newcode103
webkit/glue/plugins/pepper_image_data.cc:103:
module()->GetSomeInstance()->delegate()->CreateImage2D(width, height));
There have been lots of times when I get a spurious crash in code like this b/c
of the assumption that (e.g.) delegate() return non-NULL.  The cause is usually
something like a bad MIME type for the plugin, but when all I get is a crash at
this level, that kind of stuff can be hard to find.  Maybe some asserts or
if-blocks would be useful?

http://codereview.chromium.org/1697008/diff/83005/133017
File webkit/glue/plugins/pepper_plugin_module.cc (right):

http://codereview.chromium.org/1697008/diff/83005/133017#newcode103
webkit/glue/plugins/pepper_plugin_module.cc:103: const void* GetInterface(const
char* name) {
Could this be implemented as a registry?

http://codereview.chromium.org/1697008/diff/83005/133017#newcode104
webkit/glue/plugins/pepper_plugin_module.cc:104: if (strcmp(name,
PPB_CORE_INTERFACE) == 0)
Not sure if this is applicable here, but generally speaking, strncmp() is more
secure than strcmp.

http://codereview.chromium.org/1697008/diff/83005/133017#newcode111
webkit/glue/plugins/pepper_plugin_module.cc:111: return
DeviceContext2D::GetInterface();
I suppose there is a plan to add 3D, audio, microphone and camera interfaces? 
Would it make sense to have a more extensible mechanism here?  I guess I'm not
totally clear on what this interface connects together.  (I did read
http://wiki.corp.google.com/twiki/bin/view/Main/PepperAPI)

http://codereview.chromium.org/1697008/diff/83005/133017#newcode190
webkit/glue/plugins/pepper_plugin_module.cc:190: int retval =
initialize_module(GetModule(), &GetInterface);
Is there a plan to support interfaces other than the ones baked into
GetInterface()?  I guess I'm not clear on what the two sides of this particular
interface are.  Is this a means for any plugin to publish it's interface to the
browser?  Or is this supposed to be a very restricted set of module interfaces
that we control?

http://codereview.chromium.org/1697008/diff/83005/133008
File webkit/glue/plugins/pepper_resource_tracker.cc (right):

http://codereview.chromium.org/1697008/diff/83005/133008#newcode27
webkit/glue/plugins/pepper_resource_tracker.cc:27: return
Singleton<ResourceTracker>::get();
I assume Singleton handles threading issues?

Is there any way to hang this off the Module instance?  It would be nice to have
fewer singletons.  Or do resources outlive the module?

http://codereview.chromium.org/1697008/diff/83005/133004
File webkit/glue/plugins/pepper_resource_tracker.h (right):

http://codereview.chromium.org/1697008/diff/83005/133004#newcode21
webkit/glue/plugins/pepper_resource_tracker.h:21: class ResourceTracker {
A comment about what a resource is, and how it relates to the module would help
me understand this class a lot more.

[brettw, 2010-05-05 16:31:57]

Response to David's comments. I didn't upload a new version yet (going to work
on Darin's comments first).

http://codereview.chromium.org/1697008/diff/83005/133021
File third_party/ppapi/bindings/ppb_device_context_2d.h (right):

http://codereview.chromium.org/1697008/diff/83005/133021#newcode34
third_party/ppapi/bindings/ppb_device_context_2d.h:34: void
(*PaintImageData)(PP_Resource device_context,
I hadn't planned on flipped coordinate systems. This sounds like something we
can add later if it turns out there's a need.

I don't think compositing at this level really makes sense. These paint commands
are really updating a backing store of the plugin. The compositing operation
here is "src" (meaning we don't do a blend, we replace the RGBA values with the
new ones). This backing store is composited with the page whenever we need to
paint the plugin, and the plugin doesn't see or control this painting. I don't
see much use for doing blending with previous painting you've done into the
backing store. This is the type of thing the plugin can do itself if it wants
to.

I don't really see the use case for doing a scale with this type of operation.
But I assume if Apple supports it, there must be a good reason. When does this
come up?

I like Darin's Flush idea. Is it okay with you guys if I file a bug on this and
check in the current painting code as the initial pass. This will also give us
more time to discuss scrolling.

http://codereview.chromium.org/1697008/diff/83005/133010
File webkit/glue/plugins/pepper_image_data.cc (right):

http://codereview.chromium.org/1697008/diff/83005/133010#newcode103
webkit/glue/plugins/pepper_image_data.cc:103:
module()->GetSomeInstance()->delegate()->CreateImage2D(width, height));
There's already a NOTREACHED in GetSomeInstance if it's about to return NULL,
and I just added a DCHECK in the constructor of Instance to make sure we don't
get a null delegate.

http://codereview.chromium.org/1697008/diff/83005/133017
File webkit/glue/plugins/pepper_plugin_module.cc (right):

http://codereview.chromium.org/1697008/diff/83005/133017#newcode103
webkit/glue/plugins/pepper_plugin_module.cc:103: const void* GetInterface(const
char* name) {
On 2010/05/05 14:42:37, David Springer wrote:
> Could this be implemented as a registry?

See comment later.

http://codereview.chromium.org/1697008/diff/83005/133017#newcode104
webkit/glue/plugins/pepper_plugin_module.cc:104: if (strcmp(name,
PPB_CORE_INTERFACE) == 0)
On 2010/05/05 14:42:37, David Springer wrote:
> Not sure if this is applicable here, but generally speaking, strncmp() is more
> secure than strcmp.

I think since one side is always a constant, there's no security benefit for
strncmp. Please correct me if I'm mistaken.

http://codereview.chromium.org/1697008/diff/83005/133017#newcode190
webkit/glue/plugins/pepper_plugin_module.cc:190: int retval =
initialize_module(GetModule(), &GetInterface);
The interface stuff isn't meant to be dynamically extensible. It's just a means
of connection vtables of known formats and having a way to version them and say
"I don't support that" between the plugin and the browser (going both ways). So
there is always a fixed set of APIs that the browser knows it supports (this may
be expanded over time as we add more, but it always fixed for a particular
version). I also don't anticipate accumulating a huge number of interfaces over
time, maybe 10-20. Given this, I did a straightforward hard-coding of everything
(this also saves some set-up).

http://codereview.chromium.org/1697008/diff/83005/133008
File webkit/glue/plugins/pepper_resource_tracker.cc (right):

http://codereview.chromium.org/1697008/diff/83005/133008#newcode27
webkit/glue/plugins/pepper_resource_tracker.cc:27: return
Singleton<ResourceTracker>::get();
On 2010/05/05 14:42:37, David Springer wrote:
> I assume Singleton handles threading issues?

Yes.

> Is there any way to hang this off the Module instance?  It would be nice to
have
> fewer singletons.  Or do resources outlive the module?

I thought about that. I did it this way to simplify the API. If we hang this off
the module, we need to pass the module ID for all commands that take a resource
so we know where to look it up. It's nicer from the caller of a Pepper API's
perspective to just give a resource, and have the browser figure out which
module that resource is associated with.

We could also solve this problem by doing a separate thunk for each module that
records which module the call is coming from for each call, but that seemed like
unnecessary complexity.

[brettw, 2010-05-05 17:42:16]

New snap up. Hopefully I got everything.

http://codereview.chromium.org/1697008/diff/83005/133027
File third_party/ppapi/bindings/ppb_instance.h (right):

http://codereview.chromium.org/1697008/diff/83005/133027#newcode29
third_party/ppapi/bindings/ppb_instance.h:29: bool
(*BindGraphicsDeviceContext)(PP_Instance instance,
On 2010/05/05 01:39:29, darin wrote:
> BindDeviceContext seems sufficient since a PP_Instance refers to an <embed>
tag,
> but i can see that you wanted to eliminate confusion with audio devices, etc.

Sorry, this isn't clear. Do you want me to change this? I was trying to reduce
confusion with other types of devices and resources.

http://codereview.chromium.org/1697008/diff/83005/133033
File third_party/ppapi/bindings/ppp_class.h (right):

http://codereview.chromium.org/1697008/diff/83005/133033#newcode52
third_party/ppapi/bindings/ppp_class.h:52: } PPP_Class;
On 2010/05/05 01:39:29, darin wrote:
> I don't see any references to PPP_Class outside of this header file, so it is
> not completely clear how this will be used.

This is used in cpp/plugin_class.cc. I haven't actually hooked that up in the
browser yet. This is the replacement for NPClass.

[David Springer, 2010-05-05 20:17:20]

http://codereview.chromium.org/1697008/diff/83005/133007
File webkit/glue/plugins/pepper_resource.h (right):

http://codereview.chromium.org/1697008/diff/83005/133007#newcode21
webkit/glue/plugins/pepper_resource.h:21: Resource(PluginModule* module);
Nit: I think style guide wants 'explicit' on single-param ctors.

http://codereview.chromium.org/1697008/diff/83005/133007#newcode38
webkit/glue/plugins/pepper_resource.h:38: PluginModule* module_;
It's not clear to me who owns this pointer.  Would a scoped_refptr() make sense
here?

http://codereview.chromium.org/1697008/diff/83005/133008
File webkit/glue/plugins/pepper_resource_tracker.cc (right):

http://codereview.chromium.org/1697008/diff/83005/133008#newcode27
webkit/glue/plugins/pepper_resource_tracker.cc:27: return
Singleton<ResourceTracker>::get();
On 2010/05/05 16:31:58, brettw wrote:
> On 2010/05/05 14:42:37, David Springer wrote:
> > I assume Singleton handles threading issues?
> 
> Yes.
> 
> > Is there any way to hang this off the Module instance?  It would be nice to
> have
> > fewer singletons.  Or do resources outlive the module?
> 
> I thought about that. I did it this way to simplify the API. If we hang this
off
> the module, we need to pass the module ID for all commands that take a
resource
> so we know where to look it up. It's nicer from the caller of a Pepper API's
> perspective to just give a resource, and have the browser figure out which
> module that resource is associated with.
> 
> We could also solve this problem by doing a separate thunk for each module
that
> records which module the call is coming from for each call, but that seemed
like
> unnecessary complexity.

Couple of comments:
1. If the mapping of module -> resource really is one-to-many, then I think it's
good to pass the module along with the resource.  The idea is that the API
doesn't assume the existence of any services (which usually makes testing
easier).

2. Funcitonal testing of this interface seems difficult: it assumes a browser
that can map resources to a module, correct?  It might make writing tests easier
(from a mocking and injection POV) to make the connection between modules and
resources more explicit.

3. Is it possible for resources to disappear outside of this manager object? 
What happens in that case (if it's possible)?

[brettw, 2010-05-05 20:50:08]

On Wed, May 5, 2010 at 1:17 PM,  <dspringer@google.com> wrote:
>
> http://codereview.chromium.org/1697008/diff/83005/133007
> File webkit/glue/plugins/pepper_resource.h (right):
>
> http://codereview.chromium.org/1697008/diff/83005/133007#newcode21
> webkit/glue/plugins/pepper_resource.h:21: Resource(PluginModule*
> module);
> Nit: I think style guide wants 'explicit' on single-param ctors.

Thanks

> http://codereview.chromium.org/1697008/diff/83005/133007#newcode38
> webkit/glue/plugins/pepper_resource.h:38: PluginModule* module_;
> It's not clear to me who owns this pointer.  Would a scoped_refptr()
> make sense here?

I commented that this is a non-owning reference. See below.

> http://codereview.chromium.org/1697008/diff/83005/133008
> File webkit/glue/plugins/pepper_resource_tracker.cc (right):
>
> http://codereview.chromium.org/1697008/diff/83005/133008#newcode27
> webkit/glue/plugins/pepper_resource_tracker.cc:27: return
> Singleton<ResourceTracker>::get();
> On 2010/05/05 16:31:58, brettw wrote:
>>
>> On 2010/05/05 14:42:37, David Springer wrote:
>> > I assume Singleton handles threading issues?
>
>> Yes.
>
>> > Is there any way to hang this off the Module instance?  It would be
>
> nice to
>>
>> have
>> > fewer singletons.  Or do resources outlive the module?
>
>> I thought about that. I did it this way to simplify the API. If we
>
> hang this off
>>
>> the module, we need to pass the module ID for all commands that take a
>
> resource
>>
>> so we know where to look it up. It's nicer from the caller of a Pepper
>
> API's
>>
>> perspective to just give a resource, and have the browser figure out
>
> which
>>
>> module that resource is associated with.
>
>> We could also solve this problem by doing a separate thunk for each
>
> module that
>>
>> records which module the call is coming from for each call, but that
>
> seemed like
>>
>> unnecessary complexity.
>
> Couple of comments:
> 1. If the mapping of module -> resource really is one-to-many, then I
> think it's good to pass the module along with the resource.  The idea is
> that the API doesn't assume the existence of any services (which usually
> makes testing easier).
>
> 2. Funcitonal testing of this interface seems difficult: it assumes a
> browser that can map resources to a module, correct?  It might make
> writing tests easier (from a mocking and injection POV) to make the
> connection between modules and resources more explicit.

We do need to do more work on this since when a module goes away, we
need to be sure to free all resources. I was going to revisit this on
a separate pass since right now there's no easy way to get all
resources for a given module. That pass should probably also include
figuring out how we're going to do testing.

I'd really like to find a way not to force the API to include the
module ID everywhere. It seems silly from a plugin author perspective
that basically every function dealing with resources (for example,
every ImageData and Device2D function), must take a module ID because
we don't want to track this in the browser.

> 3. Is it possible for resources to disappear outside of this manager
> object?  What happens in that case (if it's possible)?

Not the way it's currently implemented. If this somehow happened, you
wouldn't be able to use it in any pepper function calls since we would
think it's invalid.

Brett

[jam, 2010-05-05 23:24:51]

some minor comments, overall looks good

http://codereview.chromium.org/1697008/diff/77052/137016
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137016#newcode41
chrome/renderer/pepper_plugin_delegate_impl.cc:41: 
nit: extra line

http://codereview.chromium.org/1697008/diff/77052/137015
File chrome/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/1697008/diff/77052/137015#newcode22
chrome/renderer/pepper_plugin_delegate_impl.h:22: RenderView* render_view_;
nit: perhaps this should be a WeakPtr?

http://codereview.chromium.org/1697008/diff/77052/137033
File third_party/ppapi/c/pp_instance.h (right):

http://codereview.chromium.org/1697008/diff/77052/137033#newcode15
third_party/ppapi/c/pp_instance.h:15: } PP_Instance;
nit: i'm curious why add all these header files with only a small struct in
each?  are you planning on adding more stuff here in the future?  if not, why
not put them together?  it makes inspecting the api simpler if everything is in
one file.

http://codereview.chromium.org/1697008/diff/77052/137031
File third_party/ppapi/c/pp_rect.h (right):

http://codereview.chromium.org/1697008/diff/77052/137031#newcode15
third_party/ppapi/c/pp_rect.h:15: } PP_Rect;
nit: perhaps doesn't need to be done now, but we'll need Point and Size (i use
them for theming api)

http://codereview.chromium.org/1697008/diff/77052/137028
File third_party/ppapi/c/ppb_device_context_2d.h (right):

http://codereview.chromium.org/1697008/diff/77052/137028#newcode29
third_party/ppapi/c/ppb_device_context_2d.h:29: // A 0-sized dirty rect means to
paint the entire ImageData (do not use a
I'm curious as to why you chose this?  It seems confusing.

http://codereview.chromium.org/1697008/diff/77052/137020
File third_party/ppapi/example/example.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137020#newcode1
third_party/ppapi/example/example.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
should this example get checked in with the C++ API, since that's what it uses?

[darin (slow to review), 2010-05-05 23:43:07]

looks really great overall.  just minor suggestions:

http://codereview.chromium.org/1697008/diff/77052/137016
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137016#newcode41
chrome/renderer/pepper_plugin_delegate_impl.cc:41: 
nit: extra new line

http://codereview.chromium.org/1697008/diff/77052/137029
File third_party/ppapi/c/ppb_var.h (right):

http://codereview.chromium.org/1697008/diff/77052/137029#newcode38
third_party/ppapi/c/ppb_var.h:38: PP_Var (*VarFromUtf8)(const char* data,
uint32_t len);
i'm a fan of len == -1 allows data to be null terminated so you can easily do
VarFromUtf8("foo bar", -1), but i guess i don't care too much as the C++ layer
can provide for such convenience.

http://codereview.chromium.org/1697008/diff/77052/137029#newcode83
third_party/ppapi/c/ppb_var.h:83: //   Dom_Call(obj, "DoIt") = obj.DoIt() in
JavaScript.
nit: Dom_Call

http://codereview.chromium.org/1697008/diff/77052/137029#newcode105
third_party/ppapi/c/ppb_var.h:105: // count reached 0, the class' Destruct
function wlil be called.
Destruct -> Deallocate, right?  same goes for the Destruct comments below.

http://codereview.chromium.org/1697008/diff/77052/137029#newcode130
third_party/ppapi/c/ppb_var.h:130: 
nit: extra new lines

http://codereview.chromium.org/1697008/diff/77052/137041
File third_party/ppapi/c/ppp_class.h (right):

http://codereview.chromium.org/1697008/diff/77052/137041#newcode13
third_party/ppapi/c/ppp_class.h:13: // to this interface to
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX.
PPB_Var::CreateObject

http://codereview.chromium.org/1697008/diff/77052/137020
File third_party/ppapi/example/example.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137020#newcode74
third_party/ppapi/example/example.cc:74: printf("Paint image data\n");
nit: indentation

http://codereview.chromium.org/1697008/diff/77052/137014
File webkit/glue/plugins/pepper_image_data.h (right):

http://codereview.chromium.org/1697008/diff/77052/137014#newcode35
webkit/glue/plugins/pepper_image_data.h:35: ImageData* GetAsImageData() { return
this; }
AsImageData?

http://codereview.chromium.org/1697008/diff/77052/137019
File webkit/glue/plugins/pepper_plugin_delegate.h (right):

http://codereview.chromium.org/1697008/diff/77052/137019#newcode37
webkit/glue/plugins/pepper_plugin_delegate.h:37: 
nit: extra new line

http://codereview.chromium.org/1697008/diff/77052/137007
File webkit/glue/plugins/pepper_plugin_instance.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137007#newcode62
webkit/glue/plugins/pepper_plugin_instance.cc:62: void BuildKeyEvent(const
WebInputEvent* event, PP_Event* npevent) {
nit: npevent ;)

http://codereview.chromium.org/1697008/diff/77052/137007#newcode69
webkit/glue/plugins/pepper_plugin_instance.cc:69: void BuildCharEvent(const
WebInputEvent* event, PP_Event* npevent) {
nit: npevent

http://codereview.chromium.org/1697008/diff/77052/137007#newcode83
webkit/glue/plugins/pepper_plugin_instance.cc:83: void BuildMouseEvent(const
WebInputEvent* event, PP_Event* npevent) {
nit: npevent

http://codereview.chromium.org/1697008/diff/77052/137007#newcode93
webkit/glue/plugins/pepper_plugin_instance.cc:93: void
BuildMouseWheelEvent(const WebInputEvent* event, PP_Event* npevent) {
nit: npevent

http://codereview.chromium.org/1697008/diff/77052/137007#newcode219
webkit/glue/plugins/pepper_plugin_instance.cc:219: //case
PP_Event_Type_Minimize:
nit: delete these comments

http://codereview.chromium.org/1697008/diff/77052/137012
File webkit/glue/plugins/pepper_plugin_instance.h (right):

http://codereview.chromium.org/1697008/diff/77052/137012#newcode18
webkit/glue/plugins/pepper_plugin_instance.h:18: typedef struct _pp_Resource
PP_Resource;
nit: group _pp_* together

http://codereview.chromium.org/1697008/diff/77052/137018
File webkit/glue/plugins/pepper_plugin_module.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137018#newcode38
webkit/glue/plugins/pepper_plugin_module.cc:38: typedef std::set<PluginModule*>
PluginModuleSet;
perhaps a static function that returns a pointer to a function static variable?

http://codereview.chromium.org/1697008/diff/77052/137018#newcode41
webkit/glue/plugins/pepper_plugin_module.cc:41: // PPB_Core
--------------------------------------------------------------------
maybe this should be in a separate source file as it may grow over time

http://codereview.chromium.org/1697008/diff/77052/137018#newcode185
webkit/glue/plugins/pepper_plugin_module.cc:185: NOTREACHED();
LOG(ERROR) or CHECK(false) ?

http://codereview.chromium.org/1697008/diff/77052/137022
File webkit/glue/plugins/pepper_plugin_module.h (right):

http://codereview.chromium.org/1697008/diff/77052/137022#newcode32
webkit/glue/plugins/pepper_plugin_module.h:32: PP_Module GetModule() const;
should this be GetPPModule to match FromPPModule?

http://codereview.chromium.org/1697008/diff/77052/137008
File webkit/glue/plugins/pepper_resource.h (right):

http://codereview.chromium.org/1697008/diff/77052/137008#newcode26
webkit/glue/plugins/pepper_resource.h:26: static PP_Resource GetNullResource();
how about just naming this method Null?

pepper::Resource::Null() is short and sweet. or pepper::NullResource(). v8 has
v8::Null().

I also think that this could be implemented inline since the implementation is
super simple (equivalent to return 0).

http://codereview.chromium.org/1697008/diff/77052/137008#newcode28
webkit/glue/plugins/pepper_resource.h:28: PP_Resource GetResource() const;
resource->GetResource() seems a bit awkward.  Would it make sense to call this
the value of a Resource?  Or might we want to just use the generic term "get"
for this?

I also think that this could be implemented inline since the implementation is
super simple (equivalent to return this).

I see the same issue with Module and PluginInstance.  In those cases, the .cc
file uses GetModule and GetInstance, in which case the naming definitely helps. 
Maybe my suggestions here should be ignored then.  Or maybe GetNullResource ->
GetNullPPResource and GetResource -> GetPPResource :-/

http://codereview.chromium.org/1697008/diff/77052/137008#newcode34
webkit/glue/plugins/pepper_resource.h:34: virtual DeviceContext2D*
GetAsDeviceContext2D() { return NULL; }
nit: drop the Get prefix?  resource->AsDeviceContext2D() reads nicely and we use
that convention elsewhere.

http://codereview.chromium.org/1697008/diff/77052/137009
File webkit/glue/plugins/pepper_resource_tracker.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137009#newcode15
webkit/glue/plugins/pepper_resource_tracker.cc:15: namespace {
nit: delete empty namespace

http://codereview.chromium.org/1697008/diff/77052/137009#newcode27
webkit/glue/plugins/pepper_resource_tracker.cc:27: return
Singleton<ResourceTracker>::get();
does this need to be threadsafe?  (nothing else is threadsafe here.)  can it
just be a function static?

http://codereview.chromium.org/1697008/diff/77052/137009#newcode45
webkit/glue/plugins/pepper_resource_tracker.cc:45: NOTREACHED();
DLOG(ERROR) or CHECK(false)?

http://codereview.chromium.org/1697008/diff/77052/137010
File webkit/glue/plugins/pepper_string.h (right):

http://codereview.chromium.org/1697008/diff/77052/137010#newcode20
webkit/glue/plugins/pepper_string.h:20: const std::string& str() const { return
str_; }
nit: str -> value ?

otherwise, you end up with str.str() since a String variable might reasonably be
named str.

http://codereview.chromium.org/1697008/diff/77052/137010#newcode22
webkit/glue/plugins/pepper_string.h:22: const char* Get(uint32* len) const {
it seems like this should just be defined in VarToUtf8 since the subtlety of the
behavior here is specific to that function.

http://codereview.chromium.org/1697008/diff/77052/137026
File webkit/glue/plugins/pepper_var.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137026#newcode19
webkit/glue/plugins/pepper_var.cc:19: 
nit: add a TODO comment

http://codereview.chromium.org/1697008/diff/77052/137026#newcode27
webkit/glue/plugins/pepper_var.cc:27: 
nit: add a TODO comment

http://codereview.chromium.org/1697008/diff/77052/137013
File webkit/glue/plugins/pepper_webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137013#newcode92
webkit/glue/plugins/pepper_webplugin_delegate_impl.cc:92: return -1;  //
FIXME(brettw)
nit: FIXME -> TODO

http://codereview.chromium.org/1697008/diff/77052/137006
File webkit/glue/plugins/pepper_webplugin_delegate_impl.h (right):

http://codereview.chromium.org/1697008/diff/77052/137006#newcode72
webkit/glue/plugins/pepper_webplugin_delegate_impl.h:72: 
nit: extra new line

[brettw, 2010-05-06 17:08:08]

I think I addressed the rest of the comments. Thanks guys.

http://codereview.chromium.org/1697008/diff/77052/137015
File chrome/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/1697008/diff/77052/137015#newcode22
chrome/renderer/pepper_plugin_delegate_impl.h:22: RenderView* render_view_;
On 2010/05/05 23:24:52, John Abd-El-Malek wrote:
> nit: perhaps this should be a WeakPtr?

In my change to RenderView to hook this up, I just made this a member, so
there's no lifetime ambiguity that would be solved by a weak ptr (this is why
the comment says the RenderView owns us).

http://codereview.chromium.org/1697008/diff/77052/137033
File third_party/ppapi/c/pp_instance.h (right):

http://codereview.chromium.org/1697008/diff/77052/137033#newcode15
third_party/ppapi/c/pp_instance.h:15: } PP_Instance;
I was trying to be consistent with one struct/interface per header which had the
same name, which I'm hoping will make stuff easier to find (like WebKit does).

http://codereview.chromium.org/1697008/diff/77052/137028
File third_party/ppapi/c/ppb_device_context_2d.h (right):

http://codereview.chromium.org/1697008/diff/77052/137028#newcode29
third_party/ppapi/c/ppb_device_context_2d.h:29: // A 0-sized dirty rect means to
paint the entire ImageData (do not use a
On 2010/05/05 23:24:52, John Abd-El-Malek wrote:
> I'm curious as to why you chose this?  It seems confusing.

This was really for the C++ API so you can pass pp::Rect() as this parameter to
get the whole thing updated (which will be the normal case for most plugins).

http://codereview.chromium.org/1697008/diff/77052/137020
File third_party/ppapi/example/example.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137020#newcode1
third_party/ppapi/example/example.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2010/05/05 23:24:52, John Abd-El-Malek wrote:
> should this example get checked in with the C++ API, since that's what it
uses?

Yes, I moved it.

http://codereview.chromium.org/1697008/diff/77052/137018
File webkit/glue/plugins/pepper_plugin_module.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137018#newcode41
webkit/glue/plugins/pepper_plugin_module.cc:41: // PPB_Core
--------------------------------------------------------------------
I'm actually not sure what else will get added here so I'd prefer to just move
it later if it gets larger.

http://codereview.chromium.org/1697008/diff/77052/137008
File webkit/glue/plugins/pepper_resource.h (right):

http://codereview.chromium.org/1697008/diff/77052/137008#newcode26
webkit/glue/plugins/pepper_resource.h:26: static PP_Resource GetNullResource();
I made this pepper::NullPPResource().

http://codereview.chromium.org/1697008/diff/77052/137008#newcode28
webkit/glue/plugins/pepper_resource.h:28: PP_Resource GetResource() const;
On 2010/05/05 23:43:07, darin wrote:
> resource->GetResource() seems a bit awkward.  Would it make sense to call this
> the value of a Resource?  Or might we want to just use the generic term "get"
> for this?
> 
> I also think that this could be implemented inline since the implementation is
> super simple (equivalent to return this).

I didn't do inline since the header file doesn't have to include the definition
of PP_Instance otherwise.

> I see the same issue with Module and PluginInstance.  In those cases, the .cc
> file uses GetModule and GetInstance, in which case the naming definitely
helps. 
> Maybe my suggestions here should be ignored then.  Or maybe GetNullResource ->
> GetNullPPResource and GetResource -> GetPPResource :-/

I standardized on GetPP*

http://codereview.chromium.org/1697008/diff/77052/137009
File webkit/glue/plugins/pepper_resource_tracker.cc (right):

http://codereview.chromium.org/1697008/diff/77052/137009#newcode27
webkit/glue/plugins/pepper_resource_tracker.cc:27: return
Singleton<ResourceTracker>::get();
On 2010/05/05 23:43:07, darin wrote:
> does this need to be threadsafe?  (nothing else is threadsafe here.)  can it
> just be a function static?

You need to be able to do AddRef and Release of resources from other threads. I
made the rest of this object threadsafe. We will need to take a close look at
the entire API to make sure that either everything is threadsafe or we return
errors if things are called on the wrong thread.

http://codereview.chromium.org/1697008/diff/77052/137009#newcode45
webkit/glue/plugins/pepper_resource_tracker.cc:45: NOTREACHED();
I think it makes sense to keep this as a debug assert like it is now. Hitting
this is a programming error on our part in the renderer, rather than a misuse of
the pepper API.

[darin (slow to review), 2010-05-06 18:20:16]

On Thu, May 6, 2010 at 10:08 AM, <brettw@chromium.org> wrote:

> I think I addressed the rest of the comments. Thanks guys.
>
>
>
> http://codereview.chromium.org/1697008/diff/77052/137015
> File chrome/renderer/pepper_plugin_delegate_impl.h (right):
>
> http://codereview.chromium.org/1697008/diff/77052/137015#newcode22
> chrome/renderer/pepper_plugin_delegate_impl.h:22: RenderView*
> render_view_;
> On 2010/05/05 23:24:52, John Abd-El-Malek wrote:
>
>> nit: perhaps this should be a WeakPtr?
>>
>
> In my change to RenderView to hook this up, I just made this a member,
> so there's no lifetime ambiguity that would be solved by a weak ptr
> (this is why the comment says the RenderView owns us).
>
>
> http://codereview.chromium.org/1697008/diff/77052/137033
> File third_party/ppapi/c/pp_instance.h (right):
>
> http://codereview.chromium.org/1697008/diff/77052/137033#newcode15
> third_party/ppapi/c/pp_instance.h:15: } PP_Instance;
> I was trying to be consistent with one struct/interface per header which
> had the same name, which I'm hoping will make stuff easier to find (like
> WebKit does).
>
>
> http://codereview.chromium.org/1697008/diff/77052/137028
> File third_party/ppapi/c/ppb_device_context_2d.h (right):
>
> http://codereview.chromium.org/1697008/diff/77052/137028#newcode29
> third_party/ppapi/c/ppb_device_context_2d.h:29: // A 0-sized dirty rect
> means to paint the entire ImageData (do not use a
> On 2010/05/05 23:24:52, John Abd-El-Malek wrote:
>
>> I'm curious as to why you chose this?  It seems confusing.
>>
>
> This was really for the C++ API so you can pass pp::Rect() as this
> parameter to get the whole thing updated (which will be the normal case
> for most plugins).
>
>
> http://codereview.chromium.org/1697008/diff/77052/137020
> File third_party/ppapi/example/example.cc (right):
>
> http://codereview.chromium.org/1697008/diff/77052/137020#newcode1
> third_party/ppapi/example/example.cc:1: // Copyright (c) 2010 The
> Chromium Authors. All rights reserved.
> On 2010/05/05 23:24:52, John Abd-El-Malek wrote:
>
>> should this example get checked in with the C++ API, since that's what
>>
> it uses?
>
> Yes, I moved it.
>
>
> http://codereview.chromium.org/1697008/diff/77052/137018
> File webkit/glue/plugins/pepper_plugin_module.cc (right):
>
> http://codereview.chromium.org/1697008/diff/77052/137018#newcode41
> webkit/glue/plugins/pepper_plugin_module.cc:41: // PPB_Core
> --------------------------------------------------------------------
> I'm actually not sure what else will get added here so I'd prefer to
> just move it later if it gets larger.
>
>
> http://codereview.chromium.org/1697008/diff/77052/137008
> File webkit/glue/plugins/pepper_resource.h (right):
>
> http://codereview.chromium.org/1697008/diff/77052/137008#newcode26
> webkit/glue/plugins/pepper_resource.h:26: static PP_Resource
> GetNullResource();
> I made this pepper::NullPPResource().
>
>
> http://codereview.chromium.org/1697008/diff/77052/137008#newcode28
> webkit/glue/plugins/pepper_resource.h:28: PP_Resource GetResource()
> const;
> On 2010/05/05 23:43:07, darin wrote:
>
>> resource->GetResource() seems a bit awkward.  Would it make sense to
>>
> call this
>
>> the value of a Resource?  Or might we want to just use the generic
>>
> term "get"
>
>> for this?
>>
>
>  I also think that this could be implemented inline since the
>>
> implementation is
>
>> super simple (equivalent to return this).
>>
>
> I didn't do inline since the header file doesn't have to include the
> definition of PP_Instance otherwise.
>
>
>  I see the same issue with Module and PluginInstance.  In those cases,
>>
> the .cc
>
>> file uses GetModule and GetInstance, in which case the naming
>>
> definitely helps.
>
>> Maybe my suggestions here should be ignored then.  Or maybe
>>
> GetNullResource ->
>
>> GetNullPPResource and GetResource -> GetPPResource :-/
>>
>
> I standardized on GetPP*
>
>
> http://codereview.chromium.org/1697008/diff/77052/137009
> File webkit/glue/plugins/pepper_resource_tracker.cc (right):
>
> http://codereview.chromium.org/1697008/diff/77052/137009#newcode27
> webkit/glue/plugins/pepper_resource_tracker.cc:27: return
> Singleton<ResourceTracker>::get();
> On 2010/05/05 23:43:07, darin wrote:
>
>> does this need to be threadsafe?  (nothing else is threadsafe here.)
>>
> can it
>
>> just be a function static?
>>
>
> You need to be able to do AddRef and Release of resources from other
> threads. I made the rest of this object threadsafe. We will need to take
> a close look at the entire API to make sure that either everything is
> threadsafe or we return errors if things are called on the wrong thread.
>
>
> http://codereview.chromium.org/1697008/diff/77052/137009#newcode45
> webkit/glue/plugins/pepper_resource_tracker.cc:45: NOTREACHED();
> I think it makes sense to keep this as a debug assert like it is now.
> Hitting this is a programming error on our part in the renderer, rather
> than a misuse of the pepper API.
>
>
Is there any need to handle the error in release builds?
-Darin



>
> http://codereview.chromium.org/1697008/show
>

[darin (slow to review), 2010-05-06 18:58:30]

LGTM

http://codereview.chromium.org/1697008/diff/184001/161003
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/1697008/diff/184001/161003#newcode40
chrome/renderer/pepper_plugin_delegate_impl.cc:40:
PepperPluginDelegateImpl::PepperPluginDelegateImpl(RenderView* render_view) {
i guess you meant to assign render_view_ here.

http://codereview.chromium.org/1697008/diff/184001/161013
File third_party/ppapi/c/ppb_core.h (right):

http://codereview.chromium.org/1697008/diff/184001/161013#newcode23
third_party/ppapi/c/ppb_core.h:23: //PPB_ScheduleTimer ScheduleTimer;
I think we should compact all of this down into a single call that has an
optional delay parameter.  We should also provide a GetCurrentTime function.

http://codereview.chromium.org/1697008/diff/184001/161032
File webkit/glue/plugins/pepper_resource.cc (right):

http://codereview.chromium.org/1697008/diff/184001/161032#newcode30
webkit/glue/plugins/pepper_resource.cc:30: 
nit: extra new lines

[tfarina (gmail-do not use), 2010-05-07 20:36:05]

http://codereview.chromium.org/1697008/diff/211001/212009
File webkit/glue/plugins/pepper_image_data.h (right):

http://codereview.chromium.org/1697008/diff/211001/212009#newcode27
webkit/glue/plugins/pepper_image_data.h:27: ImageData(PluginModule* module);
nit: explicit

http://codereview.chromium.org/1697008/diff/211001/212014
File webkit/glue/plugins/pepper_plugin_module.h (right):

http://codereview.chromium.org/1697008/diff/211001/212014#newcode13
webkit/glue/plugins/pepper_plugin_module.h:13: #include <set>
nit: move above basictypes.h ?

http://codereview.chromium.org/1697008/diff/211001/212014#newcode53
webkit/glue/plugins/pepper_plugin_module.h:53: PluginModule(const FilePath&
filename);
nit: explicit