Add Skia to CG adapter for plugins.

Add Skia to CG adapter for plugins.

Skia on Mac uses Skia to render WebKit, and CG
to render UI elements. The CG elements need a
transcribed graphics context that preserves the
canvas matrix, and the canvas clip.

The SkiaBitLocker utility class sets up a CGContext
from the SkCanvas, locks the bitmap's bits, and
releases the lock when the class goes out of scope.

The SkiaBitLocker implementation is described by
this patch:
http://codereview.chromium.org/7031006/

For ppapi, use the CG bitmap copy only if Skia
is not used.

Otherwise, adjust the Mac-specific code to convert
the canvas to a CGContext as needed.

Also, add a CG-specific entry point for painting
so that the delegate can paint using a WebCanvas,
while the proxy paints with a CGContext.

This change adds a code path that will be enabled
in the future, but does not modify any
existing code, so there is no functional change.

BUG=79463
TEST=none

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

[stuartmorgan, 2011-04-12 17:35:51]

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/npapi/webplugin_...
File webkit/plugins/npapi/webplugin_delegate_impl_mac.mm (right):

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/npapi/webplugin_...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:478: void
WebPluginDelegateImpl::Paint(WebKit::WebCanvas* canvas,
I don't suppose there's a less confusing way to do these defines? Having the #if
wrapping one function in the header and the other in the implementation looks
like a mistake; I had to read it a few times to understand that it wasn't.

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/npapi/webplugin_...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:480: gfx::SkiaBitLocker
bitLocker(canvas);
Does this have the same transparency-preserving properties that the CG version
has, so that transparent plugins will composite correctly without the background
store?

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppapi_plug...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppapi_plug...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1220: gfx::SkiaBitLocker
bitLocker(canvas);
Indentation is wrong on this whole block

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppapi_plug...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1395: CGContextRef cgContext =
bitLocker.cgContext();
Is round-tripping through CG actually the best way to render an SkBitmap to a
Skia canvas type? That seems surprising.

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppb_graphi...
File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right):

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppb_graphi...
webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:521: CGContextRef cgContext =
bitLocker.cgContext();
Same question here; why use CG?

[stuartmorgan, 2011-04-12 18:14:04]

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppapi_plug...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppapi_plug...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1321: #endif  //
defined(OS_MACOSX)
FYI, it looks like you could/should just change the defines here and around
DrawSkBitmapToCanvas, so that the Skia-Mac path is the same as all the other
platforms, rather than making a function that was only needed for CG handle
Skia.

[_cary, 2011-04-12 21:03:54]

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/npapi/webplugin_...
File webkit/plugins/npapi/webplugin_delegate_impl_mac.mm (right):

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/npapi/webplugin_...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:478: void
WebPluginDelegateImpl::Paint(WebKit::WebCanvas* canvas,
On 2011/04/12 17:35:51, stuartmorgan wrote:
> I don't suppose there's a less confusing way to do these defines? Having the
#if
> wrapping one function in the header and the other in the implementation looks
> like a mistake; I had to read it a few times to understand that it wasn't.

Done.

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/npapi/webplugin_...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:480: gfx::SkiaBitLocker
bitLocker(canvas);
Yes, the Skia bitmap preserves alpha so transparent compositing works.

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppapi_plug...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppapi_plug...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1220: gfx::SkiaBitLocker
bitLocker(canvas);
On 2011/04/12 17:35:51, stuartmorgan wrote:
> Indentation is wrong on this whole block

Done.

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppapi_plug...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1321: #endif  //
defined(OS_MACOSX)
On 2011/04/12 18:14:04, stuartmorgan wrote:
> FYI, it looks like you could/should just change the defines here and around
> DrawSkBitmapToCanvas, so that the Skia-Mac path is the same as all the other
> platforms, rather than making a function that was only needed for CG handle
> Skia.

Done.

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppapi_plug...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1395: CGContextRef cgContext =
bitLocker.cgContext();
Fixed, by using non-CG code paths where appropriate.

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppb_graphi...
File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right):

