Clean up Pepper ImageData resource class.

Clean up Pepper ImageData resource class.

-This change introduces an abstract ImageData class, and two subclasses.
-PlatformImageData allows access to the platform-specific canvas and handles.
-SimpleImageData is platform independent, suitable for use in the NaCl proxy.
-Also changes ImageData creation messages to use PP_ImageDataDesc instead of
 serializing to std::string.
-Remove NaCl-isms in naming.

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

[bbudge, 2013-06-07 22:52:08]

The only potential pitfall is that the cache may mix up the two types of
ImageData. I'm not sure if it's possible or not.

[bbudge, 2013-06-07 22:56:03]

+Justin for ppapi_messages.h

[bbudge, 2013-06-07 23:01:06]

On 2013/06/07 22:56:03, bbudge1 wrote:
> +Justin for ppapi_messages.h

David and Justin, hold off on reviewing. I think I'll upload another patch to
remove some NaCl-isms so this will be more useable.

[bbudge, 2013-06-08 22:00:13]

Ready for review.

[dmichael (off chromium), 2013-06-10 18:07:29]

Good point about the image data cache. I think we should at least add some
protection against it. Maybe add "IsPlatform" and "IsSimple" virtual functions.
If you want to just CHECK or DCHECK now that it's the expected kind (Platform
for trusted, Simple for NaCl?), that would be good. Otherwise, if you add
caching for this API and forget to fix the caching issue, the trusted case will
be messed up.

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/ppb_image_dat...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:492: #if defined(OS_NACL)
I think you want to change to checking platform at runtime now, right? And
probably compile out the "PlatformImageData" case in NaCl?

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/ppb_image_dat...
File ppapi/proxy/ppb_image_data_proxy.h (right):

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.h:102: // SimpleImageData is a simple, platform
independent image data resource which
teeny nit: I would hyphenate platform-independent.

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.h:155: bool platform,
suggestion: you could use an enum for this, to make call sites more
self-documenting.

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/video_source_...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:114: (new PlatformImageData(image_data,
image_desc, fd))->GetReference();
Does this case still make sense with this CL? I thought you'd make a
PlatformImageSimple and always use the handle from reply_params now?

(Or are you saving that for another CL?)

https://codereview.chromium.org/16605006/diff/27002/ppapi/thunk/ppb_image_dat...
File ppapi/thunk/ppb_image_data_thunk.cc (right):

https://codereview.chromium.org/16605006/diff/27002/ppapi/thunk/ppb_image_dat...
ppapi/thunk/ppb_image_data_thunk.cc:45: init_to_zero);
I think here you'll need to do conditional compilation to make NaCl use
CreateImageDataSimple?

This also brings up the question of how the plugin draws frames. Does it use
what it's given, or does it have to create a new one? If it's creating a new
one, in the trusted case, it will be creating & sending "platform" image datas,
while it is receiving "simple" ones. I don't actually see any harm in that, but
it is a little odd.

[dmichael (off chromium), 2013-06-10 18:11:18]

(PS I should say thanks for doing this; it looks really nice overall. I should
have written it that way to begin with!)

[bbudge, 2013-06-10 23:28:37]

Now that we have an enum to define the ImageData type, I added a type() getter
so we can match types when looking in the cache.

Also, it seemed reasonable to change CreateImageDataPlatform / Simple to a
single CreateImage method in the resource API. It takes the enum param for type.

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/ppb_image_dat...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:492: #if defined(OS_NACL)
On 2013/06/10 18:07:29, dmichael wrote:
> I think you want to change to checking platform at runtime now, right? And
> probably compile out the "PlatformImageData" case in NaCl?

Good catch.

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/ppb_image_dat...
File ppapi/proxy/ppb_image_data_proxy.h (right):

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.h:102: // SimpleImageData is a simple, platform
independent image data resource which
On 2013/06/10 18:07:29, dmichael wrote:
> teeny nit: I would hyphenate platform-independent.

Done.

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.h:155: bool platform,
On 2013/06/10 18:07:29, dmichael wrote:
> suggestion: you could use an enum for this, to make call sites more
> self-documenting.

