Allow the creation of a skia::PlatformCanvas from a CGContextRef.

Wire up missing functionality in BitmapPlatformContext to allow the creation of a skia::PlatformCanvas
from a CGContextRef.

BUG=
TEST=test_shell_tests, try (mac,win,linux)

[sjl, 2010-12-28 04:26:09]

Brett - does this skia change look good to you?

Albert - sent your way as you're the last person to look at this code (I
think...)

[brettw, 2010-12-29 21:03:52]

http://codereview.chromium.org/6064007/diff/1/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

http://codereview.chromium.org/6064007/diff/1/skia/ext/platform_canvas.h#newc...
skia/ext/platform_canvas.h:51: bool initialize(CGContextRef context, int width,
int height, bool is_opaque);
Can you move this before the other one to match the order of the platformCanvas
constructors?

http://codereview.chromium.org/6064007/diff/1/skia/ext/platform_canvas.h#newc...
skia/ext/platform_canvas.h:111: bool initialize(SkDevice* device);
We should avoid overloading the initialize function like this. How about calling
it initWithDevice(). Can the comment also say that it's used internally by the
initialize() functions?

Also, you declare this for every platform but only define/use it on Mac. It
looks like this code is copied into all other platforms. Can you move the
implementation to the shared platform_canvas.cc file and make all the platforms
use it instead of the device setting and unrefing code?

http://codereview.chromium.org/6064007/diff/1/webkit/glue/webmediaplayer_impl.cc
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6064007/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.cc:587: // TODO(sjl): Can this ever happen?
I was wondering the same thing as this todo says. Looking quickly at the
documentation, it's my (possibly incorrect) understanding that this will happen
if the context is created in a way where the storage is unspecified. Is that
your understanding as well?

If we never hit this, I think it would be better to assert instead of keeping
this (kind of a lot of) code around, which may have bugs and will bitrot.

[sjl, 2010-12-31 05:55:49]

Thanks - addressed all comments.

However... this cl isn't good due to a regression in alpha handling: copied from
the bug (17702).

------------
This works, other than a failure with LayoutTests/media/video-canvas-alpha.html.
The failure is due not propagating the current global alpha from the CG context
into the SkCanvas current SkPaint. Unfortunately as things stand right now
there's no way to get the current global alpha at this level as all we have is
the CGContext (WebCanvas) and there's no api on the CGContext to get it.

However, event if we had the alpha value, we'd need to perform a
SkCanvas::saveLayerAlpha(), which is just what the skia implementation does in
WebMediaPlayerClientImpl::paint(), which introduces an extra copy and composite.

Indeed, this implies that the CG implementation currently has the same number of
copies as the skia implementation (just in different places).

So for now I don't think much can be done with this unless a real
PlatformGraphicsContext for CG is introduced (rather than it being a typedef for
CGContextRef).
------------

So, in summary (unless there are any good ideas). The changes to
webmediaplayer_impl.cc shouldn't be checked in, but I believe the skia changes
are still good.

Brett - do you want those in? If so, I'll remove webmedia changes from this cl
and rename it (rather than creating a new cl as the review history would be
lost).

Thoughts?

http://codereview.chromium.org/6064007/diff/1/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

http://codereview.chromium.org/6064007/diff/1/skia/ext/platform_canvas.h#newc...
skia/ext/platform_canvas.h:51: bool initialize(CGContextRef context, int width,
int height, bool is_opaque);
On 2010/12/29 21:03:52, brettw wrote:
> Can you move this before the other one to match the order of the
platformCanvas
> constructors?

Done.

http://codereview.chromium.org/6064007/diff/1/skia/ext/platform_canvas.h#newc...
skia/ext/platform_canvas.h:111: bool initialize(SkDevice* device);
On 2010/12/29 21:03:52, brettw wrote:
> We should avoid overloading the initialize function like this. How about
calling
> it initWithDevice(). Can the comment also say that it's used internally by the
> initialize() functions?
> 
> Also, you declare this for every platform but only define/use it on Mac. It
> looks like this code is copied into all other platforms. Can you move the
> implementation to the shared platform_canvas.cc file and make all the
platforms
> use it instead of the device setting and unrefing code?

Done.

http://codereview.chromium.org/6064007/diff/1/webkit/glue/webmediaplayer_impl.cc
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6064007/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.cc:587: // TODO(sjl): Can this ever happen?
On 2010/12/29 21:03:52, brettw wrote:
> I was wondering the same thing as this todo says. Looking quickly at the
> documentation, it's my (possibly incorrect) understanding that this will
happen
> if the context is created in a way where the storage is unspecified. Is that
> your understanding as well?

That's my understanding too.
> 
> If we never hit this, I think it would be better to assert instead of keeping
> this (kind of a lot of) code around, which may have bugs and will bitrot.

Good idea. Done.

[brettw, 2011-01-03 21:02:34]

The skia changes seem like they reduce some copied code, so would be nice to get
in.

[sjl, 2011-01-04 18:30:59]

Removed the changes to webmediaplayer_impl.cc and made this cl just about the
skia changes.

[brettw, 2011-01-04 18:45:21]

LGTM

[scherkus (not reviewing), 2011-01-05 01:32:21]

LGTM

don't forget to update CL description, TEST= and BUG= if needed

[scherkus (not reviewing), 2011-01-05 01:33:05]

whoops I don't even think I was a reviewer!

[sjl, 2011-01-05 01:41:42]

On 2011/01/05 01:33:05, scherkus wrote:
> whoops I don't even think I was a reviewer!

Heh. There's no bug - as this was a side effect of fixing the original bug.

[brettw, 2011-01-06 07:03:56]

Landed in r70596