Adding support for color interpolation hints to CSS gradients.

Adding support for color interpolation hints to CSS gradients.

CSS images level 4 defines support for gradient color interpolation
hints (aka midpoints):
http://dev.w3.org/csswg/css-images-4/#color-interpolation-hint

Webkit: https://bugs.webkit.org/show_bug.cgi?id=137171
Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1074056

Manual test: http://codepen.io/adobe/full/fhnBJ

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

[rosca, 2014-10-06 10:43:30]

rosca@adobe.com changed reviewers:
+ fmalita@chromium.org

[rosca, 2014-10-06 10:43:31]

Hi Florin, could you please review this patch?

[f(malita), 2014-10-08 16:13:55]

Looks good, just some nits.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
File Source/core/css/CSSGradientValue.cpp (right):

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:40: #include
"platform/graphics/skia/SkiaUtils.h"
Is this used at all?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:119: if (stop.m_color &&
stop.m_color->colorIsDerivedFromElement()) {
I think these checks would look better if we add an explicit
CSSGradientColorStop helper:

bool CSSGradientColorStop::isHint() const {
    ASSERT(m_color || m_position);
    return !m_color;
}

WDYT?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:139: if (stop.m_color)
!stop.isHint()?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:147: {
ASSERT(p >= 0 && p <= 1)?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:160: static void
replaceColorHintsWithColorStops(Vector<GradientStop>& stops, const
Vector<CSSGradientColorStop, 2>& cssGradientStops)
Since cssGradientStops is always a CSSGradientValue field, can we make this a
non-static const CSSGradientValue method with a single parameter (stops)?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:180: if (cssGradientStops[i].m_color)
!cssGradientStops[i].isHint()?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:184: size_t x = i + indexOffset;
Let's ASSERT(x >= 1) since indexOffset can potentially become negative.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:203: if (WebCoreFloatNearlyEqual(leftDist,
rightDist)) {
Should we also drop the hint if offset1 == offset2 (overlapping color stops =>
no interpolation)?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:278: size_t numColorHints = 0;
I don't think we really care about the number here, this can be bool hasHints =
false;

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:290: if (stop.m_color)
!stop.isHint()?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:293: ++numColorHints;
hasHints = true?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:372: if (numColorHints) {
hasHints?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:593: if (stop.m_color &&
stop.m_resolvedColor.hasAlpha())
!stop.isHint()?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:6827: const bool kSupportColorHints
=
bool supportsColorHints = ...

(not a static const)

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:6849: // Two midpoints in a row is
not allowed.
// Two hints in a row are not allowed.

[rosca, 2014-10-08 17:18:52]

Thanks for the review!

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
File Source/core/css/CSSGradientValue.cpp (right):

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:40: #include
"platform/graphics/skia/SkiaUtils.h"
On 2014/10/08 16:13:54, Florin Malita wrote:
> Is this used at all?

Yes, it is used for WebCoreFloatNearlyEqual(...)

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:119: if (stop.m_color &&
stop.m_color->colorIsDerivedFromElement()) {
On 2014/10/08 16:13:54, Florin Malita wrote:
> I think these checks would look better if we add an explicit
> CSSGradientColorStop helper:
> 
> bool CSSGradientColorStop::isHint() const {
>     ASSERT(m_color || m_position);
>     return !m_color;
> }
> 
> WDYT?
Sounds good. I added it inline.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:139: if (stop.m_color)
On 2014/10/08 16:13:54, Florin Malita wrote:
> !stop.isHint()?

Done.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:147: {
On 2014/10/08 16:13:54, Florin Malita wrote:
> ASSERT(p >= 0 && p <= 1)?

Done.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:160: static void
replaceColorHintsWithColorStops(Vector<GradientStop>& stops, const
Vector<CSSGradientColorStop, 2>& cssGradientStops)
On 2014/10/08 16:13:54, Florin Malita wrote:
> Since cssGradientStops is always a CSSGradientValue field, can we make this a
> non-static const CSSGradientValue method with a single parameter (stops)?

The problem here is that "struct GradientStop" is define here in cpp, being an
implementation detail. Do you think we should move it to the interface, as well?

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:180: if (cssGradientStops[i].m_color)
On 2014/10/08 16:13:54, Florin Malita wrote:
> !cssGradientStops[i].isHint()?

Done.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:184: size_t x = i + indexOffset;
On 2014/10/08 16:13:54, Florin Malita wrote:
> Let's ASSERT(x >= 1) since indexOffset can potentially become negative.

Done.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:203: if (WebCoreFloatNearlyEqual(leftDist,
rightDist)) {
On 2014/10/08 16:13:54, Florin Malita wrote:
> Should we also drop the hint if offset1 == offset2 (overlapping color stops =>
> no interpolation)?
This expression (leftDist == rightDist) should always be true when offset1 ==
offset2.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:278: size_t numColorHints = 0;
On 2014/10/08 16:13:54, Florin Malita wrote:
> I don't think we really care about the number here, this can be bool hasHints
=
> false;

Done.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:290: if (stop.m_color)
On 2014/10/08 16:13:54, Florin Malita wrote:
> !stop.isHint()?

Done.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:293: ++numColorHints;
On 2014/10/08 16:13:54, Florin Malita wrote:
> hasHints = true?

Done.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:372: if (numColorHints) {
On 2014/10/08 16:13:54, Florin Malita wrote:
> hasHints?

Done.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:593: if (stop.m_color &&
stop.m_resolvedColor.hasAlpha())
On 2014/10/08 16:13:54, Florin Malita wrote:
> !stop.isHint()?

Done.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:6827: const bool kSupportColorHints
=
On 2014/10/08 16:13:54, Florin Malita wrote:
> bool supportsColorHints = ...
> 
> (not a static const)

Done.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:6849: // Two midpoints in a row is
not allowed.
On 2014/10/08 16:13:54, Florin Malita wrote:
> // Two hints in a row are not allowed.

Done.

[f(malita), 2014-10-08 17:34:30]

Thanks. LGTM.

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
File Source/core/css/CSSGradientValue.cpp (right):

https://codereview.chromium.org/631753002/diff/20001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:160: static void
replaceColorHintsWithColorStops(Vector<GradientStop>& stops, const
Vector<CSSGradientColorStop, 2>& cssGradientStops)
On 2014/10/08 17:18:52, rosca wrote:
> On 2014/10/08 16:13:54, Florin Malita wrote:
> > Since cssGradientStops is always a CSSGradientValue field, can we make this
a
> > non-static const CSSGradientValue method with a single parameter (stops)?
> 
> The problem here is that "struct GradientStop" is define here in cpp, being an
> implementation detail. Do you think we should move it to the interface, as
well?

Ah, you're right, I missed that part. Let's leave it alone, not a big deal.

[rosca, 2014-10-08 18:54:30]

The CQ bit was checked by rosca@adobe.com

[rosca, 2014-10-08 18:55:04]

The CQ bit was unchecked by rosca@adobe.com

[Timothy Loh, 2014-10-09 06:41:09]

timloh@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2014-10-09 06:41:10]

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
File Source/core/css/CSSGradientValue.cpp (right):

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:152: static inline Color interpolate(Color
c1, Color c2, float p)
There's already a blend function in Color.h

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:180: // Not a color interpolation hint.
This comment doesn't add any value

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:204: // If the color hint is positioned
exactly in the middle, we can remove it.
ditto

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:211: // Check if the color hint coincides
with the left color stop.
ditto

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:217: // Check if the color hint coincides
with the right color stop.
ditto

[rosca, 2014-10-09 09:32:03]

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
File Source/core/css/CSSGradientValue.cpp (right):

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:152: static inline Color interpolate(Color
c1, Color c2, float p)
On 2014/10/09 06:41:09, Timothy Loh wrote:
> There's already a blend function in Color.h

Thanks, I will use it. Also, I should have used the premultiplied colors in the
interpolation, and the blend function has this option. Now color stops with
transparent colors look more correct.

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:180: // Not a color interpolation hint.
On 2014/10/09 06:41:09, Timothy Loh wrote:
> This comment doesn't add any value

Done.

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:204: // If the color hint is positioned
exactly in the middle, we can remove it.
On 2014/10/09 06:41:09, Timothy Loh wrote:
> ditto

Done.

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:211: // Check if the color hint coincides
with the left color stop.
On 2014/10/09 06:41:09, Timothy Loh wrote:
> ditto

Done.

https://codereview.chromium.org/631753002/diff/40001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:217: // Check if the color hint coincides
with the right color stop.
On 2014/10/09 06:41:09, Timothy Loh wrote:
> ditto

Done.

[Timothy Loh, 2014-10-09 10:31:32]

lgtm

https://codereview.chromium.org/631753002/diff/60001/Source/core/css/CSSGradi...
File Source/core/css/CSSGradientValue.cpp (right):

https://codereview.chromium.org/631753002/diff/60001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:176: float offset1 = stops[x - 1].offset;
offsetLeft / offsetRight reads better

[rosca, 2014-10-09 11:17:17]

https://codereview.chromium.org/631753002/diff/60001/Source/core/css/CSSGradi...
File Source/core/css/CSSGradientValue.cpp (right):

https://codereview.chromium.org/631753002/diff/60001/Source/core/css/CSSGradi...
Source/core/css/CSSGradientValue.cpp:176: float offset1 = stops[x - 1].offset;
On 2014/10/09 10:31:32, Timothy Loh wrote:
> offsetLeft / offsetRight reads better

Done.

[rosca, 2014-10-09 11:57:25]

The CQ bit was checked by rosca@adobe.com

[commit-bot: I haz the power, 2014-10-09 11:58:22]

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

[commit-bot: I haz the power, 2014-10-09 13:08:22]

Committed patchset #5 (id:80001) as 183460