skia::PlatformCanvas is being deprecated. Going forward we will use gfx::Canvas wherever we need ...

skia::PlatformCanvas is being deprecated. Going forward we will use gfx::Canvas wherever we need to render using both skia and native context.
This patch also refactores gfx::Canvas by not deriving from SkCanvas. It rather wraps an SkCanvas. This will allow us to use other types of skia canvas (such as gpu accelerated) on the fly.

[alokp, 2011-04-19 21:51:01]

Scott:

I am trying to deprecate skia::PlatformCanvas in favor of gfx::Canvas. But
before I do that I would like to refactor gfx::CanvasSkia and
gfx::CanvasSkiaPaint to not derive from SkCanvas.

Deriving from SkCanvas does not buy anything as we are not overloading any
virtual functions. It however prevents us from using gpu-accelerated
implementation of SkCanvas.

The main thing to be reviewed in this CL is gfx::CanvasSkia. The implementation
for gfx::CanvasSkiaPaint has been mostly copied from skia::PlatformCanvasPaint,
which will be deprecated as well. Rest of the changes are just to fix callers of
the new API. I still need to fix a few callers on linux_views, chromeos, and
mac.

Could you please take a look at gfx::CanvasSkia API change so that I do not have
to fix the callers twice?

Thanks,
-Alok

[sky, 2011-04-19 22:18:57]

The other day Ben mentioned we might want to nuke Canvas entirely and go back to
subclassing SkCanvas directly. So, I'm pushing this to review to him.

Note, the following chain makes me ill:

  canvas->AsCanvasSkia()->skia_canvas()->DoStuff

  -Scott

[Ben Goodger (Google), 2011-04-19 22:24:37]

Yes please can we not add that?

[alokp, 2011-04-19 22:28:56]

On 2011/04/19 22:18:57, sky wrote:
> The other day Ben mentioned we might want to nuke Canvas entirely and go back
to
> subclassing SkCanvas directly. So, I'm pushing this to review to him.

I am not a big fan of subclassing when we do not need to. I would feel much
better if we refactored Canvas into a set of standalone helper functions and
used SkCanvas directly.

> Note, the following chain makes me ill:
> 
>   canvas->AsCanvasSkia()->skia_canvas()->DoStuff
> 

Me too! This is happening because gfx::Canvas API is incomplete. May be we
should just give up trying to wrap SkCanvas and use it directly. It is a nice
cross-platform interface. Not it also supports gpu acceleration. The gfx::Canvas
would either be something like this:

class Canvas {
  Canvas();
  ~Canvas();

  bool Init(width, height, opaque);

  // Get skia context.
  SkCanvas* skia_canvas();
  // Get native context.
  NativeContext* BeginNativePaint();
  EndNativePaint();
};

So now Canvas is basically a bitmap shared two contexts - skia and native. At
this point I am not sure if we can support native context which gpu accelerated
skia context. Mike: Can you comment?

[alokp, 2011-04-21 16:30:06]

> Note, the following chain makes me ill:
> 
>   canvas->AsCanvasSkia()->skia_canvas()->DoStuff
> 

Mike suggested adding AsSkiaCanvas() to gfx::Canvas to get rid of an extra link
in the chain. It is confusing to have both - AsCanvasSkia() and AsSkiaCanvas()
but it is temporary. If you are planning to get rid of Canvas/CanvasSkia, it
will go away.

[Ben Goodger (Google), 2011-04-21 16:31:57]

Since you guys are adding hardware accelerated 2D rendering to skia, I no
longer think the gfx::Canvas interface is necessary... I added it so I could
experiment with a Direct2D backend which I never completed as Skia usage is
entrenched in Chrome code.

With that in mind, gfx::CanvasSkia could be renamed gfx::Canvas, the old
interface removed along with gfx::CanvasDirect2D. I'd prefer to do that than
introduce all this extra indirection throughout the code.

-Ben

On Thu, Apr 21, 2011 at 9:30 AM, <alokp@chromium.org> wrote:

