Eliminate skia::PlatformCanvas - Step 1

Eliminate skia::PlatformCanvas, a subclass of SkCanvas. Skia provides multiple types of SkCanvas classes that we would like to use. Unfortunately these classes are implemented as subclasses of SkCanvas. Subclassing SkCanvas in both Skia and Chromium makes it impossible to dynamically use any SkCanvas. There is also no reason for chromium to subclass SkCanvas. Most of the extra functionalities can be implemented by hanging meta-data from SkCanvas.

We cannot eliminate skia::PlatformCanvas in one step due to WebKit's dependency on skia::PlatformCanvas. WebKit::WebCanvas is typedef as skia::PlatformDevice. It should be SkCanvas. So we need to do it in multiple steps:
1. Prepare Chromium tree for the change in WebKit::WebCanvas tyepdef. This basically means adding a couple of static_cast<skia::PlatformCanvas>(WebCanvas).
2. Change WebKit::WebCanvas typedef from skia::PlatformCanvas to SkCanvas
3. Eliminate skia::PlatformCanvas in chromium

This CL accomplishes the first step on windows.

WebKit BUG=https://bugs.webkit.org/show_bug.cgi?id=57563
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80955

[reed1, 2011-04-01 17:44:41]

http://codereview.chromium.org/6783023/diff/10004/webkit/glue/webthemeengine_...
File webkit/glue/webthemeengine_impl_win.cc (right):

http://codereview.chromium.org/6783023/diff/10004/webkit/glue/webthemeengine_...
webkit/glue/webthemeengine_impl_win.cc:30: 
In preparation for the future, is it clear to create two helpers here?

static HDC PlatformBeginPaint(SkCanvas* canvas) {
    return <PlatformCanvas*>(canvas)->beginPlatformPaint();
}

static void PlatformEndPaint(SkCanvas* canvas) {
    ...
}

Its nearly the same, but it would seem to prepare the way going forward to
replace the cast with SkMetaData calls. Just a thought, not a strong suggestion.

http://codereview.chromium.org/6783023/diff/10004/webkit/plugins/sad_plugin.cc
File webkit/plugins/sad_plugin.cc (left):

http://codereview.chromium.org/6783023/diff/10004/webkit/plugins/sad_plugin.c...
webkit/plugins/sad_plugin.cc:41: gfx::NativeDrawingContext context =
webcanvas->beginPlatformPaint();
I presume these casts get replaced by some static function in the future, where
PlatformCanvas goes away permanently?

[alokp, 2011-04-01 19:57:38]

PTAL

http://codereview.chromium.org/6783023/diff/10004/webkit/glue/webthemeengine_...
File webkit/glue/webthemeengine_impl_win.cc (right):

http://codereview.chromium.org/6783023/diff/10004/webkit/glue/webthemeengine_...
webkit/glue/webthemeengine_impl_win.cc:30: 
On 2011/04/01 17:44:42, reed1 wrote:
> In preparation for the future, is it clear to create two helpers here?
> 
> static HDC PlatformBeginPaint(SkCanvas* canvas) {
>     return <PlatformCanvas*>(canvas)->beginPlatformPaint();
> }
> 
> static void PlatformEndPaint(SkCanvas* canvas) {
>     ...
> }
> 
> Its nearly the same, but it would seem to prepare the way going forward to
> replace the cast with SkMetaData calls. Just a thought, not a strong
suggestion.

Done.

http://codereview.chromium.org/6783023/diff/10004/webkit/plugins/sad_plugin.cc
File webkit/plugins/sad_plugin.cc (left):

http://codereview.chromium.org/6783023/diff/10004/webkit/plugins/sad_plugin.c...
webkit/plugins/sad_plugin.cc:41: gfx::NativeDrawingContext context =
webcanvas->beginPlatformPaint();
On 2011/04/01 17:44:42, reed1 wrote:
> I presume these casts get replaced by some static function in the future,
where
> PlatformCanvas goes away permanently?

