CL removing inheritance of SkDevice from PlatformDevice. Flavours of PlatformDevice classes now ...

CL removing inheritance of SkDevice from PlatformDevice.  

PlatformDevice is now a base interface, which is implemented by the various flavours of BitmapPlatformDevice, and VectorPlatformDevice.  The BitmapPlatformDevice and VectorPlatformDevice classes now inherit directly from SkDevice, or SkPDFDevice, as appropriate.

PlatformDevice helper functions access the PlatformDevice interface attached to a SkDevice via meta-data on the SkDevice.

BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98230
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98585

[Jeff Timanus, 2011-08-12 23:59:22]

PTAL.

Here's a start at what a post-inheritance PlatformDevice world would look like. 

I think the only serious points to address are the dangers posed by:
 - The change in onAccessBitmap.
 - Instances returned from onCreateCompatibleDevice lose their platform device
flavour.

I ran the layout tests (src@96545), and got the following failures:
http://pastebin.com/zPMQWH4c
 
It will take a bit of investigation to determine which of these are related
directly to this change.

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

http://codereview.chromium.org/7633040/diff/3007/skia/ext/vector_platform_dev...
skia/ext/vector_platform_device_skia.cc:64:
skia::EndPlatformPaint(raster_canvas_.get());
This was an example of where BitmapPlatformDevice::onAccessBitmap was required. 
The code was drawing from a GDI backed bitmap before calling EndPlatformPaint,
and so no gdiFlush would have been called.  By placing the EndPlatformPaint call
at the entrance to the routine, the problem should be fixed.

[vandebo (ex-Chrome), 2011-08-15 19:52:10]

http://codereview.chromium.org/7633040/diff/3007/skia/ext/vector_platform_dev...
File skia/ext/vector_platform_device_emf_win.h (right):