http://codereview.chromium.org/6823081/diff/1/webkit/plugins/ppapi/ppb_graphi...
webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:521: CGContextRef cgContext =
bitLocker.cgContext();
On 2011/04/12 17:35:51, stuartmorgan wrote:
> Same question here; why use CG?

Done.

[stuartmorgan, 2011-04-12 21:15:37]

http://codereview.chromium.org/6823081/diff/6002/content/plugin/webplugin_pro...
File content/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/6823081/diff/6002/content/plugin/webplugin_pro...
content/plugin/webplugin_proxy.cc:353: delegate_->CGPaint(windowless_context_,
rect);
This is another case where this whole codepath exists because of the fork
between CG and Skia; having CGPaint be public is a hack working around that.

I'm okay with you not fixing this for now (especially since this will need perf
testing when you unify it, and some tweaks like not drawing black or doing the
coordinate transforms on the Mac), but please put a TODO on this block for
unifying the Skia path.

http://codereview.chromium.org/6823081/diff/6002/webkit/plugins/npapi/webplug...
File webkit/plugins/npapi/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/6823081/diff/6002/webkit/plugins/npapi/webplug...
webkit/plugins/npapi/webplugin_delegate_impl.h:105: void CGPaint(CGContextRef
context, const gfx::Rect& rect);
This shouldn't be in the WebPluginDelegate implementation section. It also needs
a comment explaining why it's here (and that it's temporary)

[_cary, 2011-04-13 11:58:18]

http://codereview.chromium.org/6823081/diff/6002/content/plugin/webplugin_pro...
File content/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/6823081/diff/6002/content/plugin/webplugin_pro...
content/plugin/webplugin_proxy.cc:353: delegate_->CGPaint(windowless_context_,
rect);
On 2011/04/12 21:15:37, stuartmorgan wrote:
> This is another case where this whole codepath exists because of the fork
> between CG and Skia; having CGPaint be public is a hack working around that.
> 
> I'm okay with you not fixing this for now (especially since this will need
perf
> testing when you unify it, and some tweaks like not drawing black or doing the
> coordinate transforms on the Mac), but please put a TODO on this block for
> unifying the Skia path.

Done.

http://codereview.chromium.org/6823081/diff/6002/webkit/plugins/npapi/webplug...
File webkit/plugins/npapi/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/6823081/diff/6002/webkit/plugins/npapi/webplug...
webkit/plugins/npapi/webplugin_delegate_impl.h:105: void CGPaint(CGContextRef
context, const gfx::Rect& rect);
On 2011/04/12 21:15:37, stuartmorgan wrote:
> This shouldn't be in the WebPluginDelegate implementation section. It also
needs
> a comment explaining why it's here (and that it's temporary)

Done.

[stuartmorgan, 2011-04-13 15:19:10]

LGTM with the function declaration moved.

http://codereview.chromium.org/6823081/diff/7001/webkit/plugins/npapi/webplug...
File webkit/plugins/npapi/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/6823081/diff/7001/webkit/plugins/npapi/webplug...
webkit/plugins/npapi/webplugin_delegate_impl.h:108: void CGPaint(CGContextRef
context, const gfx::Rect& rect);
This still needs to be moved out of the block of WebPluginDelegate
implementation functions.

[_cary, 2011-04-13 16:22:43]

http://codereview.chromium.org/6823081/diff/7001/webkit/plugins/npapi/webplug...
File webkit/plugins/npapi/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/6823081/diff/7001/webkit/plugins/npapi/webplug...
webkit/plugins/npapi/webplugin_delegate_impl.h:108: void CGPaint(CGContextRef
context, const gfx::Rect& rect);
On 2011/04/13 15:19:10, stuartmorgan wrote:
> This still needs to be moved out of the block of WebPluginDelegate
> implementation functions.

Done.

[_cary, 2011-05-02 13:09:12]

Stuart. Please re-review. This is unchanged from your earlier review, with the
exception of moving the SkCanvas/CGContext utility into webkit. Thanks

[stuartmorgan, 2011-05-03 20:28:22]

Still LGTM

[_cary, 2011-05-09 17:03:54]

I've added all owners of the files in this CL. Please review, or remove yourself
from the Reviewers line if you're not interested in reviewing. Thanks.

