Implement color stop position syntax from CSS Image Values 4

Implement color stop position syntax from CSS Image Values 4

CSS Image Values 4 allows specifying up to two positions per color stop, e.g.:

  linear-gradient(45deg, black 0% 50%, white 50% 100%)

This is equivalent to repeating the color at the given positions:

  linear-gradient(45deg, black 0%, black 50%, white 50%, white 100%)

(https://drafts.csswg.org/css-images-4/#color-stop-syntax)

Add support for the new syntax, behind a runtime flag.

BUG=707047

[f(malita), 2017-04-05 20:55:09]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 20:55:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-04-05 21:45:04]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 21:45:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-04-05 22:08:33]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 22:09:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 23:51:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 23:51:39]

Dry run: This issue passed the CQ dry run.

[f(malita), 2017-04-06 16:57:26]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 16:57:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-04-06 17:10:50]

Description was changed from

==========
Implement color stop position syntax from CSS Image Values 4

WIP

BUG=707047
==========

to

==========
Implement color stop position syntax from CSS Image Values 4

CSS Image Values 4 allows specifying up to two positions per color stop, e.g.:

  linear-gradient(45deg, black 0% 50%, white 50% 100%)

This is equivalent to repeating the color at the given positions:

  linear-gradient(45deg, black 0%, black 50%, white 50%, white 100%)

(https://drafts.csswg.org/css-images-4/#color-stop-syntax)

Add support for the new syntax, behind a runtime flag.

BUG=707047
==========

[f(malita), 2017-04-06 17:10:50]

fmalita@chromium.org changed reviewers:
+ fs@opera.com, schenney@chromium.org

[f(malita), 2017-04-06 19:12:35]

Per offline conversation with rbyers@, it should be OK to land this behind a
flag and wait for a CSSWG CR before shipping.

[commit-bot: I haz the power, 2017-04-06 19:14:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 19:14:53]

Dry run: This issue passed the CQ dry run.

[fs, 2017-04-06 19:58:48]

LGTM w/ some thoughts on additional tests, and some REF feature "wrapping"
pondering.

https://codereview.chromium.org/2799793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSGradientValue.cpp (right):

https://codereview.chromium.org/2799793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSGradientValue.cpp:1081: // At most two
consecutive stops can share the same CSS value for color.
Could we add a test for this as well - both the "compressing" behavior ("blue
10%, blue 20%" serializing as "blue 10% 20%") and that more than 2 stops are not
joined ("blue 10%, blue 20%, blue 30%" -> "blue 10% 20%, blue 30%" or so I
presume...) It might be easier to write this in "forward consuming" fashion
rather than "backward merge" (like this). Also, another case to test: "blue 10%,
blue 20% 30%".

https://codereview.chromium.org/2799793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2799793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:972:
!RuntimeEnabledFeatures::multipleColorStopPositionsEnabled())
Ponder: Maybe we could have this as a:

if (RuntimeEnabledFeatures::multipleColorStopPositionsEnabled()) {
  ...parsing of additional offset...
}

block instead?

[f(malita), 2017-04-06 20:29:04]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[f(malita), 2017-04-06 20:29:43]

https://codereview.chromium.org/2799793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSGradientValue.cpp (right):

https://codereview.chromium.org/2799793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSGradientValue.cpp:1081: // At most two
consecutive stops can share the same CSS value for color.
On 2017/04/06 19:58:48, fs wrote:
> Could we add a test for this as well - both the "compressing" behavior ("blue
> 10%, blue 20%" serializing as "blue 10% 20%") and that more than 2 stops are
not
> joined ("blue 10%, blue 20%, blue 30%" -> "blue 10% 20%, blue 30%" or so I
> presume...) It might be easier to write this in "forward consuming" fashion
> rather than "backward merge" (like this). Also, another case to test: "blue
10%,
> blue 20% 30%".

Actually, the current impl bends over backwards to preserve the original format:

  "blue 10%, blue 20%" -> "blue 10%, blue 20%"
  "blue 10% 20%"       -> "blue 10% 20%"

(we use the CSSValue pointer to detect whether the color value is shared by two
consecutive stops, and only compress if that's the case)

The reason is this serializer appears to feed getComputedValue(), and the
computed value for background-image is supposed to be "as specified, but with
URIs made absolute"
(https://www.w3.org/TR/css3-background/#the-background-image).

I don't feel strong either way, but based on the spec language it seemed more
prudent to preserve the input as much as possible.  WDYT?

https://codereview.chromium.org/2799793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2799793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:972:
!RuntimeEnabledFeatures::multipleColorStopPositionsEnabled())
On 2017/04/06 19:58:48, fs wrote:
> Ponder: Maybe we could have this as a:
> 
> if (RuntimeEnabledFeatures::multipleColorStopPositionsEnabled()) {
>   ...parsing of additional offset...
> }
> 
> block instead?

Done.

[commit-bot: I haz the power, 2017-04-06 20:29:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2017-04-06 20:44:36]

https://codereview.chromium.org/2799793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSGradientValue.cpp (right):

https://codereview.chromium.org/2799793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSGradientValue.cpp:1081: // At most two
consecutive stops can share the same CSS value for color.
On 2017/04/06 at 20:29:42, f(malita) wrote:
> On 2017/04/06 19:58:48, fs wrote:
> > Could we add a test for this as well - both the "compressing" behavior
("blue
> > 10%, blue 20%" serializing as "blue 10% 20%") and that more than 2 stops are
not
> > joined ("blue 10%, blue 20%, blue 30%" -> "blue 10% 20%, blue 30%" or so I
> > presume...) It might be easier to write this in "forward consuming" fashion
> > rather than "backward merge" (like this). Also, another case to test: "blue
10%,
> > blue 20% 30%".
> 
> Actually, the current impl bends over backwards to preserve the original
format:
> 
>   "blue 10%, blue 20%" -> "blue 10%, blue 20%"
>   "blue 10% 20%"       -> "blue 10% 20%"
> 
> (we use the CSSValue pointer to detect whether the color value is shared by
two consecutive stops, and only compress if that's the case)

Oh, yeah, right, the stop.color == stops[i - 1].color was comparing the
objects...
 
> The reason is this serializer appears to feed getComputedValue(), and the
computed value for background-image is supposed to be "as specified, but with
URIs made absolute"
(https://www.w3.org/TR/css3-background/#the-background-image).
> 
> I don't feel strong either way, but based on the spec language it seemed more
prudent to preserve the input as much as possible.  WDYT?

I was looking around to find relevant serialization rules, and the only thing I
found was the ones in CSS Image L3:
https://drafts.csswg.org/css-images-3/#serialization ; which I guess essentially
says to emit "as little as possible". I'm not sure if I think this "as
specified" in css3-background actually makes sense (it does for <url> but that
not much to go on), since at least 'currentColor' stops ought to compute to the
actual color (not that it does in Blink.) Anyway, we can keep it as-is (this is
fuzzy territory after all), but then having the tests would be even more
valuable I think =).

[f(malita), 2017-04-06 21:01:04]

On 2017/04/06 20:44:36, fs wrote:
> Anyway, we can keep it as-is (this is
> fuzzy territory after all), but then having the tests would be even more
> valuable I think =).

Ha!  So I was convinced we must have coverage for non-compressing behavior, but
then I started adding tests and found out it behaves as you thought, not as I
did :)

"black 10%, black 20%" -> "black 10% 20%"

@#$@#$?!

Turns out that *some* CSSColorValues are cached - thus we cannot disambiguate
based on the pointer:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSCo...

So in order to preserve the original format, we'd have to tag each color stop -
which is exactly what I was trying to avoid :(

Since we're on the fence, I think compressing greedily would be a cleaner thing
to do at this point.  I'll update.

[fs, 2017-04-06 21:05:49]

On 2017/04/06 at 21:01:04, fmalita wrote:
> On 2017/04/06 20:44:36, fs wrote:
> > Anyway, we can keep it as-is (this is
> > fuzzy territory after all), but then having the tests would be even more
> > valuable I think =).
> 
> Ha!  So I was convinced we must have coverage for non-compressing behavior,
but then I started adding tests and found out it behaves as you thought, not as
I did :)
> 
> "black 10%, black 20%" -> "black 10% 20%"
> 
> @#$@#$?!
> 
> Turns out that *some* CSSColorValues are cached - thus we cannot disambiguate
based on the pointer:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSCo...

Well, at least that part of the caching is deterministic =)

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSVa...

