Move SkPaint mangement for 2D canvas into CanvasRenderingContext2DState

Move SkPaint mangement for 2D canvas into CanvasRenderingContext2DState

With this change CanvasRenderingContext2DState now has the ability to
generate the SkPaint objects required draw canvas primitives directly
to an SkCanvas, without using blink::GraphicsContext. This change
migrates the fillRect and strokeRect methods to use the new code path
thus making them completely independent of GraphicsContext

BUG=453113
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192449

[Justin Novosad, 2015-03-18 21:32:17]

junov@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Justin Novosad, 2015-03-18 21:32:18]

PTAL

[Stephen White, 2015-03-18 22:09:35]

Looks like a great start. I like the direction here.

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1193: //
FIXME(crbug.com/425531): Funtional.h cannot handle override function signature.
Is this comment still relevant?

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
File Source/core/html/canvas/CanvasRenderingContext2DState.cpp (right):

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
Source/core/html/canvas/CanvasRenderingContext2DState.cpp:308: // These
constants are from RadiusToSigma() from DrawLooperBuilder.cpp.
Nit: could you expose that function (or move it to SkiaUtils) instead of copying
it?

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
File Source/core/html/canvas/CanvasRenderingContext2DState.h (right):

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
Source/core/html/canvas/CanvasRenderingContext2DState.h:124: const SkPaint*
skPaint(CanvasPaintType, ShadowMode, CanvasShadowStrategy) const;
Since this is more than just an accessor, maybe it should be getPaint().

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
Source/core/html/canvas/CanvasRenderingContext2DState.h:165: unsigned
m_textAlign : 3;
Could we hold off on using bitfields here until we have a real-world test case
that proves then necessary? That would obviate the need for all those
static_casts above.

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
File Source/core/html/canvas/CanvasStyle.cpp (right):

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
Source/core/html/canvas/CanvasStyle.cpp:168: return SK_ColorBLACK;
Nit: this is an RGBA32 (Blink type) but you're returning a Skia constant
(SK_ColorBLACK).

[Justin Novosad, 2015-03-19 14:32:21]

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1193: //
FIXME(crbug.com/425531): Funtional.h cannot handle override function signature.
On 2015/03/18 22:09:34, Stephen White wrote:
> Is this comment still relevant?

Yes, unfortunately. because drawRect is virtual, we need this little wrapper
func.

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
File Source/core/html/canvas/CanvasStyle.cpp (right):

https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
Source/core/html/canvas/CanvasStyle.cpp:168: return SK_ColorBLACK;
On 2015/03/18 22:09:35, Stephen White wrote:
> Nit: this is an RGBA32 (Blink type) but you're returning a Skia constant
> (SK_ColorBLACK).

Yeah... RGBA32 and SkColor are completely interchangeable.  The only reason I
can think of for RGBA32 to exist is to prevent blink::Color from depending on
SkColor, but perhaps that is futile? I think we should consider removing RGBA32
from the code entirely and just use SkColor. Anyways... for this CL, I will use
Color::black here.

[Stephen White, 2015-03-19 14:34:37]

On 2015/03/19 14:32:21, junov wrote:
>
https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1193: //
> FIXME(crbug.com/425531): Funtional.h cannot handle override function
signature.
> On 2015/03/18 22:09:34, Stephen White wrote:
> > Is this comment still relevant?
> 
> Yes, unfortunately. because drawRect is virtual, we need this little wrapper
> func.
> 
>
https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
> File Source/core/html/canvas/CanvasStyle.cpp (right):
> 
>
https://codereview.chromium.org/1008173003/diff/1/Source/core/html/canvas/Can...
> Source/core/html/canvas/CanvasStyle.cpp:168: return SK_ColorBLACK;
> On 2015/03/18 22:09:35, Stephen White wrote:
> > Nit: this is an RGBA32 (Blink type) but you're returning a Skia constant
> > (SK_ColorBLACK).
> 
> Yeah... RGBA32 and SkColor are completely interchangeable.  The only reason I
> can think of for RGBA32 to exist is to prevent blink::Color from depending on
> SkColor, but perhaps that is futile? I think we should consider removing
RGBA32
> from the code entirely and just use SkColor.

