Simplify Gradient by holding a Color instead of four floats.

Simplify Gradient by holding a Color instead of four floats.

This makes it easier to read, and also prevents pointless conversions
back and forth between a 32-bit integer and four floats.

This resolves a FIXME which asked why this was the case.

BUG=

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

[jbroman, 2014-03-17 18:38:07]

This is a minor simplification, but it was a little silly that we took a 32-bit
integer, split it into four floats, and then reassembled them again in the same
32-bit form to pass to Skia.

[f(malita), 2014-03-17 18:50:17]

lgtm + nit.

https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/Gradient.cpp (right):

https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/Gradient.cpp:181: colors[0] = stop->color.rgb();
We should use SkColorSetARGB() to avoid any packing assumptions.

https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/Gradient.cpp:187: colors[i] = stop->color.rgb();
Ditto.

[jbroman, 2014-03-17 18:53:06]

On 2014/03/17 18:50:17, Florin Malita wrote:
> lgtm + nit.
> 
>
https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
> File Source/platform/graphics/Gradient.cpp (right):
> 
>
https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
> Source/platform/graphics/Gradient.cpp:181: colors[0] = stop->color.rgb();
> We should use SkColorSetARGB() to avoid any packing assumptions.
> 
>
https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
> Source/platform/graphics/Gradient.cpp:187: colors[i] = stop->color.rgb();
> Ditto.

This is the idiom we use everywhere to convert WebCore::Color to SkColor (e.g.
look at any call to SkPaint::setColor in GraphicsContext or
GraphicsContextState).

If we want to change that, wouldn't it be better to give WebCore::Color a method
to convert to SkColor or something like that (bearing in mind it would more or
less be the same as rgb)?

[f(malita), 2014-03-17 19:01:00]

On 2014/03/17 18:53:06, jbroman wrote:
> On 2014/03/17 18:50:17, Florin Malita wrote:
> > lgtm + nit.
> > 
> >
>
https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
> > File Source/platform/graphics/Gradient.cpp (right):
> > 
> >
>
https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
> > Source/platform/graphics/Gradient.cpp:181: colors[0] = stop->color.rgb();
> > We should use SkColorSetARGB() to avoid any packing assumptions.
> > 
> >
>
https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
> > Source/platform/graphics/Gradient.cpp:187: colors[i] = stop->color.rgb();
> > Ditto.
> 
> This is the idiom we use everywhere to convert WebCore::Color to SkColor (e.g.
> look at any call to SkPaint::setColor in GraphicsContext or
> GraphicsContextState).

The idiom is wrong as it may very well break if/when Skia decides to change its
SkColor packing :)


> If we want to change that, wouldn't it be better to give WebCore::Color a
method
> to convert to SkColor or something like that (bearing in mind it would more or
> less be the same as rgb)?

Currently, the plan is to refactor simple Blink types to be Skia type backed.
That means medium/long term we want Color to be just a simple wrapper around
SkColor - that solves the conversion problem nicely. In the meantime I still
think it's a good idea to use macros and avoid packing assumptions at the
Blink/Skia boundary.

[jbroman, 2014-03-17 20:53:31]

On 2014/03/17 19:01:00, Florin Malita wrote:
> On 2014/03/17 18:53:06, jbroman wrote:
> > On 2014/03/17 18:50:17, Florin Malita wrote:
> > > lgtm + nit.
> > > 
> > >
> >
>
https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
> > > File Source/platform/graphics/Gradient.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
> > > Source/platform/graphics/Gradient.cpp:181: colors[0] = stop->color.rgb();
> > > We should use SkColorSetARGB() to avoid any packing assumptions.
> > > 
> > >
> >
>
https://codereview.chromium.org/201503002/diff/1/Source/platform/graphics/Gra...
> > > Source/platform/graphics/Gradient.cpp:187: colors[i] = stop->color.rgb();
> > > Ditto.
> > 
> > This is the idiom we use everywhere to convert WebCore::Color to SkColor
(e.g.
> > look at any call to SkPaint::setColor in GraphicsContext or
> > GraphicsContextState).
> 
> The idiom is wrong as it may very well break if/when Skia decides to change
its
> SkColor packing :)

My vague impression was that this was a rigid part of the Skia API. Done.
(A little investigation reveals that compilers are apparently smart enough to
optimize this away anyhow. Neat.)

> > If we want to change that, wouldn't it be better to give WebCore::Color a
> method
> > to convert to SkColor or something like that (bearing in mind it would more
or
> > less be the same as rgb)?
> 
> Currently, the plan is to refactor simple Blink types to be Skia type backed.
> That means medium/long term we want Color to be just a simple wrapper around
> SkColor - that solves the conversion problem nicely. In the meantime I still
> think it's a good idea to use macros and avoid packing assumptions at the
> Blink/Skia boundary.

What do you think about adding WebCore::Color::operator SkColor (or an explicit
method if we prefer, but this would be an essentially-free conversion)? Its
implementation would become totally trivial when/if Color holds an SkColor
instead of an RGBA32 (which, amusingly, stores things in ARGB order).

(As an aside, there is similar confusion at the WebColor boundary, where
WebColor is a typedef for "unsigned", but a comment saying that it's an SkColor
and an ARGB quad, whereas SkColor is a "uint32_t". We also seem to write
static_cast<WebColor>(foo.rgb()) at that boundary.)

[f(malita), 2014-03-17 21:43:24]

On 2014/03/17 20:53:31, jbroman wrote:
> On 2014/03/17 19:01:00, Florin Malita wrote:
> > The idiom is wrong as it may very well break if/when Skia decides to change
> its
> > SkColor packing :)
> 
> My vague impression was that this was a rigid part of the Skia API. Done.

Thanks. It certainly hasn't changed AFAICT, but Skia provides those macros
precisely to prevent external dependencies on its internal format.


> > > If we want to change that, wouldn't it be better to give WebCore::Color a
> > method
> > > to convert to SkColor or something like that (bearing in mind it would
more
> or
> > > less be the same as rgb)?
> > 
> > Currently, the plan is to refactor simple Blink types to be Skia type
backed.
> > That means medium/long term we want Color to be just a simple wrapper around
> > SkColor - that solves the conversion problem nicely. In the meantime I still
> > think it's a good idea to use macros and avoid packing assumptions at the
> > Blink/Skia boundary.
> 
> What do you think about adding WebCore::Color::operator SkColor (or an
explicit
> method if we prefer, but this would be an essentially-free conversion)? Its
> implementation would become totally trivial when/if Color holds an SkColor
> instead of an RGBA32 (which, amusingly, stores things in ARGB order).

SGTM! (and yes, RGBA32 is one big lie :)

[jbroman, 2014-03-17 22:53:39]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-03-17 22:53:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/201503002/20001

[commit-bot: I haz the power, 2014-03-17 22:54:27]

Change committed as 169396