http://codereview.chromium.org/7633040/diff/3007/skia/ext/vector_platform_dev...
skia/ext/vector_platform_device_emf_win.h:21: class VectorPlatformDeviceEmf :
public PlatformDevice, public SkDevice {
Would it make sense to rename this to EmfDevice that only inherits SkDevice and
then create a new VectorPlatformDeviceEmf (one that mirrors
VectorPlatformDeviceSkia) ?

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

http://codereview.chromium.org/7633040/diff/3007/skia/ext/vector_platform_dev...
skia/ext/vector_platform_device_skia.cc:73: SkCanvas
pdf_canvas(GetOwningDevice());
Will using two different canvases (with different clips and transforms) confuse
the device if it uses setMatrixClip?  Seems like we might need to match the
matrix and clip in this canvas even though drawSprite doesn't really need them.

Another solution might be a static canvas method to draw a sprite to a device
without molesting the device's other state.

[Jeff Timanus, 2011-08-15 20:06:24]

http://codereview.chromium.org/7633040/diff/3007/skia/ext/vector_platform_dev...
File skia/ext/vector_platform_device_emf_win.h (right):

http://codereview.chromium.org/7633040/diff/3007/skia/ext/vector_platform_dev...
skia/ext/vector_platform_device_emf_win.h:21: class VectorPlatformDeviceEmf :
public PlatformDevice, public SkDevice {
On 2011/08/15 19:52:10, vandebo wrote:
> Would it make sense to rename this to EmfDevice that only inherits SkDevice
and
> then create a new VectorPlatformDeviceEmf (one that mirrors
> VectorPlatformDeviceSkia) ?

Yes.  That would make sense for all of the classes that truly need to override
SkDevice.  It would also remove the need for the logic to specify the ownership
parameters of the PlatformDevice instance during assignment to a SkDevice.

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

http://codereview.chromium.org/7633040/diff/3007/skia/ext/vector_platform_dev...
skia/ext/vector_platform_device_skia.cc:73: SkCanvas
pdf_canvas(GetOwningDevice());
On 2011/08/15 19:52:10, vandebo wrote:
> Will using two different canvases (with different clips and transforms)
confuse
> the device if it uses setMatrixClip?  Seems like we might need to match the
> matrix and clip in this canvas even though drawSprite doesn't really need
them.
> 
> Another solution might be a static canvas method to draw a sprite to a device
> without molesting the device's other state.

A third option is to make drawSprite a public entry point to SkDevice.  Are
there reasons why we should not take this approach?  Which is cleaner, from an
API perspective - public drawSprite call, or static method that preserves state?

[vandebo (ex-Chrome), 2011-08-15 20:13:56]

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

http://codereview.chromium.org/7633040/diff/3007/skia/ext/vector_platform_dev...
skia/ext/vector_platform_device_skia.cc:73: SkCanvas
pdf_canvas(GetOwningDevice());
On 2011/08/15 20:06:25, Jeff Timanus wrote:
> On 2011/08/15 19:52:10, vandebo wrote:
> > Will using two different canvases (with different clips and transforms)
> confuse
> > the device if it uses setMatrixClip?  Seems like we might need to match the
> > matrix and clip in this canvas even though drawSprite doesn't really need
> them.
> > 
> > Another solution might be a static canvas method to draw a sprite to a
device
> > without molesting the device's other state.
> 
> A third option is to make drawSprite a public entry point to SkDevice.  Are
> there reasons why we should not take this approach?  Which is cleaner, from an
> API perspective - public drawSprite call, or static method that preserves
state?
> 

I don't think either of these are particularly clean...  The consumer isn't
really meant to deal with drawing to the device directly, that's the job of
canvas.

Is there enough information exposed from canvas to clone the state into the
local canvas you already have?

There aren't any particular ramifications for PDF for any of these options, but
that might not be true for GPU.  Maybe we can get Brian to weigh in.

[alokp, 2011-08-15 20:38:46]

Sorry for being late to the party - I was out for a week.

I do not understand this change or the change that adds a matrix-clip observer
to skia. I understand that having all devices derive from PlatformDevice (which
in-turn derives from SkDevice) is bad, but sub-classing SkDevice directly is
fine. May be someone can explain the issues?

[Jeff Timanus, 2011-08-15 20:46:59]

On 2011/08/15 20:38:46, Alok Priyadarshi wrote:
> Sorry for being late to the party - I was out for a week.
> 
> I do not understand this change or the change that adds a matrix-clip observer
> to skia. I understand that having all devices derive from PlatformDevice
(which
> in-turn derives from SkDevice) is bad, but sub-classing SkDevice directly is
> fine. May be someone can explain the issues?

My understanding is that some of the classes in skia\ext are genuine SkDevice
implementations.  For example, the VectorEMF class overrides a significant
portion of SkDevice.  I believe that these classes should be considered on-par
with the gpu and pdf back-ends.

PlatformDevice, on the other hand, provides a set of helper routines that
augment a SkDevice instance for platform-specific behaviour.  By decoupling them
from the SkDevice inheritance, we can allow mixing different SkDevice
sub-classes with different PlatformDevice subclasses, and obtain the flexibility
we need when dropping in the GPU and PDF back-ends.

The need for the observer change in Skia is so that PlaformDevice instances can
receive notification to changes on the SkDevice to which they are bound.

[alokp, 2011-08-15 20:53:46]

> My understanding is that some of the classes in skia\ext are genuine SkDevice
> implementations.  For example, the VectorEMF class overrides a significant
> portion of SkDevice.  I believe that these classes should be considered on-par
> with the gpu and pdf back-ends.
> 
> PlatformDevice, on the other hand, provides a set of helper routines that
> augment a SkDevice instance for platform-specific behaviour.  By decoupling
them
> from the SkDevice inheritance, we can allow mixing different SkDevice
> sub-classes with different PlatformDevice subclasses, and obtain the
flexibility
> we need when dropping in the GPU and PDF back-ends.

In that case I will replace PlatformDevice with a set of functions, or a class
with just static helper functions. I would still have BitmapPlatformDevice
derive from SkDevice and override setMatrixClip, and hence no need for an
observer.

AFAICT the problem here is a common PlatformDevice that derives from SkDevice.
Individual devices in chrome should derive directly from SkDevice, SkGPUDevice,
or SkPDFDevice.

> The need for the observer change in Skia is so that PlaformDevice instances
can
> receive notification to changes on the SkDevice to which they are bound.

[Jeff Timanus, 2011-08-16 15:14:40]

On 2011/08/15 20:53:46, Alok Priyadarshi wrote:
> > My understanding is that some of the classes in skia\ext are genuine
SkDevice
> > implementations.  For example, the VectorEMF class overrides a significant
> > portion of SkDevice.  I believe that these classes should be considered
on-par
> > with the gpu and pdf back-ends.
> > 
> > PlatformDevice, on the other hand, provides a set of helper routines that
> > augment a SkDevice instance for platform-specific behaviour.  By decoupling
> them
> > from the SkDevice inheritance, we can allow mixing different SkDevice
> > sub-classes with different PlatformDevice subclasses, and obtain the
> flexibility
> > we need when dropping in the GPU and PDF back-ends.
> 
> In that case I will replace PlatformDevice with a set of functions, or a class
> with just static helper functions. I would still have BitmapPlatformDevice
> derive from SkDevice and override setMatrixClip, and hence no need for an
> observer.
> 
> AFAICT the problem here is a common PlatformDevice that derives from SkDevice.
> Individual devices in chrome should derive directly from SkDevice,
SkGPUDevice,
> or SkPDFDevice.

This sounds like a very reasonable plan.

A potential concern that comes to mind is that the PlatformDevice classes may
still be dependent on the type of class with which they are bound.  For example,
a BitmapPlatformDevice may only operate when bound with a BitmapDevice, not with
an arbitrary implementation of the SkDevice interface.

Would this constraint be a problem?

> 
> > The need for the observer change in Skia is so that PlaformDevice instances
> can
> > receive notification to changes on the SkDevice to which they are bound.

[alokp, 2011-08-16 21:01:23]

> > 
> > In that case I will replace PlatformDevice with a set of functions, or a
class
> > with just static helper functions. I would still have BitmapPlatformDevice
> > derive from SkDevice and override setMatrixClip, and hence no need for an
> > observer.
> > 
> > AFAICT the problem here is a common PlatformDevice that derives from
SkDevice.
> > Individual devices in chrome should derive directly from SkDevice,
> SkGPUDevice,
> > or SkPDFDevice.
> 
> This sounds like a very reasonable plan.
> 
> A potential concern that comes to mind is that the PlatformDevice classes may
> still be dependent on the type of class with which they are bound.  For
example,
> a BitmapPlatformDevice may only operate when bound with a BitmapDevice, not
with
> an arbitrary implementation of the SkDevice interface.
> 
> Would this constraint be a problem?
> 

I think I mistyped. PlatformDevice will not be subclassed. It will just be a set
of static functions. In fact I will just get rid of PlatformDevice and replace
them with a set of helper functions in skia namespace.

BitmapDevice will subclass from SkDevice. It will also have platform-specific
BitmapDeviceData as meta-data that will be used by helper functions to implement
BeginPlatformPaint.

[Jeff Timanus, 2011-08-17 18:51:12]

On 2011/08/16 21:01:23, Alok Priyadarshi wrote:
> > > 
> > > In that case I will replace PlatformDevice with a set of functions, or a
> class
> > > with just static helper functions. I would still have BitmapPlatformDevice
> > > derive from SkDevice and override setMatrixClip, and hence no need for an
> > > observer.
> > > 
> > > AFAICT the problem here is a common PlatformDevice that derives from
> SkDevice.
> > > Individual devices in chrome should derive directly from SkDevice,
> > SkGPUDevice,
> > > or SkPDFDevice.
> > 
> > This sounds like a very reasonable plan.
> > 
> > A potential concern that comes to mind is that the PlatformDevice classes
may
> > still be dependent on the type of class with which they are bound.  For
> example,
> > a BitmapPlatformDevice may only operate when bound with a BitmapDevice, not
> with
> > an arbitrary implementation of the SkDevice interface.
> > 
> > Would this constraint be a problem?
> > 
> 
> I think I mistyped. PlatformDevice will not be subclassed. It will just be a
set
> of static functions. In fact I will just get rid of PlatformDevice and replace
> them with a set of helper functions in skia namespace.

To follow up on what we mentioned during the stand-up:

I think the implementation may be more straight-forward if PlatformDevice is
still subclassed.  PlatformDevice would now be a base interface, not inheriting
from SkDevice, with different implementations for the various types of SkDevice
overrides in skia\ext.  This seems like the easiest approach, because the set of
structures required to implement the PlatformDevice set of routines is disjoint
among the Vector|Bitmap classes.

Also, I did a search through the code, and it appears that all of the
PlatformDevice routines are still in use in various parts of Chrome. 
PlatformDevice still needs to export all of the currently defined routines.  

On Windows, I found the following users of the PlatformDevice methods:

BeginPlatformPaint/EndPlatformPaint:  Called frequently.

DrawToNativeContext:
  skia/ext/canvas_paint_win.h
  views/controls/menu/menu_win.cc
  webkit/glue/webmediaplayer_impl.cc
  views/controls/table_view.cc
  views/controls/menu/native_menu_win.cc
  views/controls/table/native_table_win.cc
  webkit/tools/test_shell/webwidget_host_win.cc
  webkit/tools/test_shell/mac/webwidget_host.mm
  chrome/browser/renderer_host/render_widget_host_view_win.cc
  chrome/browser/ui/views/omnibox/omnibox_view_win.cc

MakeOpaque:
  ui/gfx/canvas_skia_win.cc
  webkit/plugins/ppapi/ppb_scrollbar_impl.cc
  chrome/browser/ui/views/tabs/native_view_photobooth_win.cc
  
IsNativeFontRenderingAllowed:
  third_party/WebKit/Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp

AlphaBlendUsed:
  chrome/renderer/print_web_view_helper_win.cc

> 
> BitmapDevice will subclass from SkDevice. It will also have platform-specific
> BitmapDeviceData as meta-data that will be used by helper functions to
implement
> BeginPlatformPaint.

I like this approach.  If there are no objections, I'll submit a follow-up CL
that implements these ideas.

[Jeff Timanus, 2011-08-18 19:44:24]

On 2011/08/17 18:51:12, Jeff Timanus wrote:
> On 2011/08/16 21:01:23, Alok Priyadarshi wrote:
> > > > 
> > > > In that case I will replace PlatformDevice with a set of functions, or a
> > class
> > > > with just static helper functions. I would still have
BitmapPlatformDevice
> > > > derive from SkDevice and override setMatrixClip, and hence no need for
an
> > > > observer.
> > > > 
> > > > AFAICT the problem here is a common PlatformDevice that derives from
> > SkDevice.
> > > > Individual devices in chrome should derive directly from SkDevice,
> > > SkGPUDevice,
> > > > or SkPDFDevice.
> > > 
> > > This sounds like a very reasonable plan.
> > > 
> > > A potential concern that comes to mind is that the PlatformDevice classes
> may
> > > still be dependent on the type of class with which they are bound.  For
> > example,
> > > a BitmapPlatformDevice may only operate when bound with a BitmapDevice,
not
> > with
> > > an arbitrary implementation of the SkDevice interface.
> > > 
> > > Would this constraint be a problem?
> > > 
> > 
> > I think I mistyped. PlatformDevice will not be subclassed. It will just be a
> set
> > of static functions. In fact I will just get rid of PlatformDevice and
replace
> > them with a set of helper functions in skia namespace.
> 
> To follow up on what we mentioned during the stand-up:
> 
> I think the implementation may be more straight-forward if PlatformDevice is
> still subclassed.  PlatformDevice would now be a base interface, not
inheriting
> from SkDevice, with different implementations for the various types of
SkDevice
> overrides in skia\ext.  This seems like the easiest approach, because the set
of
> structures required to implement the PlatformDevice set of routines is
disjoint
> among the Vector|Bitmap classes.
> 
> Also, I did a search through the code, and it appears that all of the
> PlatformDevice routines are still in use in various parts of Chrome. 
> PlatformDevice still needs to export all of the currently defined routines.  
> 
> On Windows, I found the following users of the PlatformDevice methods:
> 
> BeginPlatformPaint/EndPlatformPaint:  Called frequently.
> 
> DrawToNativeContext:
>   skia/ext/canvas_paint_win.h
>   views/controls/menu/menu_win.cc
>   webkit/glue/webmediaplayer_impl.cc
>   views/controls/table_view.cc
>   views/controls/menu/native_menu_win.cc
>   views/controls/table/native_table_win.cc
>   webkit/tools/test_shell/webwidget_host_win.cc
>   webkit/tools/test_shell/mac/webwidget_host.mm
>   chrome/browser/renderer_host/render_widget_host_view_win.cc
>   chrome/browser/ui/views/omnibox/omnibox_view_win.cc
> 
> MakeOpaque:
>   ui/gfx/canvas_skia_win.cc
>   webkit/plugins/ppapi/ppb_scrollbar_impl.cc
>   chrome/browser/ui/views/tabs/native_view_photobooth_win.cc
>   
> IsNativeFontRenderingAllowed:
>   third_party/WebKit/Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp
> 
> AlphaBlendUsed:
>   chrome/renderer/print_web_view_helper_win.cc
> 
> > 
> > BitmapDevice will subclass from SkDevice. It will also have
platform-specific
> > BitmapDeviceData as meta-data that will be used by helper functions to
> implement
> > BeginPlatformPaint.
> 
> I like this approach.  If there are no objections, I'll submit a follow-up CL
> that implements these ideas.

Patch set #4 implements the ideas discussed above (for Windows only). 
PlatformDevice is now a base interface, implemented by the BitmapPlatformDevice
and VectorPlatformDevice classes. 

Classes that are true SkDevice overrides also inherit from SkDevice.  These
classes are:  BitmapPlatformDevice, VectorPlatformDeviceEMF, and
VectorPlatformDeviceCairo.

VectorPlatformDeviceSkia only implements the PlatformDevice interface, and and
is bound directly with a SkPDFDevice.

The PlatformDevice instance is accessed from SkDevice instance via the
SkMetadata, just as previously.  In the case of devices implementing both
SkDevice, and PlatformDevice, the lifetimes are managed by the inheritance
mechanism, otherwise the lifetime of the PlatformDevice is controlled by a
function-pointer passed into the SkMetadata.

As Alok mentioned, this approach removes the need for the matrix-clip observer. 
However, the problematic call to drawSprite is still present in
VectorPlatformDeviceSkia::EndPlatformPaint.

Will upload a subsequent patch with the same changes for Linux and Mac, and
confirm that no layout/test failures are introduced.

[alokp, 2011-08-19 03:01:41]

Looks better. Is there still a need to have PlatformDevice decalred in three
different files?

http://codereview.chromium.org/7633040/diff/12002/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/7633040/diff/12002/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_linux.cc:78: BitmapPlatformDevice*
BitmapPlatformDevice::Create(int width, int height, bool is_opaque,
indenting

[Jeff Timanus, 2011-08-19 18:32:26]

On 2011/08/19 03:01:41, Alok Priyadarshi wrote:
> Looks better. Is there still a need to have PlatformDevice decalred in three
> different files?

No, I don't think there is a need to have the three separate headers.  I'll try
to coalesce them into a single header, with the appropriate #ifs.

> 
>
http://codereview.chromium.org/7633040/diff/12002/skia/ext/bitmap_platform_de...
> File skia/ext/bitmap_platform_device_linux.cc (right):
> 
>
http://codereview.chromium.org/7633040/diff/12002/skia/ext/bitmap_platform_de...
> skia/ext/bitmap_platform_device_linux.cc:78: BitmapPlatformDevice*
> BitmapPlatformDevice::Create(int width, int height, bool is_opaque,
> indenting

[vandebo (ex-Chrome), 2011-08-19 21:54:22]

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_linux.cc:78: BitmapPlatformDevice*
BitmapPlatformDevice::Create(int width, int height, bool is_opaque,
Formatting was correct before.

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_linux.cc:91: BitmapPlatformDevice*
BitmapPlatformDevice::Create(int width, int height, bool is_opaque) {
Formatting was correct before.

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_linux.cc:106: BitmapPlatformDevice*
BitmapPlatformDevice::Create(int width, int height, bool is_opaque,
Formatting was correct before.

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

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_mac.cc:186: }
Does this need 'SetPlatformDevice(this, this, false);' like the Linux version?

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

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.cc:156: int height,
nit: indent

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.cc:180: SetPlatformDevice(this, this,
false);
This generally seems pretty fragile, is there any to do this automagically?

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

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.h:41: static BitmapPlatformDevice*
create(int width,int height, bool is_opaque,
width, int

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

http://codereview.chromium.org/7633040/diff/18001/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_emf_win.cc:94: //return new
VectorPlatformDeviceEmf(dc, bitmap);
remove

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

http://codereview.chromium.org/7633040/diff/18001/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_skia.cc:25: SetPlatformDevice(pdf_device, this,
true);
This is the only place where ownership is transfered.  Would making
VectorPlatformDeviceSkia inherit from SkPDFDevice instead of taking one,
eliminate the third arg?  If so, I think that's doable... looks like the only
call site for the constructor is pdf_metafile_skia, which makes the SkPDFDevice
immediately prior.

I think that would also solve the drawSprite issue...

[Jeff Timanus, 2011-08-20 02:31:04]

Thanks for the reviews.  Addressed the comments, and uploaded a new patch that
removes the various platform-specific platform_device_{linux|mac|win}.h files.

The bitmap_platform_device headers could potentially also be unified, but at
this point, I'd rather get these changes in and do some follow-up work.

The most recent change required no modification to skia.

http://codereview.chromium.org/7633040/diff/12002/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/7633040/diff/12002/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_linux.cc:78: BitmapPlatformDevice*
BitmapPlatformDevice::Create(int width, int height, bool is_opaque,
On 2011/08/19 03:01:41, Alok Priyadarshi wrote:
> indenting

Done.

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_linux.cc:78: BitmapPlatformDevice*
BitmapPlatformDevice::Create(int width, int height, bool is_opaque,
On 2011/08/19 21:54:22, vandebo wrote:
> Formatting was correct before.

Done.

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_linux.cc:91: BitmapPlatformDevice*
BitmapPlatformDevice::Create(int width, int height, bool is_opaque) {
On 2011/08/19 21:54:22, vandebo wrote:
> Formatting was correct before.

Done.

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_linux.cc:106: BitmapPlatformDevice*
BitmapPlatformDevice::Create(int width, int height, bool is_opaque,
On 2011/08/19 21:54:22, vandebo wrote:
> Formatting was correct before.

Done.

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

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_mac.cc:186: }
On 2011/08/19 21:54:22, vandebo wrote:
> Does this need 'SetPlatformDevice(this, this, false);' like the Linux version?

Done.

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

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.cc:156: int height,
On 2011/08/19 21:54:22, vandebo wrote:
> nit: indent

Done.

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.cc:180: SetPlatformDevice(this, this,
false);
On 2011/08/19 21:54:22, vandebo wrote:
> This generally seems pretty fragile, is there any to do this automagically?

Yes, I was also worried about the fragile requirement to register the
PlatformDevice properly.

After thinking about your suggestion to allow VectorPlatformDeviceSkia to also
inherit from SkDevice, the need to specify the ownership is no longer required. 

I pondered having the PlatformDevice constructor also register itself with the
bound SkDevice's meta-data, but that is a problem due to multiple inheritance. 
The style guide is inherit in alphabetical order, but that means that the
SkDevice is constructed AFTER the PlatformDevice, which means that the
PlatformDevice would try to package itself in the metadata of a
yet-to-be-constructed SkDevice.  

I think it's best if these classes still explicitly call SetPlatformDevice in
their constructors.

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

http://codereview.chromium.org/7633040/diff/18001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.h:41: static BitmapPlatformDevice*
create(int width,int height, bool is_opaque,
On 2011/08/19 21:54:22, vandebo wrote:
> width, int

Done.

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

http://codereview.chromium.org/7633040/diff/18001/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_emf_win.cc:94: //return new
VectorPlatformDeviceEmf(dc, bitmap);
On 2011/08/19 21:54:22, vandebo wrote:
> remove

Done.

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

http://codereview.chromium.org/7633040/diff/18001/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_skia.cc:25: SetPlatformDevice(pdf_device, this,
true);
On 2011/08/19 21:54:22, vandebo wrote:
> This is the only place where ownership is transfered.  Would making
> VectorPlatformDeviceSkia inherit from SkPDFDevice instead of taking one,
> eliminate the third arg?  If so, I think that's doable... looks like the only
> call site for the constructor is pdf_metafile_skia, which makes the
SkPDFDevice
> immediately prior.
> 
> I think that would also solve the drawSprite issue...

Yes, that's a very good suggestion.  It clears up a lot of the issues.

Done.

[Jeff Timanus, 2011-08-20 02:51:47]

Patch set 8 seems to be introducing a printing regression on Linux (and possibly
other platforms):

PrintWebViewHelperPreviewTest.OnPrintPreview is crashing with the following
stack:

	base::debug::StackTrace::StackTrace() [0x137885e]
	logging::LogMessage::~LogMessage() [0x139b46a]
	SkDebugf_FileLine() [0x1721953]
	SkDeque::F2BIter::next() [0x16a244e]
	SkClipStack::operator=() [0x169f166]
	GraphicStackState::GraphicStackState() [0x1705b79]
	SkPDFDevice::copyContentEntriesToData() [0x17039f2]
	SkPDFDevice::copyContentToData() [0x1703bc0]
	SkPDFDevice::content() [0x170396f]
	SkPDFPage::finalizePage() [0x3aab656]
	SkPDFDocument::emitPDF() [0x3aaa0ce]
	printing::PdfMetafileSkia::FinishDocument() [0x326704c]
	PrintWebViewHelper::PrintPreviewContext::FinalizePreviewDocument() [0x12df1bd]
	PrintWebViewHelper::FinalizePreviewDocument() [0x12dbf70]
	PrintWebViewHelper::OnContinuePreview() [0x12dbee8]
	PrintWebViewHelperPreviewTest_OnPrintPreview_Test::TestBody() [0x713f48]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x1791863]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x178ee52]
	testing::Test::Run() [0x1784182]
	testing::TestInfo::Run() [0x17849a6]
	testing::TestCase::Run() [0x178509c]
	testing::internal::UnitTestImpl::RunAllTests() [0x1789e73]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x17926af]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x178f643]
	testing::UnitTest::Run() [0x1788990]
	base::TestSuite::Run() [0x141054f]
	main [0x7582ea]
	0x7f60858bec4d
	0x439499

[Jeff Timanus, 2011-08-23 16:14:32]

Patch 11 corrects the crash mentioned below.

The problem was incorrect reference counting of newly constructed PDF devices.

PTAL.

On 2011/08/20 02:51:47, Jeff Timanus wrote:
> Patch set 8 seems to be introducing a printing regression on Linux (and
possibly
> other platforms):
> 
> PrintWebViewHelperPreviewTest.OnPrintPreview is crashing with the following
> stack:
> 
> 	base::debug::StackTrace::StackTrace() [0x137885e]
> 	logging::LogMessage::~LogMessage() [0x139b46a]
> 	SkDebugf_FileLine() [0x1721953]
> 	SkDeque::F2BIter::next() [0x16a244e]
> 	SkClipStack::operator=() [0x169f166]
> 	GraphicStackState::GraphicStackState() [0x1705b79]
> 	SkPDFDevice::copyContentEntriesToData() [0x17039f2]
> 	SkPDFDevice::copyContentToData() [0x1703bc0]
> 	SkPDFDevice::content() [0x170396f]
> 	SkPDFPage::finalizePage() [0x3aab656]
> 	SkPDFDocument::emitPDF() [0x3aaa0ce]
> 	printing::PdfMetafileSkia::FinishDocument() [0x326704c]
> 	PrintWebViewHelper::PrintPreviewContext::FinalizePreviewDocument()
[0x12df1bd]
> 	PrintWebViewHelper::FinalizePreviewDocument() [0x12dbf70]
> 	PrintWebViewHelper::OnContinuePreview() [0x12dbee8]
> 	PrintWebViewHelperPreviewTest_OnPrintPreview_Test::TestBody() [0x713f48]
> 	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x1791863]
> 	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x178ee52]
> 	testing::Test::Run() [0x1784182]
> 	testing::TestInfo::Run() [0x17849a6]
> 	testing::TestCase::Run() [0x178509c]
> 	testing::internal::UnitTestImpl::RunAllTests() [0x1789e73]
> 	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x17926af]
> 	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x178f643]
> 	testing::UnitTest::Run() [0x1788990]
> 	base::TestSuite::Run() [0x141054f]
> 	main [0x7582ea]
> 	0x7f60858bec4d
> 	0x439499

[vandebo (ex-Chrome), 2011-08-23 17:40:35]

LGTM with nits.

http://codereview.chromium.org/7633040/diff/31001/printing/pdf_metafile_skia.cc
File printing/pdf_metafile_skia.cc (right):

http://codereview.chromium.org/7633040/diff/31001/printing/pdf_metafile_skia....
printing/pdf_metafile_skia.cc:64: // TODO(ctguil): Refactor: don't create the
PDF device explicitly here.
nit: remove now old comment.

http://codereview.chromium.org/7633040/diff/31001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

http://codereview.chromium.org/7633040/diff/31001/skia/ext/platform_device.h#...
skia/ext/platform_device.h:66: #if defined(WIN32)
The standard os defines are OS_WIN, OS_MACOSX, OS_LINUX, and OS_POSIX (for mac +
linux) - see build/build_config.h.  Not sure why these aren't used above.

http://codereview.chromium.org/7633040/diff/31001/skia/ext/platform_device.h#...
skia/ext/platform_device.h:116: virtual void DrawToNativeContext(PlatformSurface
surface, int x, int y,
No platform has a meaningful implementation of this, make it pure virtual?

[Jeff Timanus, 2011-08-23 21:13:02]

Done.

Will submit as soon as I get some green trys.

http://codereview.chromium.org/7633040/diff/31001/printing/pdf_metafile_skia.cc
File printing/pdf_metafile_skia.cc (right):

http://codereview.chromium.org/7633040/diff/31001/printing/pdf_metafile_skia....
printing/pdf_metafile_skia.cc:64: // TODO(ctguil): Refactor: don't create the
PDF device explicitly here.
On 2011/08/23 17:40:35, vandebo wrote:
> nit: remove now old comment.

Done.

http://codereview.chromium.org/7633040/diff/31001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

http://codereview.chromium.org/7633040/diff/31001/skia/ext/platform_device.h#...
skia/ext/platform_device.h:66: #if defined(WIN32)
On 2011/08/23 17:40:35, vandebo wrote:
> The standard os defines are OS_WIN, OS_MACOSX, OS_LINUX, and OS_POSIX (for mac
+
> linux) - see build/build_config.h.  Not sure why these aren't used above.

Done.

http://codereview.chromium.org/7633040/diff/31001/skia/ext/platform_device.h#...
skia/ext/platform_device.h:116: virtual void DrawToNativeContext(PlatformSurface
surface, int x, int y,
On 2011/08/23 17:40:35, vandebo wrote:
> No platform has a meaningful implementation of this, make it pure virtual?

The routine was previously pure virtual on windows and mac, but not on linux. 
The linux VectorPlatformDevice, and BitmapPlatformDevice did not have
implementations.

I shuffled the code around so that the routine is pure virtual, and the linux
vector and bitmap classes provide implementations that SkASSERT, which does seem
a bit cleaner, and more explicit.

[vandebo (ex-Chrome), 2011-08-23 21:16:41]

Cool, it'll be great to have this fixed.

http://codereview.chromium.org/7633040/diff/38001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

http://codereview.chromium.org/7633040/diff/38001/skia/ext/platform_device.h#...
skia/ext/platform_device.h:89: #elif defined(OS_LINUX)
There are OS_ defines for the BSD's and Sun, if you feel like adding them.

[alokp, 2011-08-24 17:03:02]

lgtm

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

http://codereview.chromium.org/7633040/diff/38001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.cc:157: BitmapPlatformDevice* device =
BitmapPlatformDevice::create(screen_dc, width, height,
nit: formatting

http://codereview.chromium.org/7633040/diff/38001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

http://codereview.chromium.org/7633040/diff/38001/skia/ext/platform_device.h#...
skia/ext/platform_device.h:83: class SK_API PlatformDevice {
This is OK for now. Medium term I would like to get rid of this hierarchy.
PlatformDevice should be replaced by a set of helper functions.

http://codereview.chromium.org/7633040/diff/38001/skia/ext/platform_device.h#...
skia/ext/platform_device.h:86: typedef HDC PlatformSurface;
definition of PlatformSurface is duplicated here.

[Jeff Timanus, 2011-08-24 22:09:58]

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

http://codereview.chromium.org/7633040/diff/38001/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.cc:157: BitmapPlatformDevice* device =
BitmapPlatformDevice::create(screen_dc, width, height,
On 2011/08/24 17:03:02, Alok Priyadarshi wrote:
> nit: formatting

Done.

http://codereview.chromium.org/7633040/diff/38001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

http://codereview.chromium.org/7633040/diff/38001/skia/ext/platform_device.h#...
skia/ext/platform_device.h:83: class SK_API PlatformDevice {
On 2011/08/24 17:03:02, Alok Priyadarshi wrote:
> This is OK for now. Medium term I would like to get rid of this hierarchy.
> PlatformDevice should be replaced by a set of helper functions.

This change is a good step in that direction.

I'm wondering if I'm being thick-headed, but if all of these routines were
helper functions, taking a SkDevice as an argument, wouldn't we have to roll our
own type-inference?

A struct that contains the union of all of the fields used by all of the
different types of platform device implementations could be created, and stashed
in the devices meta-data.  However, for any of the calls, we would have to
switch on the type of device being serviced to take the appropriate action.

Isn't having a class heirarchy cleaner than switching on the type of class?

http://codereview.chromium.org/7633040/diff/38001/skia/ext/platform_device.h#...
skia/ext/platform_device.h:86: typedef HDC PlatformSurface;
On 2011/08/24 17:03:02, Alok Priyadarshi wrote:
> definition of PlatformSurface is duplicated here.

Done.

http://codereview.chromium.org/7633040/diff/38001/skia/ext/platform_device.h#...
skia/ext/platform_device.h:89: #elif defined(OS_LINUX)
On 2011/08/23 21:16:43, vandebo wrote:
> There are OS_ defines for the BSD's and Sun, if you feel like adding them.

Done.

[commit-bot: I haz the power, 2011-08-25 04:06:11]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win, revision is 98154, job name
was 7633040-48001 (retry) (retry) (retry) (retry).

[Jeff Timanus, 2011-08-25 19:52:50]

Commit was reverted because it broke the shared library builds.

A Skia change is required to properly export SkPDFDevice::setDrawingArea.  See
landed skia change:  http://code.google.com/p/skia/source/detail?r=2172

Will re-submit once skia rolls past 2172.