ui/gfx: Make gfx::Canvas inherit from gfx::CanvasSkia.

ui/gfx: Make gfx::Canvas inherit from gfx::CanvasSkia.

The final goal is to merge these two classes into a single gfx::Canvas class.

BUG=116572
R=ben@chromium.org,asvitkine@chromium.org
TBR=ben@chromium.org

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

[tfarina, 2012-03-01 23:27:53]

Does this make sense?

[Peter Kasting, 2012-03-01 23:30:01]

The Canvas virtual methods that you're now falling back on seem to be marked
with a "TODO get rid of these quick hacks" sort of comment from Ben.  So I would
ask him what the plans there are, because if those comments are still in force,
this probably is not what we want.

[tfarina, 2012-03-01 23:33:11]

On 2012/03/01 23:30:01, Peter Kasting wrote:
> The Canvas virtual methods that you're now falling back on seem to be marked
> with a "TODO get rid of these quick hacks" sort of comment from Ben.  So I
would
> ask him what the plans there are, because if those comments are still in
force,
> this probably is not what we want.

Ben, could you comment on this?

[Ben Goodger (Google), 2012-03-01 23:56:17]

I didn't add this... can you ask asvitkine?

[tfarina, 2012-03-02 00:02:33]

On 2012/03/01 23:56:17, Ben Goodger (Google) wrote:
> I didn't add this... can you ask asvitkine?

Antonie added sk_canvas here http://codereview.chromium.org/8122013.

The ben's TODO is in ui/gfx/canvas.h +217

[Peter Kasting, 2012-03-02 00:06:49]

OK, Ben and I chatted and got things straightened out.

The story is, canvas.h was basically added as part of the experiment to add a
Direct2D-based canvas.  However, that experiment is dead.  So rather than make
callers rely on virtual functions in canvas.h, we should just kill canvas.h
entirely, and rename canvas_skia back to canvas (both in filenames and
classnames), and eliminate or make non-virtual all the virtual functions and
stuff.

Make sense?

[tfarina, 2012-03-02 02:12:08]

On 2012/03/02 00:06:49, Peter Kasting wrote:
> OK, Ben and I chatted and got things straightened out.
> 
> The story is, canvas.h was basically added as part of the experiment to add a
> Direct2D-based canvas.  However, that experiment is dead.  So rather than make
> callers rely on virtual functions in canvas.h, we should just kill canvas.h
> entirely, and rename canvas_skia back to canvas (both in filenames and
> classnames), and eliminate or make non-virtual all the virtual functions and
> stuff.
> 
> Make sense?

