Removal of dependencies on PlatformDevice classes.

This change implements a first pass in the effort to remove the dependency of PlatformDevice within Chrome.  The Skia library now provides multiple back-ends for the SkDevice class, so PlatformDevice's inheritance of SkDevice, and the assumption of instances of PlatformDevice limits the use of these new back-ends.

A new set of helper functions is provided for the PlatformDevice entry points.  Upon construction of a PlatformDevice, a pointer to the interface is cached in the parent SkDevice's SkMetaData.  The new helper functions forward calls to the interface cached in the metadata.

BUG=NONE
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86625
Reverted:  http://src.chromium.org/viewvc/chrome?view=rev&revision=86625
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86823

[Jeff Timanus, 2011-05-18 01:53:51]

Hi Alok,

Here's the CL that we've been discussing.  I'm still ironing out some try-bot
issues, but feel free to take a look.  There are a lot of files, but most of the
changes are trivial.

I've tried to remove all use of direct use PlatformDevice outside of Skia.

Jeff

[alokp, 2011-05-18 03:25:42]

A couple of minor questions.

http://codereview.chromium.org/7019013/diff/1223/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/7019013/diff/1223/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_win.cc:184:
skia::platform_util::DrawToHDC(&canvas.getTopDevice(), paint_dc,
Could we rename this to DrawToNativeContext and change the argument from device
to canvas?

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

http://codereview.chromium.org/7019013/diff/1223/skia/ext/platform_canvas.h#n...
skia/ext/platform_canvas.h:72: skia::platform_util::PlatformSurface
beginPlatformPaint() const;
do we need these member functions? Can't we directly use the functions below in
skia namespace?

http://codereview.chromium.org/7019013/diff/1223/skia/ext/platform_canvas.h#n...
skia/ext/platform_canvas.h:88: SkDevice& getTopDevice() const;
SkCanvas also has a function named getTopDevice(). It would also return the same
object. One caveat is that when I used it last time, it caused many regressions.
I guess getting the device through an iterator changes a few other things as
well. It would be better to use SKCanvas::getTopDevice() if we can.

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

http://codereview.chromium.org/7019013/diff/1223/skia/ext/platform_device.h#n...
skia/ext/platform_device.h:33: namespace platform_util {
is this namespace required? can we keep things in skia namespace itself?

http://codereview.chromium.org/7019013/diff/1223/skia/ext/platform_device.h#n...
skia/ext/platform_device.h:54: SK_API void SetPlatformDevice(SkDevice* device,
PlatformDevice* behaviour);
this is a little confusing. PlatformBehaviour and PlatformDevice are used
interchangeably.

http://codereview.chromium.org/7019013/diff/1223/skia/ext/platform_device.h#n...
skia/ext/platform_device.h:74: SK_API SkColor GetColorAt(SkDevice* device, int
x, int y);
this function does not seem to be doing anything useful. I think SkBitmap has
functions that does exactly the same thing.

[Jeff Timanus, 2011-05-18 22:10:27]

Thanks for your review, Alok.

Mike, can you comment on Alok's suggestion to replace
PlatformCanvas::getTopPlatformDevice with SkCanvas::getTopDevice?  I had a look,
and it seems that the two routines are doing fundamentally different things.

http://codereview.chromium.org/7019013/diff/1223/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/7019013/diff/1223/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_win.cc:184:
skia::platform_util::DrawToHDC(&canvas.getTopDevice(), paint_dc,
On 2011/05/18 03:25:42, Alok Priyadarshi wrote:
> Could we rename this to DrawToNativeContext and change the argument from
device
> to canvas?

Done.  I added a new routine in platform_canvas that forwards to this call, and
changed all call-sites to use the canvas version.

To keep the code uniform, I changed DrawContext (platform_device_mac), and
drawToHDC(platform_device_win) to DrawToNativeContext, and added a no-op stub to
the linux version.

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

http://codereview.chromium.org/7019013/diff/1223/skia/ext/platform_canvas.h#n...
skia/ext/platform_canvas.h:72: skia::platform_util::PlatformSurface
beginPlatformPaint() const;
On 2011/05/18 03:25:42, Alok Priyadarshi wrote:
> do we need these member functions? Can't we directly use the functions below
in
> skia namespace?

Yes, the functions below can be directly used.  

Done.

http://codereview.chromium.org/7019013/diff/1223/skia/ext/platform_canvas.h#n...
skia/ext/platform_canvas.h:88: SkDevice& getTopDevice() const;
On 2011/05/18 03:25:42, Alok Priyadarshi wrote:
> SkCanvas also has a function named getTopDevice(). It would also return the
same
> object. One caveat is that when I used it last time, it caused many
regressions.
> I guess getting the device through an iterator changes a few other things as
> well. It would be better to use SKCanvas::getTopDevice() if we can.

I looked into the difference of behaviour between SkCanvas::getTopDevice, and
this code, and it seems that the layer iter is performing quite a few operations
on the canvas and top device:
  - SkCanvas::prepareForDeviceDraw
  - SkDevice::accessBitmap(true)

I would prefer to leave removal of this override until aver this CL has landed,
as the changes to ensure the correct behaviour using SkCanvas::getTopDevice
appear non-trivial.

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

http://codereview.chromium.org/7019013/diff/1223/skia/ext/platform_device.h#n...
skia/ext/platform_device.h:33: namespace platform_util {
On 2011/05/18 03:25:42, Alok Priyadarshi wrote:
> is this namespace required? can we keep things in skia namespace itself?

No, it's not required.  I added it to further encapsulate these routines.

Removed.

http://codereview.chromium.org/7019013/diff/1223/skia/ext/platform_device.h#n...
skia/ext/platform_device.h:54: SK_API void SetPlatformDevice(SkDevice* device,
PlatformDevice* behaviour);
On 2011/05/18 03:25:42, Alok Priyadarshi wrote:
> this is a little confusing. PlatformBehaviour and PlatformDevice are used
> interchangeably.

My mistake.  Corrected.

http://codereview.chromium.org/7019013/diff/1223/skia/ext/platform_device.h#n...
skia/ext/platform_device.h:74: SK_API SkColor GetColorAt(SkDevice* device, int
x, int y);
On 2011/05/18 03:25:42, Alok Priyadarshi wrote:
> this function does not seem to be doing anything useful. I think SkBitmap has
> functions that does exactly the same thing.

Removed.  Turned out that there were only very few call-sites.

See src\chrome\browser\ui\views\theme_helpers.cc

[alokp, 2011-05-19 18:15:15]

looking much better

http://codereview.chromium.org/7019013/diff/8001/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/native_view_photobooth_win.cc (right):

http://codereview.chromium.org/7019013/diff/8001/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/native_view_photobooth_win.cc:114:
&canvas->AsCanvasSkia()->getTopDevice(),
could skia::MakeOpaque take canvas argument instead of device?

http://codereview.chromium.org/7019013/diff/8001/chrome/browser/ui/views/them...
File chrome/browser/ui/views/theme_helpers.cc (right):

http://codereview.chromium.org/7019013/diff/8001/chrome/browser/ui/views/them...
chrome/browser/ui/views/theme_helpers.cc:68: SkAutoLockPixels lock(bitmap);
I do not think you need to lock pixels here.

http://codereview.chromium.org/7019013/diff/8001/content/renderer/webplugin_d...
File content/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/7019013/diff/8001/content/renderer/webplugin_d...
content/renderer/webplugin_delegate_proxy.cc:853: CGContextRef bg_context =
skia::GetBitmapContext(
skia::GetBitmapContext should be replaced by skia::BeginPlatformPaint

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

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_canvas.h#n...
skia/ext/platform_canvas.h:124: SK_API skia::PlatformSurface BeginPlatformPaint(
nit: the whole line can fit in 80 chars.

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_canvas_uni...
File skia/ext/platform_canvas_unittest.cc (right):

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_canvas_uni...
skia/ext/platform_canvas_unittest.cc:258:
platform_util::MakeOpaque(&canvas.getTopDevice(), kInnerX, kInnerY,
platform_util?

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_device.cc
File skia/ext/platform_device.cc (right):

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_device.cc#...
skia/ext/platform_device.cc:16: SkMetaData& meta_data = device->getMetaData();
This is still a bit confusing but OK for the first iteration. I think the
meta-data should really be BitmapPlatformDeviceData.

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

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_device.h#n...
skia/ext/platform_device.h:12: #if defined(WIN32)
This is fine for the first iteration. But I would eventually like to get rid of
PlatformDevice. BitmapPlatformDevice could directly derive from SkDevice.
Likewise VectorPlatformDevice could directly derive from SkDevice.

[Jeff Timanus, 2011-05-19 20:38:28]

Thanks again for your review.  Addressed your comments.

Please take another look.

http://codereview.chromium.org/7019013/diff/8001/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/native_view_photobooth_win.cc (right):

http://codereview.chromium.org/7019013/diff/8001/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/native_view_photobooth_win.cc:114:
&canvas->AsCanvasSkia()->getTopDevice(),
On 2011/05/19 18:15:15, Alok Priyadarshi wrote:
> could skia::MakeOpaque take canvas argument instead of device?

Done.

http://codereview.chromium.org/7019013/diff/8001/chrome/browser/ui/views/them...
File chrome/browser/ui/views/theme_helpers.cc (right):

http://codereview.chromium.org/7019013/diff/8001/chrome/browser/ui/views/them...
chrome/browser/ui/views/theme_helpers.cc:68: SkAutoLockPixels lock(bitmap);
On 2011/05/19 18:15:15, Alok Priyadarshi wrote:
> I do not think you need to lock pixels here.

According to the docs in SkBitmap.h on getColor, the result is undefined if the
pixels have not been locked.  Is there an assumption about the state of a bitmap
returned from accessBitmap that means no lock is required?

http://codereview.chromium.org/7019013/diff/8001/content/renderer/webplugin_d...
File content/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/7019013/diff/8001/content/renderer/webplugin_d...
content/renderer/webplugin_delegate_proxy.cc:853: CGContextRef bg_context =
skia::GetBitmapContext(
On 2011/05/19 18:15:15, Alok Priyadarshi wrote:
> skia::GetBitmapContext should be replaced by skia::BeginPlatformPaint

Done.

I still kept the GetBitmapContext helper function around for it's current use in
webkit glue: 
http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/webkit/glue/webkit_g...

I did not want to introduce an un-paired BeginPlatformPaint call.

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

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_canvas.h#n...
skia/ext/platform_canvas.h:124: SK_API skia::PlatformSurface BeginPlatformPaint(
On 2011/05/19 18:15:15, Alok Priyadarshi wrote:
> nit: the whole line can fit in 80 chars.

Done.

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_canvas_uni...
File skia/ext/platform_canvas_unittest.cc (right):

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_canvas_uni...
skia/ext/platform_canvas_unittest.cc:258:
platform_util::MakeOpaque(&canvas.getTopDevice(), kInnerX, kInnerY,
On 2011/05/19 18:15:15, Alok Priyadarshi wrote:
> platform_util?

Corrected.  Thanks for catching that.

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_device.cc
File skia/ext/platform_device.cc (right):

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_device.cc#...
skia/ext/platform_device.cc:16: SkMetaData& meta_data = device->getMetaData();
On 2011/05/19 18:15:15, Alok Priyadarshi wrote:
> This is still a bit confusing but OK for the first iteration. I think the
> meta-data should really be BitmapPlatformDeviceData.

The reason I place the meta-data here is so that all of the overrides of
PlatformDevice behaviour in the VectorPlatformDevice classes.  

If we want to call BeginPlatformPaint on a SkDevice* (that's 
 really a VectorPlaformDevice), then it needs to somehow lookup the correct
functions to call.  The VectorPlatformDevice classes have no equivalent of
BitmapPlatformDevice data.

Reviewing the code, I see that there could be a common PlatformData class that
could provide a union of the data housed in VectorPlatformDevice and
BitmapPlatformDevice.

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

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_device.h#n...
skia/ext/platform_device.h:12: #if defined(WIN32)
On 2011/05/19 18:15:15, Alok Priyadarshi wrote:
> This is fine for the first iteration. But I would eventually like to get rid
of
> PlatformDevice. BitmapPlatformDevice could directly derive from SkDevice.
> Likewise VectorPlatformDevice could directly derive from SkDevice.

Noted.  Will start working on a follow-up CL that tries to implement your
suggestion more cleanly than my previous effort.

[alokp, 2011-05-19 21:52:27]

lgtm

You would need approval from various OWNERS and of course reed@.

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_device.cc
File skia/ext/platform_device.cc (right):

http://codereview.chromium.org/7019013/diff/8001/skia/ext/platform_device.cc#...
skia/ext/platform_device.cc:16: SkMetaData& meta_data = device->getMetaData();
On 2011/05/19 20:38:29, Jeff Timanus wrote:
> On 2011/05/19 18:15:15, Alok Priyadarshi wrote:
> > This is still a bit confusing but OK for the first iteration. I think the
> > meta-data should really be BitmapPlatformDeviceData.
> 
> The reason I place the meta-data here is so that all of the overrides of
> PlatformDevice behaviour in the VectorPlatformDevice classes.  
> 
> If we want to call BeginPlatformPaint on a SkDevice* (that's 
>  really a VectorPlaformDevice), then it needs to somehow lookup the correct
> functions to call.  The VectorPlatformDevice classes have no equivalent of
> BitmapPlatformDevice data.
> 
> Reviewing the code, I see that there could be a common PlatformData class that
> could provide a union of the data housed in VectorPlatformDevice and
> BitmapPlatformDevice.

Yes. A PlatformData class would be perfect.

[Jeff Timanus, 2011-05-19 22:03:54]

+OWNERS reviewers

Please take a look.  Lots of small changes, but no expected change in
functionality.

[Jeff Timanus, 2011-05-19 22:06:25]

+jamesr, tony, -jhawkins

[jamesr, 2011-05-19 22:20:51]

I took a look at the */renderer/* and webkit/* changes and they seem fine,
although it looks like we have at least an unmatched
BeginPlatformPaint/EndPlatformPaint pair in webwidget_host_gtk.cc which makes me
think we should have a helper class for that like SkAutoLockPixels.

http://codereview.chromium.org/7019013/diff/16137/content/renderer/webplugin_...
File content/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/7019013/diff/16137/content/renderer/webplugin_...
content/renderer/webplugin_delegate_proxy.cc:909:
skia::EndPlatformPaint(background_store_canvas_.get());
it seems that here and in other spots that it'd be really handy to have a
stack-allocated helper class to pair up the
skia::BeginPlatformPaint/skia::EndPlatformPaint() calls so you don't have to
worry about matching them by hand.

http://codereview.chromium.org/7019013/diff/16137/webkit/tools/test_shell/web...
File webkit/tools/test_shell/webwidget_host_gtk.cc (right):

http://codereview.chromium.org/7019013/diff/16137/webkit/tools/test_shell/web...
webkit/tools/test_shell/webwidget_host_gtk.cc:405: cairo_t* source_surface =
skia::BeginPlatformPaint(canvas_.get());
where's the EndPlatformPaint() for this call?  Someone (apologies, I forget who)
recently added some DCHECK()s to try to ensure the calls are always paired

[jam, 2011-05-19 22:58:48]

content lgtm

[reed1, 2011-05-20 14:22:38]

PlatformCanvas has

SkDevice& getTopDevice() const;

SkCanvas has

SkDevice* getTopDevice() const;

If you think you need a different function in your subclass, shouldn't it have a
different name (or if you mean to shadow the base class, it should have the same
return type).

[alokp, 2011-05-20 15:31:46]

On 2011/05/20 14:22:38, reed1 wrote:
> PlatformCanvas has
> 
> SkDevice& getTopDevice() const;
> 
> SkCanvas has
> 
> SkDevice* getTopDevice() const;
> 
> If you think you need a different function in your subclass, shouldn't it have
a
> different name (or if you mean to shadow the base class, it should have the
same
> return type).

Ideally we should be able to use SkCanvas::getTopDevice() instead of our custom
function that uses device iterator. But the last time I tried to use
SkCanvas::getTopDevice(), it produced many regressions even though both return
the same object. I guess the iterator path does something more than just
retrieve the top device. May be we only need to use the iterator path
BeginPlatformPaint() and can use SkCanvas::getTopDevice() elsewhere.

For this patch I think it would be OK to just replace member function
PlatformCanvas::getTopDevice() with a non-member function
skia::GetTopDevice(const SkCanvas&).

[reed1, 2011-05-20 15:33:36]

That at least would remove the shadowing.

[Jeff Timanus, 2011-05-24 16:50:28]

Thanks for your review, James.

I added a new class, skia::ScopedPlatformPaint to provide the auto-begin-end
functionality you suggested.  I went through the code and added it to the
majority of the begin/end calls.

http://codereview.chromium.org/7019013/diff/16137/content/renderer/webplugin_...
File content/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/7019013/diff/16137/content/renderer/webplugin_...
content/renderer/webplugin_delegate_proxy.cc:909:
skia::EndPlatformPaint(background_store_canvas_.get());
On 2011/05/19 22:20:52, jamesr wrote:
> it seems that here and in other spots that it'd be really handy to have a
> stack-allocated helper class to pair up the
> skia::BeginPlatformPaint/skia::EndPlatformPaint() calls so you don't have to
> worry about matching them by hand.

I added ScopedPlatformPaint to skia\ext\platform_canvas.h to do exactly that. 
Also went through the code and changed paired calls to begin/end to make use of
the class.

http://codereview.chromium.org/7019013/diff/16137/webkit/tools/test_shell/web...
File webkit/tools/test_shell/webwidget_host_gtk.cc (right):

http://codereview.chromium.org/7019013/diff/16137/webkit/tools/test_shell/web...
webkit/tools/test_shell/webwidget_host_gtk.cc:405: cairo_t* source_surface =
skia::BeginPlatformPaint(canvas_.get());
On 2011/05/19 22:20:52, jamesr wrote:
> where's the EndPlatformPaint() for this call?  Someone (apologies, I forget
who)
> recently added some DCHECK()s to try to ensure the calls are always paired

Thanks for catching that, James.

Fixed.

http://codereview.chromium.org/7019013/diff/17067/chrome/browser/ui/gtk/tabs/...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/7019013/diff/17067/chrome/browser/ui/gtk/tabs/...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:659:
skia::BeginPlatformPaint(&canvas)));
This code does not have a end platform paint pair.  I think that the GTK path is
assuming that it is safe to have an unmatched pair.  A design that needs to be
changed?

http://codereview.chromium.org/7019013/diff/17067/ui/gfx/canvas_skia_linux.cc
File ui/gfx/canvas_skia_linux.cc (right):

http://codereview.chromium.org/7019013/diff/17067/ui/gfx/canvas_skia_linux.cc...
ui/gfx/canvas_skia_linux.cc:386: cairo_t* cr =
scoped_platform_paint.GetPlatformSurface();
This path was also missing a paired endPlatformPaint.  My changes will now
introduce one at the end of the scope.

[Jeff Timanus, 2011-05-24 17:57:48]

On 2011/05/20 15:33:36, reed1 wrote:
> That at least would remove the shadowing.

Patch 16 removes PlatformCanvas::getTopDevice, and replaces it with a helper
function, as suggested by Alok.

Please take a final look.

[alokp, 2011-05-24 19:24:51]

lgtm

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

http://codereview.chromium.org/7019013/diff/24012/skia/ext/platform_canvas.h#...
skia/ext/platform_canvas.h:106: SkDevice* GetTopDevice(const SkCanvas* canvas);
nit: per style guide, const arguments are passed by reference.

[Jeff Timanus, 2011-05-24 21:54:17]

+ More Owners:

Could the OWNERS please review their respective files in this CL.

I've computed the necessary ownership coverage as follows:

ben, sky - views, ui/base
avi, brettw - chrome/browser/tab_contents
ben - chrome/browser
pkasting, sky - chrome/browser/ui
erg, estate, evan - chrome/browser/ui/gtk
darin - webkit
tony - webkit/tools/test_shell

[jamesr, 2011-05-24 21:57:10]

LGTM for content/renderer

Could you send tryjobs on the latest version of the patch just to be sure there
aren't any weird compile issues?  Patches that touch this many files tend to be
likely to have odd issues.

[sky, 2011-05-24 22:08:51]

LGTM

[brettw, 2011-05-24 22:53:52]

Can you comment on the higher level of this patch? You've removed the class and
made a set of global functions that do the same thing, which isn't clearly a win
to me. Is this part of a bigger plan?

[Jeff Timanus, 2011-05-24 23:03:45]

On 2011/05/24 22:53:52, brettw wrote:
> Can you comment on the higher level of this patch? You've removed the class
and
> made a set of global functions that do the same thing, which isn't clearly a
win
> to me. Is this part of a bigger plan?

Yes, this is part of a bigger plan.  The new skia-backends (GPU & PDF) are
difficult to integrate into Chrome because of the strong assumption on SkDevices
instances being PlatformDevice (or the various *PlatformDevice classes)
instances.  

The long term goal is to entirely remove BitmapPlatformDevice, and some of the
VectorPlatformDevice classes, to allow skia to subsume all rendering
responsibility directly, without forcing native rendering into a skia surface
via SkDevice overrides.

Alok P. has similar work on the go to remove skia::PlatformCanvas.  See his CL: 
http://codereview.chromium.org/6879013

Jeff

[brettw, 2011-05-24 23:09:29]

It seems you might always want to go to the OS to render directly, for example,
rendering Windows themes for form controls. How will this be supported?

I'm interested in Mike Reed's high level opinion on this. I guess I would have
maybe expected a getter for the PlatformCanvas and then for the functions to be
on that, rather than having a global list of random functions that operate on
these objects. Why did you pick this approach?

[Jeff Timanus, 2011-05-24 23:33:40]

On 2011/05/24 23:09:29, brettw wrote:
> It seems you might always want to go to the OS to render directly, for
example,
> rendering Windows themes for form controls. How will this be supported?

I'm not directly on that task, but I believe the plan is to have the OS blit all
of form controls that will be used, and then store those as skia bitmaps.  Skia
can then render with a native-control feel, without invoking platform-specific
rendering.

> 
> I'm interested in Mike Reed's high level opinion on this. I guess I would have
> maybe expected a getter for the PlatformCanvas and then for the functions to
be
> on that, rather than having a global list of random functions that operate on
> these objects. Why did you pick this approach?

The desire is to integrate the gpu back-ends directly into all aspects of
Chrome.  These back-ends do not implement any of the features of PlatformDevice,
or PlatformCanvas.  This particular approach was chosen because it allows for
PlatformDevice calls to be invoked on SkDevice, and SkCanvas instances.  The
plan is that all of the calls will re-worked to only provide the
Platform{Canvas|Device} functionality, if the type is actually one of those
Platform* classes.  Otherwise the calls will no-op.

I believe the issue with the approach you suggested is that SkDevice, and
SkCanvas do not have accessors for PlatformCanvas instances.  Call-sites that
are presently using the PlatformDevice::* set of calls would have to query for
the PlatformObject, and conditionally invoke the appropriate call.  The global
helper functions will basically follow the same technique, but hide the need to
check for existence of PlatformDevice instances.

Alok & Mike, please chime in, if you feel I have missed any key points.

Jeff

[Avi (use Gerrit), 2011-05-25 02:24:17]

content/browser LGTM; you might still need an owner higher-up.

[alokp, 2011-05-25 04:46:34]

On 2011/05/24 23:09:29, brettw wrote:
> It seems you might always want to go to the OS to render directly, for
example,
> rendering Windows themes for form controls. How will this be supported?
> 
> I'm interested in Mike Reed's high level opinion on this. I guess I would have
> maybe expected a getter for the PlatformCanvas and then for the functions to
be
> on that, rather than having a global list of random functions that operate on
> these objects. Why did you pick this approach?

PlatformCanvas will be deprecated soon. It does not do anything; all the magic
happens in PlatformDevice, which has become problematic too with gpu accelerated
device, SkGpuDevice. 

Chromium assumes that all devices are instances of PlatformDevice, which is not
true when using SkGpuDevice. This ultimate goal is removing this assumption by
making Chromium operate on the lowest common denominator, i.e. SkDevice, and
only ever calls methods that are defined in the SkDevice base class. This patch
is a small step in that direction.

You are absolutely right about rendering native controls. We do not have a good
answer, but it is something a number of people are working on. I have band-aid
CL which just suppresses native controls when doing accelerated drawing of web
content - http://codereview.chromium.org/6914029. The project to accelerate
chrome's chrome needs to solve this problem too.

[Jeff Timanus, 2011-05-25 14:38:37]

On 2011/05/24 21:57:10, jamesr wrote:
> LGTM for content/renderer
> 
> Could you send tryjobs on the latest version of the patch just to be sure
there
> aren't any weird compile issues?  Patches that touch this many files tend to
be
> likely to have odd issues.

Try-jobs reporting success.  The linux failures are from tests that have been
flaky:

ExtensionWebSocketProxyPrivateApiTest.Pass &
ExtensionCrashRecoveryTest.TwoExtensionsShutdownWhileCrashed

[brettw, 2011-05-25 15:49:43]

chrome/browser/tab_contents LGTM, I'll let the Skia folks worry about the
direction of the device stuff (seems there has been planning).

[tony, 2011-05-25 16:03:45]

webkit/tools/test_shell LGTM

[vandebo (ex-Chrome), 2011-05-25 18:42:41]

SK_API changes LGTM