Done.

[reed1, 2011-04-01 19:59:13]

LGTM

[alokp, 2011-04-01 20:03:52]

On 2011/04/01 19:59:13, reed1 wrote:
> LGTM

Thanks Mike!
Vangelis: Any issues?

I will send another small CL for linux shortly.

[alokp, 2011-04-01 22:20:14]

Explained the steps in which skia::PlatformCanvas will be ultimately deprecated.

[vangelis, 2011-04-01 23:35:53]

Looks mostly good.  Just a couple of comments.

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

http://codereview.chromium.org/6783023/diff/9017/skia/ext/platform_canvas.h#n...
skia/ext/platform_canvas.h:118: // surface returned here can be used with the
native platform routines
Period missing from the end of the comment.

http://codereview.chromium.org/6783023/diff/16001/webkit/plugins/npapi/webplu...
File webkit/plugins/npapi/webplugin_delegate_impl_win.cc (right):

http://codereview.chromium.org/6783023/diff/16001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_delegate_impl_win.cc:465: void
WebPluginDelegateImpl::Paint(WebKit::WebCanvas* canvas,
What's the criteria for using WebCanvas here instead of SkCanvas directly?

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

http://codereview.chromium.org/6783023/diff/16001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1233:
static_cast<skia::PlatformCanvas*>(canvas);
This seems like a potentially dangerous cast once WebCanvas is changed over to
be an SkCanvas. You can eliminate them if you can find an alternative way of
getting the platform device from the SkCanvas.

[alokp, 2011-04-04 17:00:13]

Will upload updated patch sometime today.

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

http://codereview.chromium.org/6783023/diff/9017/skia/ext/platform_canvas.h#n...
skia/ext/platform_canvas.h:118: // surface returned here can be used with the
native platform routines
On 2011/04/01 23:35:53, vangelis wrote:
> Period missing from the end of the comment.

Done.

http://codereview.chromium.org/6783023/diff/16001/webkit/plugins/npapi/webplu...
File webkit/plugins/npapi/webplugin_delegate_impl_win.cc (right):

http://codereview.chromium.org/6783023/diff/16001/webkit/plugins/npapi/webplu...
webkit/plugins/npapi/webplugin_delegate_impl_win.cc:465: void
WebPluginDelegateImpl::Paint(WebKit::WebCanvas* canvas,
On 2011/04/01 23:35:54, vangelis wrote:
> What's the criteria for using WebCanvas here instead of SkCanvas directly? 
>  

The function declaration as well as other implementations (mac and gtk) use
WebCanvas. This is more for consistency than anything else.

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

http://codereview.chromium.org/6783023/diff/16001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1233:
static_cast<skia::PlatformCanvas*>(canvas);
On 2011/04/01 23:35:54, vangelis wrote:
> This seems like a potentially dangerous cast once WebCanvas is changed over to
> be an SkCanvas. You can eliminate them if you can find an alternative way of
> getting the platform device from the SkCanvas.

Ok. Working with reed@ get rid of these static casts.

[alokp, 2011-04-04 21:33:22]

Vangelis:
Using the meta-data turned out to be more involved than I originally thought.
Doing it right will make this CL unnecessarily big. So I have got rid of the
potentially dangerous static-cast by using Begin/EndPlatformPaint everywhere. I
have also pushed these two functions for native drawing to PlatformDevice so
that it is easier to eliminate PlatformCanvas later.

The only two static casts remaining are in skia::BeginPlatformPaint and
skia::EndPlatformPaint, but they are no worse than the one in
PlatformCanvas::getTopPlatformDevice. We can get rid of this by either
deprecating PlatformDevice or hanging PlatformDeviceData off SkDevice.

[vangelis, 2011-04-05 06:23:10]

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_li...
File skia/ext/platform_device_linux.h (right):

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_li...
skia/ext/platform_device_linux.h:26: virtual PlatformSurface
beginPlatformPaint() = 0;
PlatformDevice appears to be starting all method names with a capital letter (as
opposed to PlatformCanvas that doesn't... not sure why).  I think these two
method should be: BeginPlatformPaint and EndPlatformPaint

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_li...
skia/ext/platform_device_linux.h:27: virtual void endPlatformPaint();
pure virtual ?

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_ma...
File skia/ext/platform_device_mac.cc (right):

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_ma...
skia/ext/platform_device_mac.cc:43: PlatformSurface
PlatformDevice::beginPlatformPaint() {
Looks like this makes the mac compiler unhappy.  PlatformDevice::PlatformSurface
?

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_wi...
File skia/ext/platform_device_win.cc (right):

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_wi...
skia/ext/platform_device_win.cc:24: // we don't clear the DC here since it will
be likely to be used again
This reads a little strange.  If they are supposed to be two sentences then
please use a period after "again".

[alokp, 2011-04-05 15:37:56]

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_li...
File skia/ext/platform_device_linux.h (right):

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_li...
skia/ext/platform_device_linux.h:26: virtual PlatformSurface
beginPlatformPaint() = 0;
On 2011/04/05 06:23:10, vangelis wrote:
> PlatformDevice appears to be starting all method names with a capital letter
(as
> opposed to PlatformCanvas that doesn't... not sure why).  I think these two
> method should be: BeginPlatformPaint and EndPlatformPaint

Done.

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_li...
skia/ext/platform_device_linux.h:27: virtual void endPlatformPaint();
On 2011/04/05 06:23:10, vangelis wrote:
> pure virtual ? 

No. It is implemented in PlatformDevice. It is different from
BeginPlatformPaint() where implementation is different for various subclasses.

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_ma...
File skia/ext/platform_device_mac.cc (right):

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_ma...
skia/ext/platform_device_mac.cc:43: PlatformSurface
PlatformDevice::beginPlatformPaint() {
On 2011/04/05 06:23:10, vangelis wrote:
> Looks like this makes the mac compiler unhappy. 
PlatformDevice::PlatformSurface
> ? 

Done.

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_wi...
File skia/ext/platform_device_win.cc (right):

http://codereview.chromium.org/6783023/diff/23005/skia/ext/platform_device_wi...
skia/ext/platform_device_win.cc:24: // we don't clear the DC here since it will
be likely to be used again
On 2011/04/05 06:23:10, vangelis wrote:
> This reads a little strange.  If they are supposed to be two sentences then
> please use a period after "again".

Done.

[alokp, 2011-04-05 16:22:18]

Adding OWNERS.

[alokp, 2011-04-07 05:05:11]

Rebase to r80707.
Darin: Could you please take a look.

[reed1, 2011-04-07 13:10:26]

LGTM

[gwright, 2011-04-07 15:28:11]

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

http://codereview.chromium.org/6783023/diff/24001/skia/ext/platform_canvas.h#...
skia/ext/platform_canvas.h:120: SK_API SkCanvas* CreateBitmapCanvas(int width,
int height, bool is_opaque);
Do we want an is_offscreen parameter here as well?

[alokp, 2011-04-07 15:57:36]

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

http://codereview.chromium.org/6783023/diff/24001/skia/ext/platform_canvas.h#...
skia/ext/platform_canvas.h:120: SK_API SkCanvas* CreateBitmapCanvas(int width,
int height, bool is_opaque);
On 2011/04/07 15:28:12, gwright wrote:
> Do we want an is_offscreen parameter here as well?

May be. The one I am replacing in webkit does not need it. I will revisit it
once I have replaced all create instances with a factory.

[darin (slow to review), 2011-04-08 04:45:20]

LGTM

[alokp, 2011-04-08 04:45:27]

Adding OWNERS for ui. Please review.

[sky, 2011-04-08 16:12:03]

LGTM