PPAPI/NaCl: Make ImageData for NaCl just use shared memory

PPAPI/NaCl: Make ImageData for NaCl just use shared memory

BUG=116317
TEST=
TBR=ben@chromium.org
ben for ui/gfx OWNERS

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

[dmichael (off chromium), 2012-07-20 18:24:04]

This works locally for Graphics2D, if I hack in some changes to
nacl_ipc_adapter.cc (early/ugly form here:
http://codereview.chromium.org/10796064/ )

http://codereview.chromium.org/10790063/diff/12001/ppapi/proxy/ppb_buffer_pro...
File ppapi/proxy/ppb_buffer_proxy.h (left):

http://codereview.chromium.org/10790063/diff/12001/ppapi/proxy/ppb_buffer_pro...
ppapi/proxy/ppb_buffer_proxy.h:39: void* mapped_data_;
Not really related to this CL, but it turns out that this is never set, and
IsMapped won't work.

http://codereview.chromium.org/10790063/diff/12001/ui/gfx/blit.cc
File ui/gfx/blit.cc (right):

http://codereview.chromium.org/10790063/diff/12001/ui/gfx/blit.cc#newcode144
ui/gfx/blit.cc:144: }
Previously, Windows assumed that the canvas was always a PlatformCanvas. Now,
that won't be true for canvases created from NaCl apps, so we need to be able to
fall back to software scrolling.

[brettw, 2012-07-20 21:57:04]

+Mike Reed for the bottom of ppb_image_data_impl.cc (ImageDataNaClDelegate).

We're implementing a SkBitmap backed by a random shared memory thingy. I'm not
sure if the current usage of unlock pixels is correct when we free the memory.
Can you check this?

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
File webkit/plugins/ppapi/ppb_image_data_impl.cc (right):

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.cc:142: // ImageDataPlatformDelegate
--------------------------------------------------
I'd do a blank line after this.

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.cc:251: // TODO(dmichael): Is
lockPixels necessary? We own the pixels:
I'm pretty sure it isn't necessary. I think assigning the pixels gives you an
implicit lock.

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.cc:262: skia_bitmap_.unlockPixels();
I'm going to add Mike Reed to check if this is correct.

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
File webkit/plugins/ppapi/ppb_image_data_impl.h (right):

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.h:50: // Use this to create an
ImageData for use with NaCl. This is backed by a
I'd probably put a blank line here.

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.h:63: // Resource override.
I'm not sure why you moved this.

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.h:68: WEBKIT_PLUGINS_EXPORT bool
IsMapped() const;
I'd probably just annotate this whole class as exported rather than the several
functions you've done.

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.h:84: // We delegate most of our
implementation to a class that either uses a
Since this is a typedef it should be first (before the constructors).

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.h:87: class Delegate {
I don't really feel like this is a "delegate" the way we normally use it.
Normally I'd expect a delegate to be passed to the constructor to allow
something to talk to the embedder. Here, it seems more like a "Backend" or
something.

[brettw, 2012-07-20 22:02:28]

http://codereview.chromium.org/10790063/diff/14001/ppapi/proxy/ppb_image_data...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

http://codereview.chromium.org/10790063/diff/14001/ppapi/proxy/ppb_image_data...
ppapi/proxy/ppb_image_data_proxy.cc:251: #if defined(OS_NACL)
This seems weird, does this code do anything?

[dmichael (off chromium), 2012-07-20 22:32:39]

http://codereview.chromium.org/10790063/diff/14001/ppapi/proxy/ppb_image_data...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

http://codereview.chromium.org/10790063/diff/14001/ppapi/proxy/ppb_image_data...
ppapi/proxy/ppb_image_data_proxy.cc:251: #if defined(OS_NACL)
On 2012/07/20 22:02:28, brettw wrote:
> This seems weird, does this code do anything?
It does, but only in the trusted side. The message should never be received in
untrusted code, so I compile out the implementation. We probably want a better,
more general approach to minimizing the size of the IRT...  but I thought this
would be fine for now. I added a short comment to explain.

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
File webkit/plugins/ppapi/ppb_image_data_impl.cc (right):

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.cc:142: // ImageDataPlatformDelegate
--------------------------------------------------
On 2012/07/20 21:57:04, brettw wrote:
> I'd do a blank line after this.

Done.

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.cc:251: // TODO(dmichael): Is
lockPixels necessary? We own the pixels:
On 2012/07/20 21:57:04, brettw wrote:
> I'm pretty sure it isn't necessary. I think assigning the pixels gives you an
> implicit lock.
Okay, I'll take the lock and unlock out until we hear back from an expert.

By my reading, it's irrelevant; I think when you provide the backing memory (via
setPixels), that there's no SkPixelRef, so locking is a no-op. But I would love
to have someone confirm or deny :-)

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
File webkit/plugins/ppapi/ppb_image_data_impl.h (right):

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.h:63: // Resource override.
On 2012/07/20 21:57:04, brettw wrote:
> I'm not sure why you moved this.
Just a casualty of lots of churn in the file. Moved back.

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.h:68: WEBKIT_PLUGINS_EXPORT bool
IsMapped() const;
On 2012/07/20 21:57:04, brettw wrote:
> I'd probably just annotate this whole class as exported rather than the
several
> functions you've done.
Done.

