Stop using deprecated factory API for SkDevice

Stop using deprecated factory API for SkDevice
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91504
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91508

[reed1, 2011-06-27 21:56:29]

There is a related (but not dependent) change in PlatformContextSkia.cpp for
webkit.

[vandebo (ex-Chrome), 2011-06-27 22:03:19]

There's also skia/ext/vector_platform_device_*

[alokp, 2011-06-28 16:19:46]

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
File skia/ext/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
skia/ext/bitmap_platform_device_linux.cc:132: SkASSERT(config ==
SkBitmap::kARGB_8888_Config);
nit: I think we should use DCHECK in chromium code.

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
File skia/ext/bitmap_platform_device_mac.cc (right):

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
skia/ext/bitmap_platform_device_mac.cc:239: SkASSERT(config ==
SkBitmap::kARGB_8888_Config);
ditto and elsewhere

http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_canvas.cc
File skia/ext/vector_canvas.cc (right):

http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_canvas.cc#ne...
skia/ext/vector_canvas.cc:11: : PlatformCanvas() {
no need to explicitly call the default constructor.

http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_platform_dev...
File skia/ext/vector_platform_device_cairo_linux.cc (right):

http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_platform_dev...
skia/ext/vector_platform_device_cairo_linux.cc:74: return
VectorPlatformDeviceCairoFactory::CreateDevice(NULL, width, height,
Two things:
1. In chromium functions are defined in the same order they are declared in
header file.
2. Why do we need a factory here?

[vandebo (ex-Chrome), 2011-06-28 19:07:54]

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
File skia/ext/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
skia/ext/bitmap_platform_device_linux.cc:131: SkBitmap::Config config, int
width, int height, bool isOpaque, Usage) {
nit: 4 space indent. (multiple occurrences.)

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
File skia/ext/bitmap_platform_device_linux.h (right):

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
skia/ext/bitmap_platform_device_linux.h:89: virtual SkDevice*
onCreateCompatibleDevice(SkBitmap::Config,  int width,
nit: extra space after Config. (multiple occurrences.)

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
skia/ext/bitmap_platform_device_linux.h:90: int height, bool isOpaque, Usage);
nit: 'Usage usage' or 'SkDevice:Usage usage' (multiple occurences.)

http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_platform_dev...
File skia/ext/vector_platform_device_emf_win.cc (right):

http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_platform_dev...
skia/ext/vector_platform_device_emf_win.cc:23: PlatformDevice*
VectorPlatformDeviceEmfFactory::CreateDevice(
CreateDevice used to be a method of VectorCanvas, but it got moved into the
factory when I added them (see http://codereview.chromium.org/3590011/).  It
seems reasonable to me to make it a static method on VectorPlatforDeviceEmf. 
(Same goes for the cairo version.)

http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_platform_dev...
File skia/ext/vector_platform_device_skia.cc (right):

http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_platform_dev...
skia/ext/vector_platform_device_skia.cc:217: return
pdf_device_->createCompatibleDevice(config, width, height, isOpaque);
To do it this way, at least VectorPlatformDeviceSkia::drawDevice would need to
be updated, but I don't know if anything else needs to change. For now, it's
probably easy to just wrap the SkPDFDevice in a VectorPlatformDeviceSkia like
VectorPlatformDeviceSkiaFactory::newDevice did.

[reed1, 2011-06-30 18:55:37]

I think the win_layout failures are unrelated. I tried to address your comments
(except that in these files, I see lots of existing SkASSERT already, so I kept
it)

[alokp, 2011-06-30 20:00:54]

lgtm

[reed1, 2011-06-30 21:34:05]

On 2011/06/28 19:07:54, vandebo wrote:
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
> File skia/ext/bitmap_platform_device_linux.cc (right):
> 
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
> skia/ext/bitmap_platform_device_linux.cc:131: SkBitmap::Config config, int
> width, int height, bool isOpaque, Usage) {
> nit: 4 space indent. (multiple occurrences.)

Done.

> 
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
> File skia/ext/bitmap_platform_device_linux.h (right):
> 
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
> skia/ext/bitmap_platform_device_linux.h:89: virtual SkDevice*
> onCreateCompatibleDevice(SkBitmap::Config,  int width,
> nit: extra space after Config. (multiple occurrences.)

Done.

> 
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
> skia/ext/bitmap_platform_device_linux.h:90: int height, bool isOpaque, Usage);
> nit: 'Usage usage' or 'SkDevice:Usage usage' (multiple occurences.)

What is the ask? Do add a variable name in the parameter list? I see examples
where this is skipped already, and webkit even requires that you leave out the
variable name if it just renames the type.

> 
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_platform_dev...
> File skia/ext/vector_platform_device_emf_win.cc (right):
> 
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_platform_dev...
> skia/ext/vector_platform_device_emf_win.cc:23: PlatformDevice*
> VectorPlatformDeviceEmfFactory::CreateDevice(
> CreateDevice used to be a method of VectorCanvas, but it got moved into the
> factory when I added them (see http://codereview.chromium.org/3590011/).  It
> seems reasonable to me to make it a static method on VectorPlatforDeviceEmf. 
> (Same goes for the cairo version.)

Done.

> 
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_platform_dev...
> File skia/ext/vector_platform_device_skia.cc (right):
> 
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/vector_platform_dev...
> skia/ext/vector_platform_device_skia.cc:217: return
> pdf_device_->createCompatibleDevice(config, width, height, isOpaque);
> To do it this way, at least VectorPlatformDeviceSkia::drawDevice would need to
> be updated, but I don't know if anything else needs to change. For now, it's
> probably easy to just wrap the SkPDFDevice in a VectorPlatformDeviceSkia like
> VectorPlatformDeviceSkiaFactory::newDevice did.

Done.

[vandebo (ex-Chrome), 2011-06-30 21:58:36]

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
File skia/ext/bitmap_platform_device_linux.h (right):

http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
skia/ext/bitmap_platform_device_linux.h:90: int height, bool isOpaque, Usage);
> On 2011/06/28 19:07:54, vandebo wrote:
> > nit: 'Usage usage' or 'SkDevice:Usage usage' (multiple occurences.)
> 
> What is the ask? Do add a variable name in the parameter list? I see examples
> where this is skipped already, and webkit even requires that you leave out the
> variable name if it just renames the type.

Yea, the ask is Usage usage, or if you can think of a better name for it, Usage
foo.  The Google C++ style guide says "All parameters should be named, with
identical names in the declaration and implementation." 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...
 I don't see anything counter acting that in the Chrome style guide.

I agree that Usage usage is kind of silly, but scanning the header, just Usage
sticks out as something being different/wrong and Usage usage doesn't strike me
as silly until I actually read it.

http://codereview.chromium.org/7273013/diff/18007/skia/ext/vector_platform_de...
File skia/ext/vector_platform_device_skia.cc (right):

http://codereview.chromium.org/7273013/diff/18007/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_skia.cc:219: isOpaque));
It looks like we lose Usage?

[reed1, 2011-07-01 12:22:11]

On 2011/06/30 21:58:36, vandebo wrote:
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
> File skia/ext/bitmap_platform_device_linux.h (right):
> 
>
http://codereview.chromium.org/7273013/diff/3005/skia/ext/bitmap_platform_dev...
> skia/ext/bitmap_platform_device_linux.h:90: int height, bool isOpaque, Usage);
> > On 2011/06/28 19:07:54, vandebo wrote:
> > > nit: 'Usage usage' or 'SkDevice:Usage usage' (multiple occurences.)
> > 
> > What is the ask? Do add a variable name in the parameter list? I see
examples
> > where this is skipped already, and webkit even requires that you leave out
the
> > variable name if it just renames the type.
> 
> Yea, the ask is Usage usage, or if you can think of a better name for it,
Usage
> foo.  The Google C++ style guide says "All parameters should be named, with
> identical names in the declaration and implementation." 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...
>  I don't see anything counter acting that in the Chrome style guide.
> 
> I agree that Usage usage is kind of silly, but scanning the header, just Usage
> sticks out as something being different/wrong and Usage usage doesn't strike
me
> as silly until I actually read it.

Done.

> 
>
http://codereview.chromium.org/7273013/diff/18007/skia/ext/vector_platform_de...
> File skia/ext/vector_platform_device_skia.cc (right):
> 
>
http://codereview.chromium.org/7273013/diff/18007/skia/ext/vector_platform_de...
> skia/ext/vector_platform_device_skia.cc:219: isOpaque));
> It looks like we lose Usage?

Do do lose it, as the device api was not designed with proxying in mind (hence
my continuing effort to stop the subclass/indirection in chrome). Fortunately,
the pdf device does not take advantage of the Usage parameter, so we'll survive,
certainly until we can remove this indirection in vector_device.

[reed1, 2011-07-01 12:22:45]

new patch, rebaslining and with vandebo's latest comments about parameter names.

[reed1, 2011-07-01 15:11:06]

I think the win_layout and mac results are not related to this CL

[vandebo (ex-Chrome), 2011-07-01 17:20:14]

Please address nits, then LGTM.  No further review needed from me.

http://codereview.chromium.org/7273013/diff/21016/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_mac.cc (right):

http://codereview.chromium.org/7273013/diff/21016/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_mac.cc:238: SkBitmap::Config config, int width,
int height, bool isOpaque, Usage) {
nit: missing /*usage*/

http://codereview.chromium.org/7273013/diff/21016/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_mac.h (right):

http://codereview.chromium.org/7273013/diff/21016/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_mac.h:66: virtual SkDevice*
onCreateCompatibleDevice(SkBitmap::Config,  int width,
nit: extra space.

http://codereview.chromium.org/7273013/diff/21016/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_win.cc (right):

http://codereview.chromium.org/7273013/diff/21016/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.cc:258: SkBitmap::Config config, int width,
int height, bool isOpaque, Usage) {
nit: missing /*usage*/

http://codereview.chromium.org/7273013/diff/21016/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_win.h (right):

http://codereview.chromium.org/7273013/diff/21016/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.h:71: virtual SkDevice*
onCreateCompatibleDevice(SkBitmap::Config,  int width,
nit: extra space.

http://codereview.chromium.org/7273013/diff/21016/skia/ext/vector_platform_de...
File skia/ext/vector_platform_device_emf_win.cc (right):

http://codereview.chromium.org/7273013/diff/21016/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_emf_win.cc:442: SkBitmap::Config config,
nit: arg doesn't fit, so it should be indented 4 space.

http://codereview.chromium.org/7273013/diff/21016/skia/ext/vector_platform_de...
File skia/ext/vector_platform_device_skia.cc (right):

http://codereview.chromium.org/7273013/diff/21016/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_skia.cc:215: SkBitmap::Config config, int width,
int height,
nit: indent is wrong.