Peter, this is ready for another look. Although I need to figure out why
canvas_linux and canvas_skia are being both compiled, it seems it should be one
or other, otherwise I get multiple definitions for SizeStringInt, DrawStringInt,
DrawStringWithHalo. This became a giant patch :(

Please, see if this is in the right direction.

[Peter Kasting, 2012-03-02 02:16:03]

Please update the change title & description.

I'd prefer if Ben could review this as he was the one who originally forked off
canvas.h.

[Ben Goodger (Google), 2012-03-02 17:02:13]

This CL would be easier to review if:

you deleted the contents of canvas.h and replaced it with:

#include "ui/gfx/canvas_skia.h"

namespace gfx {

typedef CanvasSkia Canvas;

}

instead of trying to solve the universe's problems in a single CL :-)

[tfarina, 2012-03-02 19:35:05]

On 2012/03/02 17:02:13, Ben Goodger (Google) wrote:
> This CL would be easier to review if:
> 
> you deleted the contents of canvas.h and replaced it with:
> 
> #include "ui/gfx/canvas_skia.h"
> 
> namespace gfx {
> 
> typedef CanvasSkia Canvas;
> 
> }
> 
> instead of trying to solve the universe's problems in a single CL :-)

Done. PTAL! the main changes are in canvas.*, canvas_skia.*. The others are just
fixing the compile errors, renaming from gfx::Canvas to gfx::CanvasSkia where
needed.

[Ben Goodger (Google), 2012-03-02 20:35:10]

http://codereview.chromium.org/9562038/diff/9259/ash/app_list/app_list_item_v...
File ash/app_list/app_list_item_view.h (right):

http://codereview.chromium.org/9562038/diff/9259/ash/app_list/app_list_item_v...
ash/app_list/app_list_item_view.h:50: virtual void OnPaint(gfx::CanvasSkia*
canvas) OVERRIDE;
This is the change I wanted you to avoid.

Since you have the typedef, this should not be necessary

[tfarina, 2012-03-02 20:57:20]

http://codereview.chromium.org/9562038/diff/9259/ash/app_list/app_list_item_v...
File ash/app_list/app_list_item_view.h (right):

http://codereview.chromium.org/9562038/diff/9259/ash/app_list/app_list_item_v...
ash/app_list/app_list_item_view.h:50: virtual void OnPaint(gfx::CanvasSkia*
canvas) OVERRIDE;
On 2012/03/02 20:35:11, Ben Goodger (Google) wrote:
> This is the change I wanted you to avoid.
> 
> Since you have the typedef, this should not be necessary

The forward declaration "class Canvas;" conflicts with the typedef declaration,
hence this is necessary.

[tfarina, 2012-03-02 21:19:10]

On 2012/03/02 20:57:20, tfarina wrote:
>
http://codereview.chromium.org/9562038/diff/9259/ash/app_list/app_list_item_v...
> File ash/app_list/app_list_item_view.h (right):
> 
>
http://codereview.chromium.org/9562038/diff/9259/ash/app_list/app_list_item_v...
> ash/app_list/app_list_item_view.h:50: virtual void OnPaint(gfx::CanvasSkia*
> canvas) OVERRIDE;
> On 2012/03/02 20:35:11, Ben Goodger (Google) wrote:
> > This is the change I wanted you to avoid.
> > 
> > Since you have the typedef, this should not be necessary
> 
> The forward declaration "class Canvas;" conflicts with the typedef
declaration,
> hence this is necessary.

http://pastebin.com/KLTFeetj

[tfarina, 2012-03-05 20:33:18]

On 2012/03/02 21:19:10, tfarina wrote:
> On 2012/03/02 20:57:20, tfarina wrote:
> >
>
http://codereview.chromium.org/9562038/diff/9259/ash/app_list/app_list_item_v...
> > File ash/app_list/app_list_item_view.h (right):
> > 
> >
>
http://codereview.chromium.org/9562038/diff/9259/ash/app_list/app_list_item_v...
> > ash/app_list/app_list_item_view.h:50: virtual void OnPaint(gfx::CanvasSkia*
> > canvas) OVERRIDE;
> > On 2012/03/02 20:35:11, Ben Goodger (Google) wrote:
> > > This is the change I wanted you to avoid.
> > > 
> > > Since you have the typedef, this should not be necessary
> > 
> > The forward declaration "class Canvas;" conflicts with the typedef
> declaration,
> > hence this is necessary.
> 
> http://pastebin.com/KLTFeetj

Ben, do we have another alternative? If not, I think I'll close this issue and
try with smaller patches?

[asvitkine_google, 2012-03-05 20:54:43]

On 2012/03/05 20:33:18, tfarina wrote:
> On 2012/03/02 21:19:10, tfarina wrote:
> > On 2012/03/02 20:57:20, tfarina wrote:
> > >
> >
>
http://codereview.chromium.org/9562038/diff/9259/ash/app_list/app_list_item_v...
> > > File ash/app_list/app_list_item_view.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9562038/diff/9259/ash/app_list/app_list_item_v...
> > > ash/app_list/app_list_item_view.h:50: virtual void
OnPaint(gfx::CanvasSkia*
> > > canvas) OVERRIDE;
> > > On 2012/03/02 20:35:11, Ben Goodger (Google) wrote:
> > > > This is the change I wanted you to avoid.
> > > > 
> > > > Since you have the typedef, this should not be necessary
> > > 
> > > The forward declaration "class Canvas;" conflicts with the typedef
> > declaration,
> > > hence this is necessary.
> > 
> > http://pastebin.com/KLTFeetj
> 
> Ben, do we have another alternative? If not, I think I'll close this issue and
> try with smaller patches?

You can start off by just have gfx::Canvas extend CanvasSkia with no
implementation. And then afterwards just rename CanvasSkia to Canvas (and rename
the files to remove the "_skia" suffixes).

[tfarina, 2012-03-06 02:19:29]

On 2012/03/05 20:54:43, asvitkine wrote:
> > Ben, do we have another alternative? If not, I think I'll close this issue
and
> > try with smaller patches?
> 
> You can start off by just have gfx::Canvas extend CanvasSkia with no
> implementation. And then afterwards just rename CanvasSkia to Canvas (and
rename
> the files to remove the "_skia" suffixes).

Thanks Alexei! Please, take another look!

If the approach looks reasonable, I'll clean up the style issues I left in
canvas_skia.h when we agree this is the right way to go.

[Ben Goodger (Google), 2012-03-06 15:46:57]

http://codereview.chromium.org/9562038/diff/16001/ui/base/dragdrop/drag_utils.cc
File ui/base/dragdrop/drag_utils.cc (right):

http://codereview.chromium.org/9562038/diff/16001/ui/base/dragdrop/drag_utils...
ui/base/dragdrop/drag_utils.cc:65: void SetDragImageOnDataObject(const
gfx::CanvasSkia& canvas,
????

http://codereview.chromium.org/9562038/diff/16001/ui/base/dragdrop/drag_utils.h
File ui/base/dragdrop/drag_utils.h (right):

http://codereview.chromium.org/9562038/diff/16001/ui/base/dragdrop/drag_utils...
ui/base/dragdrop/drag_utils.h:18: class CanvasSkia;
????

http://codereview.chromium.org/9562038/diff/16001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/9562038/diff/16001/ui/gfx/canvas.h#newcode17
ui/gfx/canvas.h:17: enum {
move all this down to CanvasSkia too

[tfarina, 2012-03-06 17:09:09]

https://chromiumcodereview.appspot.com/9562038/diff/16001/ui/base/dragdrop/dr...
File ui/base/dragdrop/drag_utils.cc (right):

https://chromiumcodereview.appspot.com/9562038/diff/16001/ui/base/dragdrop/dr...
ui/base/dragdrop/drag_utils.cc:65: void SetDragImageOnDataObject(const
gfx::CanvasSkia& canvas,
On 2012/03/06 15:46:58, Ben Goodger (Google) wrote:
> ????

Done.

https://chromiumcodereview.appspot.com/9562038/diff/16001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://chromiumcodereview.appspot.com/9562038/diff/16001/ui/gfx/canvas.h#new...
ui/gfx/canvas.h:17: enum {
On 2012/03/06 15:46:58, Ben Goodger (Google) wrote:
> move all this down to CanvasSkia too

Done.

[Alexei Svitkine (slow), 2012-03-06 17:44:50]

Looks OK to me other than style issues in ui/gfx/canvas_skia.h that you've
promised to fix.

https://chromiumcodereview.appspot.com/9562038/diff/21001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://chromiumcodereview.appspot.com/9562038/diff/21001/ui/gfx/canvas.h#new...
ui/gfx/canvas.h:14: class UI_EXPORT Canvas : public CanvasSkia {
Please add a comment explaining that this is temporary and that the goal is to
rename CanvasSkia to Canvas.

https://chromiumcodereview.appspot.com/9562038/diff/21001/ui/gfx/canvas_skia.h
File ui/gfx/canvas_skia.h (right):

https://chromiumcodereview.appspot.com/9562038/diff/21001/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.h:162: void Restore() ;
You have a space between ")" and ";" here and in many places below. Please fix.

https://chromiumcodereview.appspot.com/9562038/diff/21001/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.h:283: SkCanvas* GetSkCanvas() ;
Can you move these next to sk_canvas() below and add a TODO to remove one of
them.

[tfarina, 2012-03-06 18:05:29]

https://chromiumcodereview.appspot.com/9562038/diff/21001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://chromiumcodereview.appspot.com/9562038/diff/21001/ui/gfx/canvas.h#new...
ui/gfx/canvas.h:14: class UI_EXPORT Canvas : public CanvasSkia {
On 2012/03/06 17:44:51, Alexei Svitkine wrote:
> Please add a comment explaining that this is temporary and that the goal is to
> rename CanvasSkia to Canvas.

Done.

https://chromiumcodereview.appspot.com/9562038/diff/21001/ui/gfx/canvas_skia.h
File ui/gfx/canvas_skia.h (right):

https://chromiumcodereview.appspot.com/9562038/diff/21001/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.h:162: void Restore() ;
On 2012/03/06 17:44:51, Alexei Svitkine wrote:
> You have a space between ")" and ";" here and in many places below. Please
fix.

Done.

https://chromiumcodereview.appspot.com/9562038/diff/21001/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.h:283: SkCanvas* GetSkCanvas() ;
On 2012/03/06 17:44:51, Alexei Svitkine wrote:
> Can you move these next to sk_canvas() below and add a TODO to remove one of
> them.

Done.

[Alexei Svitkine (slow), 2012-03-06 18:09:21]

LGTM

[Alexei Svitkine (slow), 2012-03-09 00:07:15]

Please also mention in the CL description that the goal is to merge these two
classes into a single gfx::Canvas class.

[tfarina, 2012-03-09 00:09:35]

On 2012/03/09 00:07:15, Alexei Svitkine wrote:
> Please also mention in the CL description that the goal is to merge these two
> classes into a single gfx::Canvas class.

Done.