[Rick Byers, 2017-04-06 21:35:05]

On 2017/04/06 19:12:35, f(malita) wrote:
> Per offline conversation with rbyers@, it should be OK to land this behind a
> flag and wait for a CSSWG CR before shipping.

It's definitely always OK to land experimental stuff behind the flag.  The spec
not being CR doesn't necessarily block us from shipping, but the CSSWG typically
prefers we wait.  It's up to you when you want to do an intent-to-ship.

[f(malita), 2017-04-06 22:06:34]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 22:08:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-04-06 22:10:00]

On 2017/04/06 21:01:04, f(malita) wrote:
> 
> Since we're on the fence, I think compressing greedily would be a cleaner
thing
> to do at this point.  I'll update.

Done.

[commit-bot: I haz the power, 2017-04-07 00:00:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 00:01:00]

Dry run: This issue passed the CQ dry run.

[fs, 2017-04-07 07:41:55]

On 2017/04/06 at 22:10:00, fmalita wrote:
> On 2017/04/06 21:01:04, f(malita) wrote:
> > 
> > Since we're on the fence, I think compressing greedily would be a cleaner
thing
> > to do at this point.  I'll update.
> 
> Done.

Thanks, still LGTM.

[f(malita), 2017-04-07 11:56:58]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2017-04-07 11:57:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 12:02:30]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491566218881550,
"parent_rev": "15343e304623319dfd5612ac3d22930977a7e20c", "commit_rev":
"546c77f5d4ce77af661664954f0a59d7a4b45edf"}

[commit-bot: I haz the power, 2017-04-07 12:07:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 12:07:36]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[f(malita), 2017-04-07 13:13:19]

On 2017/04/07 12:07:36, commit-bot: I haz the power wrote:
> Prior attempt to commit was detected, but we were not able to check whether
the
> issue was successfully committed. Please check Git history manually and
re-check
> CQ or close this issue as needed.

Yes, confused commit-bot -- the issue was indeed committed:
https://chromium.googlesource.com/chromium/src/+/546c77f5d4ce77af661664954f0a....