That's a good idea. RGBA32 made sense in the pre-fork world, where WebKit needed
a cross-platform 32-bit color, but it doesn't really make sense anymore.

> Anyways... for this CL, I will use
> Color::black here.

SGTM

[Justin Novosad, 2015-03-19 19:42:09]

new patch

[Justin Novosad, 2015-03-19 19:45:30]

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1193: static void
drawRectOnCanvas(const FloatRect& rect, SkCanvas* canvas, const SkPaint* paint)
This function is significantly different from 1st patch.  Added the SkPath code
path to mimic GC behavior that is necessary for spec compliance.

[Stephen White, 2015-03-19 20:35:17]

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1195: if (paint->getStyle()
== SkPaint::kStroke_Style && ((rect.width() > 0) ^ (rect.height() > 0))) {
Suggest (A != B) instead of xor'ing bools.

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1502: void
CanvasRenderingContext2D::fullCanvasCompositedDraw(PassOwnPtr<Function<void(SkCanvas*,
const SkPaint*)>> draw, CanvasPaintType paintType, CanvasShadowStrategy
shadowStrategy)
It looks like the shadowStrategy is always DrawLooperShadowStrategy at the call
sites. Are you going to need this later, or could you remove it?

(Actually, it looks like ImageFilterShadowStrategy isn't used/tested at all, but
I presume that'll change in a later patch.)

Thought: could we use a functor approach to hide the shadow passes and strategy
inside CanvasRenderingContext2DState itself? (Just an idea; I haven't really
thought this through.) You could express the requirements a bit more clearly
that way, as in "here's my primitive draw function" and a flag for "my primitive
might have alpha", and the state would be responsible for choosing the correct
strategy and number of passes. It would make the state more active than passive,
though.

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.h:263: void
fullCanvasCompositedDraw(PassOwnPtr<Closure> draw); // deprecated
Could you log a bug for updating/removing this deprecated flavour, so we don't
forget?

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2DState.cpp (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2DState.cpp:310:
m_shadowOnlyImageFilter =
adoptRef(SkDropShadowImageFilter::Create(m_shadowOffset.width(),
m_shadowOffset.height(), sigma, sigma, m_shadowColor,
SkDropShadowImageFilter::kDrawShadowOnly_ShadowMode));
Suggestion: refactor the Create() call into a helper function that takes a mode
and gets the rest from member vars.

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2DState.h (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2DState.h:77: Direction
direction() const { return static_cast<Direction>(m_direction); }
We shouldn't need these static_casts anymore?

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
File Source/platform/graphics/DrawLooperBuilder.h (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
Source/platform/graphics/DrawLooperBuilder.h:35: #include
"platform/graphics/skia/SkiaUtils.h"
Is there a reason this has to be in the header, or could it be in the .cpp
instead?

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
Source/platform/graphics/DrawLooperBuilder.h:35: #include
"platform/graphics/skia/SkiaUtils.h"
Could this be in the .cpp instead?

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
File Source/platform/graphics/GraphicsTypes.h (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
Source/platform/graphics/GraphicsTypes.h:79: enum CanvasPaintType {
Could this (and the CanvasShadowStrategy below) be in
CanvasRenderingContext2DState?

[Justin Novosad, 2015-03-19 21:03:36]

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1195: if (paint->getStyle()
== SkPaint::kStroke_Style && ((rect.width() > 0) ^ (rect.height() > 0))) {
On 2015/03/19 20:35:17, Stephen White wrote:
> Suggest (A != B) instead of xor'ing bools.

Acknowledged.

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1502: void
CanvasRenderingContext2D::fullCanvasCompositedDraw(PassOwnPtr<Function<void(SkCanvas*,
const SkPaint*)>> draw, CanvasPaintType paintType, CanvasShadowStrategy
shadowStrategy)
On 2015/03/19 20:35:16, Stephen White wrote:
> It looks like the shadowStrategy is always DrawLooperShadowStrategy at the
call
> sites. Are you going to need this later, or could you remove it?
> 
> (Actually, it looks like ImageFilterShadowStrategy isn't used/tested at all,
but
> I presume that'll change in a later patch.)
> 
It will be needed for drawImage


> Thought: could we use a functor approach to hide the shadow passes and
strategy
> inside CanvasRenderingContext2DState itself? (Just an idea; I haven't really
> thought this through.) You could express the requirements a bit more clearly
> that way, as in "here's my primitive draw function" and a flag for "my
primitive
> might have alpha", and the state would be responsible for choosing the correct
> strategy and number of passes. It would make the state more active than
passive,
> though.

I don't see how a functor would make any of this clearer, but I could rename
some of the enums or mash them into a bitfield.

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.h:263: void
fullCanvasCompositedDraw(PassOwnPtr<Closure> draw); // deprecated
On 2015/03/19 20:35:17, Stephen White wrote:
> Could you log a bug for updating/removing this deprecated flavour, so we don't
> forget?

I already have to remove this function as a part of the refactor (it calls GC).

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2DState.cpp (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2DState.cpp:310:
m_shadowOnlyImageFilter =
adoptRef(SkDropShadowImageFilter::Create(m_shadowOffset.width(),
m_shadowOffset.height(), sigma, sigma, m_shadowColor,
SkDropShadowImageFilter::kDrawShadowOnly_ShadowMode));
On 2015/03/19 20:35:17, Stephen White wrote:
> Suggestion: refactor the Create() call into a helper function that takes a
mode
> and gets the rest from member vars.

Tried it out and backtracked because it adds a dependency on
SkDropShadowImageFilter.h in CanvasRenderingContext2DState.h (for the ShadowMode
enum). The helper has to be a member function to access the member vars.

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2DState.h (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2DState.h:77: Direction
direction() const { return static_cast<Direction>(m_direction); }
On 2015/03/19 20:35:17, Stephen White wrote:
> We shouldn't need these static_casts anymore?

Acknowledged.

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
File Source/platform/graphics/DrawLooperBuilder.h (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
Source/platform/graphics/DrawLooperBuilder.h:35: #include
"platform/graphics/skia/SkiaUtils.h"
On 2015/03/19 20:35:17, Stephen White wrote:
> Is there a reason this has to be in the header, or could it be in the .cpp
> instead?

Acknowledged.

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
File Source/platform/graphics/GraphicsTypes.h (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
Source/platform/graphics/GraphicsTypes.h:79: enum CanvasPaintType {
On 2015/03/19 20:35:17, Stephen White wrote:
> Could this (and the CanvasShadowStrategy below) be in
> CanvasRenderingContext2DState?

They could...

These enums are used in the declaration of
CanvasRenderingContext2D::fullCanvasCompositedDraw
If I move them to CanvasRenderingContext2DState.h, that would add a new include
dependency. Currently, CanvasRenderingContext2DState only needs to be forward
declared in CanvasRenderingContext2D.h

WDYT?

[Justin Novosad, 2015-03-19 21:13:31]

new patch

[Stephen White, 2015-03-19 22:37:26]

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
File Source/platform/graphics/GraphicsTypes.h (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
Source/platform/graphics/GraphicsTypes.h:79: enum CanvasPaintType {
On 2015/03/19 21:03:36, junov wrote:
> On 2015/03/19 20:35:17, Stephen White wrote:
> > Could this (and the CanvasShadowStrategy below) be in
> > CanvasRenderingContext2DState?
> 
> They could...
> 
> These enums are used in the declaration of
> CanvasRenderingContext2D::fullCanvasCompositedDraw
> If I move them to CanvasRenderingContext2DState.h, that would add a new
include
> dependency. Currently, CanvasRenderingContext2DState only needs to be forward
> declared in CanvasRenderingContext2D.h
> 
> WDYT?

I don't mind the dependency. I think keeping canvas-specific stuff localized to
canvas headers is a nice property.

[Stephen White, 2015-03-19 22:47:54]

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1502: void
CanvasRenderingContext2D::fullCanvasCompositedDraw(PassOwnPtr<Function<void(SkCanvas*,
const SkPaint*)>> draw, CanvasPaintType paintType, CanvasShadowStrategy
shadowStrategy)
On 2015/03/19 21:03:36, junov wrote:
> On 2015/03/19 20:35:16, Stephen White wrote:
> > It looks like the shadowStrategy is always DrawLooperShadowStrategy at the
> call
> > sites. Are you going to need this later, or could you remove it?
> > 
> > (Actually, it looks like ImageFilterShadowStrategy isn't used/tested at all,
> but
> > I presume that'll change in a later patch.)
> > 
> It will be needed for drawImage
> 
> 
> > Thought: could we use a functor approach to hide the shadow passes and
> strategy
> > inside CanvasRenderingContext2DState itself? (Just an idea; I haven't really
> > thought this through.) You could express the requirements a bit more clearly
> > that way, as in "here's my primitive draw function" and a flag for "my
> primitive
> > might have alpha", and the state would be responsible for choosing the
correct
> > strategy and number of passes. It would make the state more active than
> passive,
> > though.
> 
> I don't see how a functor would make any of this clearer, but I could rename
> some of the enums or mash them into a bitfield.
> 

The idea was to hide the looper/imagefilter distinction, which is a Skia
implementation detail.

Actually maybe we could just rename the enums to PerPixelAlpha and
NoPerPixelApha, or something like that, to reflect the intent rather than the
implementation.

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2DState.cpp (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2DState.cpp:310:
m_shadowOnlyImageFilter =
adoptRef(SkDropShadowImageFilter::Create(m_shadowOffset.width(),
m_shadowOffset.height(), sigma, sigma, m_shadowColor,
SkDropShadowImageFilter::kDrawShadowOnly_ShadowMode));
On 2015/03/19 21:03:36, junov wrote:
> On 2015/03/19 20:35:17, Stephen White wrote:
> > Suggestion: refactor the Create() call into a helper function that takes a
> mode
> > and gets the rest from member vars.
> 
> Tried it out and backtracked because it adds a dependency on
> SkDropShadowImageFilter.h in CanvasRenderingContext2DState.h (for the
ShadowMode
> enum). The helper has to be a member function to access the member vars.

Acknowledged.

[Justin Novosad, 2015-03-23 18:19:41]

On 2015/03/19 22:37:26, Stephen White wrote:
>
https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
> File Source/platform/graphics/GraphicsTypes.h (right):
> 
>
https://codereview.chromium.org/1008173003/diff/40001/Source/platform/graphic...
> Source/platform/graphics/GraphicsTypes.h:79: enum CanvasPaintType {
> On 2015/03/19 21:03:36, junov wrote:
> > On 2015/03/19 20:35:17, Stephen White wrote:
> > > Could this (and the CanvasShadowStrategy below) be in
> > > CanvasRenderingContext2DState?
> > 
> > They could...
> > 
> > These enums are used in the declaration of
> > CanvasRenderingContext2D::fullCanvasCompositedDraw
> > If I move them to CanvasRenderingContext2DState.h, that would add a new
> include
> > dependency. Currently, CanvasRenderingContext2DState only needs to be
forward
> > declared in CanvasRenderingContext2D.h
> > 
> > WDYT?
> 
> I don't mind the dependency. I think keeping canvas-specific stuff localized
to
> canvas headers is a nice property.

Done.

[Justin Novosad, 2015-03-23 18:20:58]

On 2015/03/19 22:47:54, Stephen White wrote:
> 
> The idea was to hide the looper/imagefilter distinction, which is a Skia
> implementation detail.
> 
> Actually maybe we could just rename the enums to PerPixelAlpha and
> NoPerPixelApha, or something like that, to reflect the intent rather than the
> implementation.
>

Done.  I used the OpacityMode enum which I relocated to GraphicsTypes since it
is now used in multiple unrelated interfaces.

[Justin Novosad, 2015-03-24 15:48:14]

Ping.

[Stephen White, 2015-03-24 15:52:06]

LGTM

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/1008173003/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.h:263: void
fullCanvasCompositedDraw(PassOwnPtr<Closure> draw); // deprecated
On 2015/03/19 21:03:36, junov wrote:
> On 2015/03/19 20:35:17, Stephen White wrote:
> > Could you log a bug for updating/removing this deprecated flavour, so we
don't
> > forget?
> 
> I already have to remove this function as a part of the refactor (it calls
GC). 

Acknowledged.

[Justin Novosad, 2015-03-24 15:59:32]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2015-03-24 16:00:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1008173003/80001

[commit-bot: I haz the power, 2015-03-24 17:12:41]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192449