>
>  Note, the following chain makes me ill:
>>
>
>    canvas->AsCanvasSkia()->skia_canvas()->DoStuff
>>
>
>
> Mike suggested adding AsSkiaCanvas() to gfx::Canvas to get rid of an extra
> link
> in the chain. It is confusing to have both - AsCanvasSkia() and
> AsSkiaCanvas()
> but it is temporary. If you are planning to get rid of Canvas/CanvasSkia,
> it
> will go away.
>
>
>
>
> http://codereview.chromium.org/6879013/
>

[reed1, 2011-04-21 16:47:47]

Totally agree.

To support displaylists as well (and future extensions) we should also
retool the helper methods (e.g. FillRectInt) to just be static
functions. That way there need be zero indirection. e.g.

canvas->FillRectInt(color, x, y, w, h)

  becomes

FillRectInt(canvas, color, x, y, w, h)

I imagine, if structure this way, Skia itself could later roll some of
these into its corpus, if they are seen to be useful convenience
functions.

mike

On Thu, Apr 21, 2011 at 12:31 PM, Ben Goodger (Google) <ben@chromium.org> wrote:
> Since you guys are adding hardware accelerated 2D rendering to skia, I no
> longer think the gfx::Canvas interface is necessary... I added it so I could
> experiment with a Direct2D backend which I never completed as Skia usage is
> entrenched in Chrome code.
> With that in mind, gfx::CanvasSkia could be renamed gfx::Canvas, the old
> interface removed along with gfx::CanvasDirect2D. I'd prefer to do that than
> introduce all this extra indirection throughout the code.
> -Ben
>
> On Thu, Apr 21, 2011 at 9:30 AM, <alokp@chromium.org> wrote:
>>
>>> Note, the following chain makes me ill:
>>
>>>   canvas->AsCanvasSkia()->skia_canvas()->DoStuff
>>
>>
>> Mike suggested adding AsSkiaCanvas() to gfx::Canvas to get rid of an extra
>> link
>> in the chain. It is confusing to have both - AsCanvasSkia() and
>> AsSkiaCanvas()
>> but it is temporary. If you are planning to get rid of Canvas/CanvasSkia,
>> it
>> will go away.
>>
>>
>>
>> http://codereview.chromium.org/6879013/
>
>

[Jeff Timanus, 2011-05-04 22:22:29]

How soon is it planned for this change to land?

Jeff

On 2011/04/21 16:47:47, reed1 wrote:
> Totally agree.
> 
> To support displaylists as well (and future extensions) we should also
> retool the helper methods (e.g. FillRectInt) to just be static
> functions. That way there need be zero indirection. e.g.
> 
> canvas->FillRectInt(color, x, y, w, h)
> 
>   becomes
> 
> FillRectInt(canvas, color, x, y, w, h)
> 
> I imagine, if structure this way, Skia itself could later roll some of
> these into its corpus, if they are seen to be useful convenience
> functions.
> 
> mike
> 
> On Thu, Apr 21, 2011 at 12:31 PM, Ben Goodger (Google)
<mailto:ben@chromium.org> wrote:
> > Since you guys are adding hardware accelerated 2D rendering to skia, I no
> > longer think the gfx::Canvas interface is necessary... I added it so I could
> > experiment with a Direct2D backend which I never completed as Skia usage is
> > entrenched in Chrome code.
> > With that in mind, gfx::CanvasSkia could be renamed gfx::Canvas, the old
> > interface removed along with gfx::CanvasDirect2D. I'd prefer to do that than
> > introduce all this extra indirection throughout the code.
> > -Ben
> >
> > On Thu, Apr 21, 2011 at 9:30 AM, <mailto:alokp@chromium.org> wrote:
> >>
> >>> Note, the following chain makes me ill:
> >>
> >>> &nbsp; canvas->AsCanvasSkia()->skia_canvas()->DoStuff
> >>
> >>
> >> Mike suggested adding AsSkiaCanvas() to gfx::Canvas to get rid of an extra
> >> link
> >> in the chain. It is confusing to have both - AsCanvasSkia() and
> >> AsSkiaCanvas()
> >> but it is temporary. If you are planning to get rid of Canvas/CanvasSkia,
> >> it
> >> will go away.
> >>
> >>
> >>
> >> http://codereview.chromium.org/6879013/
> >
> >