Updating Chromium to Skia SkBaseDevice/SkBitmapDevice split

Skia is going to Split the SkDevice class into SkBaseDevice and SkBitmapDevice. Right now Skia has typedefs redirecting SkBaseDevice and SkBitmapDevice to SkDevice. This CL begins the Chromium-side renaming process in preparation for the real change (https://codereview.chromium.org/22978012/)

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

[danakj, 2013-08-22 19:48:30]

cc/ lgtm

https://codereview.chromium.org/22796028/diff/3001/cc/output/software_output_...
File cc/output/software_output_device.h (right):

https://codereview.chromium.org/22796028/diff/3001/cc/output/software_output_...
cc/output/software_output_device.h:11: // TODO(robertphillips): change this to
"class SkBaseDevice;"
How come this isn't possible right now?

https://codereview.chromium.org/22796028/diff/3001/cc/resources/raster_worker...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/22796028/diff/3001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.h:17: 
drop the newline here if you're making it an include

https://codereview.chromium.org/22796028/diff/3001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.h:18: // TODO(robertphillips): change this to
"class SkBaseDevice;"
and this?

[robertphillips, 2013-08-22 19:55:52]

https://codereview.chromium.org/22796028/diff/3001/cc/output/software_output_...
File cc/output/software_output_device.h (right):

https://codereview.chromium.org/22796028/diff/3001/cc/output/software_output_...
cc/output/software_output_device.h:11: // TODO(robertphillips): change this to
"class SkBaseDevice;"
I'm trying to minimize the amount of work that needs to be done when the Skia
patch lands (and DEPS rolls). Right now we would need add:

class SkDevice;
typedef SkDevice SkBaseDevice;

here to make the compiler happy. When doing the DEPS roll that includes the Skia
change I would also have to immediately convert them to 

class SkBaseDevice;

By temporarily including the header I can postpone those changes to another
patch after the big switch.

https://codereview.chromium.org/22796028/diff/3001/cc/resources/raster_worker...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/22796028/diff/3001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.h:17: 
On 2013/08/22 19:48:30, danakj wrote:
> drop the newline here if you're making it an include

Done.

[danakj, 2013-08-22 19:56:44]

https://codereview.chromium.org/22796028/diff/3001/cc/output/software_output_...
File cc/output/software_output_device.h (right):

https://codereview.chromium.org/22796028/diff/3001/cc/output/software_output_...
cc/output/software_output_device.h:11: // TODO(robertphillips): change this to
"class SkBaseDevice;"
On 2013/08/22 19:55:53, robertphillips wrote:
> I'm trying to minimize the amount of work that needs to be done when the Skia
> patch lands (and DEPS rolls). Right now we would need add:
> 
> class SkDevice;
> typedef SkDevice SkBaseDevice;
> 
> here to make the compiler happy. When doing the DEPS roll that includes the
Skia
> change I would also have to immediately convert them to 
> 
> class SkBaseDevice;
> 
> By temporarily including the header I can postpone those changes to another
> patch after the big switch.

I see, thanks. sg.

[DaleCurtis, 2013-08-22 20:01:08]

media/ lgtm

[vandebo (ex-Chrome), 2013-08-22 20:41:58]

When adding multiple reviews, please list which parts you'd like each reviewer
to look at.

*printing* is ok
skia/ext/ has a few comments

https://codereview.chromium.org/22796028/diff/3001/skia/ext/analysis_canvas.h
File skia/ext/analysis_canvas.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/analysis_canvas.h...
skia/ext/analysis_canvas.h:20: // To use: create a SkBitmap with kNo_Config,
create an AnalysisDevice
This suggests that Bitmap is a artifact of no base class.

https://codereview.chromium.org/22796028/diff/3001/skia/ext/analysis_canvas.h...
skia/ext/analysis_canvas.h:61: class SK_API AnalysisDevice : public
SkBitmapDevice {
Why inherit from BitmapDevice for this one?

https://codereview.chromium.org/22796028/diff/3001/skia/ext/bitmap_platform_d...
File skia/ext/bitmap_platform_device_mac.cc (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/bitmap_platform_d...
skia/ext/bitmap_platform_device_mac.cc:258: SkBaseDevice* bitmap_device =
BitmapPlatformDevice::CreateAndClear(width, height,
80 col

https://codereview.chromium.org/22796028/diff/3001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/platform_device.h...
skia/ext/platform_device.h:52: // The following routines provide accessor points
for the functionality
The comments in this file needs some closer attention.  Also, 80 col wrap.

https://codereview.chromium.org/22796028/diff/3001/skia/ext/platform_device.h...
skia/ext/platform_device.h:97: class SK_API PlatformDevice {
Not part of this CL, but with SkBitmapDevice separated from SkBaseDevice,
PlatformDevice might want to inherit from SkBaseDevice again (it used to inherit
from SkDevice)

https://codereview.chromium.org/22796028/diff/3001/skia/ext/vector_platform_d...
File skia/ext/vector_platform_device_emf_win.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/vector_platform_d...
skia/ext/vector_platform_device_emf_win.h:20: class VectorPlatformDeviceEmf :
public SkBitmapDevice, public PlatformDevice {
Should this be SkBaseDevice ?

[jochen (gone - plz use gerrit), 2013-08-22 21:43:56]

lgtm

can you please reference a bug in the CL description?

[Torne, 2013-08-23 10:41:09]

android_webview/ LGTM

[robertphillips, 2013-08-23 14:01:02]

After the fact, here is what I had intended:

android_webview: Torne
cc:              danakj
chrome:          jochen
content:         jam (and now brettw)
media:           DaleCurtis
printing:        vandebo
skia:            fmalita

I have added a bug reference to the description.

https://codereview.chromium.org/22796028/diff/3001/skia/ext/analysis_canvas.h
File skia/ext/analysis_canvas.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/analysis_canvas.h...
skia/ext/analysis_canvas.h:20: // To use: create a SkBitmap with kNo_Config,
create an AnalysisDevice
I think the comment is currently correct in that SkDevices' require a backing
bitmap - even if it is just a bogus one.

https://codereview.chromium.org/22796028/diff/3001/skia/ext/analysis_canvas.h...
skia/ext/analysis_canvas.h:61: class SK_API AnalysisDevice : public
SkBitmapDevice {
The Skia patch that is about to land doesn't actually change the
SkDevice/SkBaseDevice API it just splits SkDevice into two so we can, in future
CLs, fix the SkBaseDevice API. Until the bitmap-centric nature of SkBaseDevice
is remedied I think we are stuck providing support for the full interface and,
currently, the easiest way to do that is to derived from SkBitmapDevice.

Once SkBaseDevice's interface doesn't require a bitmap AnalysisDevice should
derive from it rather than SkBitmapDevice.

https://codereview.chromium.org/22796028/diff/3001/skia/ext/bitmap_platform_d...
File skia/ext/bitmap_platform_device_mac.cc (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/bitmap_platform_d...
skia/ext/bitmap_platform_device_mac.cc:258: SkBaseDevice* bitmap_device =
BitmapPlatformDevice::CreateAndClear(width, height,
On 2013/08/22 20:41:58, vandebo wrote:
> 80 col

Done.

https://codereview.chromium.org/22796028/diff/3001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/platform_device.h...
skia/ext/platform_device.h:52: // The following routines provide accessor points
for the functionality
I have removed the bits that are obviously wrong (e.g., PlatformDevice being
derived from SkDevice and derivation from SkDevice not being supported) but I'm
not familiar enough with the code to push much further.

80 cols is fixed

https://codereview.chromium.org/22796028/diff/3001/skia/ext/platform_device.h...
skia/ext/platform_device.h:97: class SK_API PlatformDevice {
I think that would make sense but I'm not too familiar with this code.

https://codereview.chromium.org/22796028/diff/3001/skia/ext/vector_platform_d...
File skia/ext/vector_platform_device_emf_win.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/vector_platform_d...
skia/ext/vector_platform_device_emf_win.h:20: class VectorPlatformDeviceEmf :
public SkBitmapDevice, public PlatformDevice {
Ultimately yes - short term no. Please see earlier comment.

[f(malita), 2013-08-23 14:25:50]

Based on Robert's comments, skia/ lgtm.

[vandebo (ex-Chrome), 2013-08-23 17:10:03]

skia/ LGTM

https://codereview.chromium.org/22796028/diff/3001/skia/ext/analysis_canvas.h
File skia/ext/analysis_canvas.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/analysis_canvas.h...
skia/ext/analysis_canvas.h:61: class SK_API AnalysisDevice : public
SkBitmapDevice {
On 2013/08/23 14:01:03, robertphillips wrote:
> The Skia patch that is about to land doesn't actually change the
> SkDevice/SkBaseDevice API it just splits SkDevice into two so we can, in
future
> CLs, fix the SkBaseDevice API. Until the bitmap-centric nature of SkBaseDevice
> is remedied I think we are stuck providing support for the full interface and,
> currently, the easiest way to do that is to derived from SkBitmapDevice.
> 
> Once SkBaseDevice's interface doesn't require a bitmap AnalysisDevice should
> derive from it rather than SkBitmapDevice.

TODO here?

https://codereview.chromium.org/22796028/diff/3001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/platform_device.h...
skia/ext/platform_device.h:97: class SK_API PlatformDevice {
On 2013/08/23 14:01:03, robertphillips wrote:
> I think that would make sense but I'm not too familiar with this code.

Can you add a TODO here to figure it out...

PlatformDevice used to inherit from SkDevice (now SkBitmapDevice) but that
didn't make sense for things like SkPDFDevice so PlatformDevice stopped
inheriting from SkDevice and takes it as an argument where needed. Now that Skia
has SkBaseDevice it might make sense to have PlatformDevice inherit from it.

https://codereview.chromium.org/22796028/diff/3001/skia/ext/vector_platform_d...
File skia/ext/vector_platform_device_emf_win.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/vector_platform_d...
skia/ext/vector_platform_device_emf_win.h:20: class VectorPlatformDeviceEmf :
public SkBitmapDevice, public PlatformDevice {
On 2013/08/23 14:01:03, robertphillips wrote:
> Ultimately yes - short term no. Please see earlier comment.

Does it make sense to add a TODO? Down the road it might be hard to remember
which ones should be changed.

[jam, 2013-08-25 16:41:13]

content lgtm

[robertphillips, 2013-08-26 13:11:48]

https://codereview.chromium.org/22796028/diff/3001/skia/ext/analysis_canvas.h
File skia/ext/analysis_canvas.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/analysis_canvas.h...
skia/ext/analysis_canvas.h:61: class SK_API AnalysisDevice : public
SkBitmapDevice {
On 2013/08/23 17:10:04, vandebo wrote:
> On 2013/08/23 14:01:03, robertphillips wrote:
> > The Skia patch that is about to land doesn't actually change the
> > SkDevice/SkBaseDevice API it just splits SkDevice into two so we can, in
> future
> > CLs, fix the SkBaseDevice API. Until the bitmap-centric nature of
SkBaseDevice
> > is remedied I think we are stuck providing support for the full interface
and,
> > currently, the easiest way to do that is to derived from SkBitmapDevice.
> > 
> > Once SkBaseDevice's interface doesn't require a bitmap AnalysisDevice should
> > derive from it rather than SkBitmapDevice.
> 
> TODO here?

Done.

https://codereview.chromium.org/22796028/diff/3001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/platform_device.h...
skia/ext/platform_device.h:97: class SK_API PlatformDevice {
On 2013/08/23 17:10:04, vandebo wrote:
> On 2013/08/23 14:01:03, robertphillips wrote:
> > I think that would make sense but I'm not too familiar with this code.
> 
> Can you add a TODO here to figure it out...
> 
> PlatformDevice used to inherit from SkDevice (now SkBitmapDevice) but that
> didn't make sense for things like SkPDFDevice so PlatformDevice stopped
> inheriting from SkDevice and takes it as an argument where needed. Now that
Skia
> has SkBaseDevice it might make sense to have PlatformDevice inherit from it.

Done.

https://codereview.chromium.org/22796028/diff/3001/skia/ext/vector_platform_d...
File skia/ext/vector_platform_device_emf_win.h (right):

https://codereview.chromium.org/22796028/diff/3001/skia/ext/vector_platform_d...
skia/ext/vector_platform_device_emf_win.h:20: class VectorPlatformDeviceEmf :
public SkBitmapDevice, public PlatformDevice {
On 2013/08/23 17:10:04, vandebo wrote:
> On 2013/08/23 14:01:03, robertphillips wrote:
> > Ultimately yes - short term no. Please see earlier comment.
> 
> Does it make sense to add a TODO? Down the road it might be hard to remember
> which ones should be changed.

Done.

[commit-bot: I haz the power, 2013-08-26 13:11:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertphillips@google.com/22796028/33001

[commit-bot: I haz the power, 2013-08-26 18:37:35]

Change committed as 219568