http://codereview.chromium.org/10790063/diff/14001/webkit/plugins/ppapi/ppb_i...
webkit/plugins/ppapi/ppb_image_data_impl.h:87: class Delegate {
On 2012/07/20 21:57:04, brettw wrote:
> I don't really feel like this is a "delegate" the way we normally use it.
> Normally I'd expect a delegate to be passed to the constructor to allow
> something to talk to the embedder. Here, it seems more like a "Backend" or
> something.
Yeah...  I was thinking of it as "pImpl" pattern, but "Impl" was taken. I agree
it's not the usual Chrome-style delegate. "Backend" WFM.

[reed1, 2012-07-21 16:58:02]

Not sure what all the constraints/motivations are for this change, but I don't
see any red-flags.

Do we really have to expose PlatformCanvas? We are hoping to deprecate and then
remove that class, in favor of just SkCanvas...

[brettw, 2012-07-21 18:06:54]

The main this this is used for is passing into WebFont and the WebKit theme
drawing code, and I think this requires a platform canvas today.

This is an implementation detail. If we can draw text and themes later without
this, we can change it.

[dmichael (off chromium), 2012-07-23 16:31:47]

On 2012/07/21 16:58:02, reed1 wrote:
> Not sure what all the constraints/motivations are for this change, but I don't
> see any red-flags.
> 
> Do we really have to expose PlatformCanvas? We are hoping to deprecate and
then
> remove that class, in favor of just SkCanvas...
FWIW, this patch reduces our usage of PlatformCanvas a bit.  See, e.g.:
http://codereview.chromium.org/10790063/diff/12004/webkit/plugins/ppapi/ppb_g...

I thought about eliminating PPB_ImageData_API::GetPlatformCanvas, but as Brett
notes, it seems that we still need to use it in one or two places (e.g., font
rendering), so I think I would actually have to still down-cast to
PlatformCanvas in at least one spot (that's my recollection from coding down
that path a short ways).

Definitely keep us posted about the transition...  some of our code could get a
fair bit simpler if we could avoid using PlatformCanvas.

[brettw, 2012-07-23 17:48:31]

lgtm

[commit-bot: I haz the power, 2012-07-23 19:33:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10790063/12004

[commit-bot: I haz the power, 2012-07-23 19:33:18]

Presubmit check for 10790063-12004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/gfx

Presubmit checks took 1.8s to calculate.

[brettw, 2012-07-23 19:34:28]

I think you can TBR the ui/gfx thing when you find a reviewer.

[dmichael (off chromium), 2012-07-23 19:38:31]

+ben (TBR) for ui/gfx OWNERS

[commit-bot: I haz the power, 2012-07-23 19:38:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10790063/12004

[commit-bot: I haz the power, 2012-07-23 19:51:49]

Try job failure for 10790063-12004 (retry) on win for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-07-23 20:56:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10790063/7006

[commit-bot: I haz the power, 2012-07-23 22:08:50]

Try job failure for 10790063-7006 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[bbudge, 2012-07-23 22:15:17]

On 2012/07/23 22:08:50, I haz the power (commit-bot) wrote:
> Try job failure for 10790063-7006 (retry) on win for step "compile" (clobber
> build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

I think you can fix the Windows compile failure by annotating the
PPB_ImageData_Api class with 'PPAPI_THUNK_EXPORT'.

[brettw, 2012-07-23 22:36:52]

There's no reason to export it since it's just a virtual class. It should be
sufficient to mark it a NON_EXPORTED_BASE (grep for examples).

[commit-bot: I haz the power, 2012-07-25 18:54:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10790063/17001

[commit-bot: I haz the power, 2012-07-25 20:14:17]

Try job failure for 10790063-17001 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-25 20:28:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10790063/17001

[commit-bot: I haz the power, 2012-07-25 21:54:50]

Change committed as 148427