[brettw, 2011-05-09 17:52:05]

http://codereview.chromium.org/6823081/diff/15001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6823081/diff/15001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:82: #include
"third_party/WebKit/Source/WebCore/platform/graphics/skia/BitLockerSkia.h"
We can't use WebCore from anywhere in Chrome, that's what the WebKit API is for
(WebCore doesn't have an API and we assume it changes arbitrarily).

If the amount of code is complex enough to want to avoid duplicating, we should
make a WebKit API for it (exposed in WebKit/Source/WebKit/chromium/public.

[_cary, 2011-05-17 20:32:16]

http://codereview.chromium.org/6823081/diff/15001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6823081/diff/15001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:82: #include
"third_party/WebKit/Source/WebCore/platform/graphics/skia/BitLockerSkia.h"
On 2011/05/09 17:52:06, brettw wrote:
> We can't use WebCore from anywhere in Chrome, that's what the WebKit API is
for
> (WebCore doesn't have an API and we assume it changes arbitrarily).
> 
> If the amount of code is complex enough to want to avoid duplicating, we
should
> make a WebKit API for it (exposed in WebKit/Source/WebKit/chromium/public.

Done.

[brettw, 2011-05-18 20:15:46]

Just some style nits.

http://codereview.chromium.org/6823081/diff/25001/content/plugin/webplugin_pr...
File content/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/6823081/diff/25001/content/plugin/webplugin_pr...
content/plugin/webplugin_proxy.cc:353: // FIXME: This is a temporary workaround
to allow the Darwin / Skia port to
This should be:
  TODO(caryclark): This is a...

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
File webkit/plugins/npapi/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_delegate_impl.h:181: // FIXME: This is a
temporary workaround to allow the Darwin / Skia port to
Ditto

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
File webkit/plugins/npapi/webplugin_delegate_impl_mac.mm (right):

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:465: const gfx::Rect& rect)
{
This should be indented to be aligned with the first arg

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:467: gfx::SkiaBitLocker
bitLocker(canvas);
Don't use camelCase for variable names:
  bitLocker -> bit_locker

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:476: const gfx::Rect& rect)
{
Align args.

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1254: gfx::SkiaBitLocker
bitLocker(canvas);
Ditto about camelCase.

[_cary, 2011-05-19 12:28:37]

http://codereview.chromium.org/6823081/diff/25001/content/plugin/webplugin_pr...
File content/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/6823081/diff/25001/content/plugin/webplugin_pr...
content/plugin/webplugin_proxy.cc:353: // FIXME: This is a temporary workaround
to allow the Darwin / Skia port to
On 2011/05/18 20:15:46, brettw wrote:
> This should be:
>   TODO(caryclark): This is a...

Done.

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
File webkit/plugins/npapi/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_delegate_impl.h:181: // FIXME: This is a
temporary workaround to allow the Darwin / Skia port to
On 2011/05/18 20:15:46, brettw wrote:
> Ditto

Done.

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
File webkit/plugins/npapi/webplugin_delegate_impl_mac.mm (right):

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:465: const gfx::Rect& rect)
{
On 2011/05/18 20:15:46, brettw wrote:
> This should be indented to be aligned with the first arg

Done.

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:467: gfx::SkiaBitLocker
bitLocker(canvas);
On 2011/05/18 20:15:46, brettw wrote:
> Don't use camelCase for variable names:
>   bitLocker -> bit_locker

Done.

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_delegate_impl_mac.mm:476: const gfx::Rect& rect)
{
On 2011/05/18 20:15:46, brettw wrote:
> Align args.

Done.

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6823081/diff/25001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1254: gfx::SkiaBitLocker
bitLocker(canvas);
On 2011/05/18 20:15:46, brettw wrote:
> Ditto about camelCase.

Done.

[brettw, 2011-05-24 17:22:31]

LGTM

[jam, 2011-05-25 16:09:30]

lgtm

[commit-bot: I haz the power, 2011-05-25 16:34:49]

List of reviewers changed.

[commit-bot: I haz the power, 2011-05-25 19:02:14]

Change committed as 86662