Done. Moved the one in PPB_ImageData_Impl to PPB_ImageData_Shared.

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/video_source_...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/16605006/diff/27002/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:114: (new PlatformImageData(image_data,
image_desc, fd))->GetReference();
Right, there's a follow on CL that exposes the MediaStream APIs for NaCl. (I'm
also planning to clean up Raymes' PDF resource/host as well.

On 2013/06/10 18:07:29, dmichael wrote:
> Does this case still make sense with this CL? I thought you'd make a
> PlatformImageSimple and always use the handle from reply_params now?
> 
> (Or are you saving that for another CL?)

https://codereview.chromium.org/16605006/diff/27002/ppapi/thunk/ppb_image_dat...
File ppapi/thunk/ppb_image_data_thunk.cc (right):

https://codereview.chromium.org/16605006/diff/27002/ppapi/thunk/ppb_image_dat...
ppapi/thunk/ppb_image_data_thunk.cc:45: init_to_zero);
Done.

Hopefully it will be a NaCl plugin that actually ships.

[dmichael (off chromium), 2013-06-11 17:55:34]

lgtm (looks great to me!), thanks!

https://codereview.chromium.org/16605006/diff/49001/ppapi/proxy/ppb_image_dat...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://codereview.chromium.org/16605006/diff/49001/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:525: #endif
#else
NOTREACHED();
#endif
(or NOTIMPLEMENTED)?
It should have no effect in release builds, so I think it might be a little
clearer and will catch us if we do something stupid.

[bbudge, 2013-06-11 18:13:01]

https://codereview.chromium.org/16605006/diff/49001/ppapi/proxy/ppb_image_dat...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://codereview.chromium.org/16605006/diff/49001/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:525: #endif
On 2013/06/11 17:55:34, dmichael wrote:
> #else
> NOTREACHED();
> #endif
> (or NOTIMPLEMENTED)?
> It should have no effect in release builds, so I think it might be a little
> clearer and will catch us if we do something stupid.

I'll go with NOTREACHED since NOTIMPLEMENTED implies that it might someday be
implemented. Which it won't in this case. Done.

[commit-bot: I haz the power, 2013-06-11 18:13:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/16605006/83001

[commit-bot: I haz the power, 2013-06-11 18:25:31]

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

[jschuh, 2013-06-11 19:03:04]

The attack surface isn't changing, so lgtm. But do me a favor and file a bug to
add some basic sanity checking on the types in ppapi_messages.h. Things like
having a positive stride and size should really be validated at the ipc layer.

[commit-bot: I haz the power, 2013-06-11 20:06:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/16605006/83001

[commit-bot: I haz the power, 2013-06-11 20:17:59]

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

[bbudge, 2013-06-11 20:26:12]

https://code.google.com/p/chromium/issues/detail?id=180103

+raymes for OWNERS in content/renderer/pepper

[raymes, 2013-06-11 20:31:57]

OWNERS rubberstamp for content/renderer/pepper

On Tue, Jun 11, 2013 at 1:26 PM,  <bbudge@chromium.org> wrote:
> https://code.google.com/p/chromium/issues/detail?id=180103
>
> +raymes for OWNERS in content/renderer/pepper
>
> https://codereview.chromium.org/16605006/

[raymes, 2013-06-11 20:32:06]

lgtm

On Tue, Jun 11, 2013 at 1:31 PM, Raymes Khoury <raymes@chromium.org> wrote:
> OWNERS rubberstamp for content/renderer/pepper
>
> On Tue, Jun 11, 2013 at 1:26 PM,  <bbudge@chromium.org> wrote:
>> https://code.google.com/p/chromium/issues/detail?id=180103
>>
>> +raymes for OWNERS in content/renderer/pepper
>>
>> https://codereview.chromium.org/16605006/

[commit-bot: I haz the power, 2013-06-11 20:34:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/16605006/83001

[commit-bot: I haz the power, 2013-06-12 00:18:17]

Change committed as 205681