CSS Motion Path: Support parsing of ray(<angle>) paths

CSS Motion Path: Support parsing of ray(<angle>) paths

Spec: https://drafts.fxtf.org/motion-1/
offset-path can be ray(<angle> && <size> && contain?)

Intent To Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/uqDmsIj6zJY/cS-Vp_XG2oYJ

BUG=641245
Review-Url: https://codereview.chromium.org/2881673003
Cr-Commit-Position: refs/heads/master@{#471987}
Committed: https://chromium.googlesource.com/chromium/src/+/2319cee58d13a3ab8d876c263fc130b2405f725b

[Eric Willigers, 2017-05-12 05:30:26]

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

[commit-bot: I haz the power, 2017-05-12 05:30:42]

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-05-12 06:53:55]

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

[commit-bot: I haz the power, 2017-05-12 06:53:55]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Eric Willigers, 2017-05-15 07:52:05]

Patchset #2 (id:20001) has been deleted

[Eric Willigers, 2017-05-15 07:53:10]

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

[commit-bot: I haz the power, 2017-05-15 07:53:31]

Dry run: CQ is trying da patch.

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

[Eric Willigers, 2017-05-15 07:53:41]

Description was changed from

==========
CSS Motion Path: Support parsing of ray(<angle>) paths

Spec: https://drafts.fxtf.org/motion-1/
says offset-path can be ray(<angle> && <size>? && contain?)

We initially implement support for ray(<angle>) only.

Intent To Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/uqDmsIj6zJY/cS-Vp_XG...

BUG=641245
==========

to

==========
CSS Motion Path: Support parsing of ray(<angle>) paths

Spec: https://drafts.fxtf.org/motion-1/
offset-path can be ray(<angle> && <size> && contain?)

Intent To Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/uqDmsIj6zJY/cS-Vp_XG...

BUG=641245
==========

[Eric Willigers, 2017-05-15 07:53:41]

ericwilligers@chromium.org changed reviewers:
+ fs@opera.com

[commit-bot: I haz the power, 2017-05-15 08:09:37]

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

[commit-bot: I haz the power, 2017-05-15 08:09:38]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Eric Willigers, 2017-05-15 08:24:40]

Patchset #2 (id:40001) has been deleted

[Eric Willigers, 2017-05-15 08:24:50]

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

[commit-bot: I haz the power, 2017-05-15 08:25:08]

Dry run: CQ is trying da patch.

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

[fs, 2017-05-15 09:18:15]

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/css-parser/offset-path-parsing.html (right):

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css-parser/offset-path-parsing.html:15:
assert_valid_value("offset-path", "ray(400grad farthest-corner contain)");
Maybe add a test with calc(...) for the <angle>.

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

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/BasicShapeFunctions.cpp:35: #include
"core/css/CSSRayValue.h"
(Only needed because StyleRay returns CSSRayValue - does it have to? Could it
return a plain CSSValue*?)

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSRayValue.h (right):

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSRayValue.h:18: static CSSRayValue*
Create(CSSValueList*);
Nit: const CSSValueList& ?

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSRayValue.h:21: const CSSIdentifierValue*
Size() const { return size_.Get(); }
Nit: Maybe the two that are not supposed to (cannot) be null can return
references instead?

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

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyOffsetPathUtils.cpp:53:
CSSValueList* arguments = CSSValueList::CreateSpaceSeparated();
Rather than constructing a temporary list, could the three arguments be passed
to CSSRayValue::Create(...) instead? (Three is still not "too many" I think.)

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyOffsetPathUtils.cpp:65:
CSSValueSides, CSSValueCover>(function_args)))
I didn't see 'cover' in <size>? C'n'P?

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StylePath.cpp (right):

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StylePath.cpp:70: // FIXME(ericwilligers):
Implement animation for offset-path.
TODO(...) per current guidelines

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleRay.cpp (right):

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleRay.cpp:53: PassRefPtr<StyleRay>
StyleRay::Create(const CSSRayValue& value) {
Could we have this just take the three values, and let the style converter do
the work?

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleRay.cpp:63: CSSRayValue*
StyleRay::ComputedCSSValue() const {
I think this might as well go in ValueForBasicShape (the major reason StylePath
has this is to allow sharing of the path-data - something that isn't needed
here.)

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleRay.cpp:77: return angle_ ==
other.angle_;
Should also check size_ and contain_?

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleRay.cpp:82: // for rays as they may
have infinite length.
This is kind of an indication that ray(...) isn't a great match for
<basic-shape> in general. Doesn't prevent treating it as such internally of
course.

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleRay.cpp:87: // FIXME(ericwilligers):
Implement animation for offset-path.
TODO

[commit-bot: I haz the power, 2017-05-15 10:31:02]

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

[commit-bot: I haz the power, 2017-05-15 10:31:03]

Dry run: This issue passed the CQ dry run.

[Eric Willigers, 2017-05-15 10:52:17]

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

[Eric Willigers, 2017-05-15 10:52:23]

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/css-parser/offset-path-parsing.html (right):

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css-parser/offset-path-parsing.html:15:
assert_valid_value("offset-path", "ray(400grad farthest-corner contain)");
On 2017/05/15 09:18:14, fs wrote:
> Maybe add a test with calc(...) for the <angle>.

Done.

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

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/BasicShapeFunctions.cpp:35: #include
"core/css/CSSRayValue.h"
On 2017/05/15 09:18:14, fs wrote:
> (Only needed because StyleRay returns CSSRayValue - does it have to? Could it
> return a plain CSSValue*?)

Done, then undone because ValueForBasicShape now calls CSSRayValue::Create.

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSRayValue.h (right):

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSRayValue.h:18: static CSSRayValue*
Create(CSSValueList*);
On 2017/05/15 09:18:14, fs wrote:
> Nit: const CSSValueList& ?

Required arguments now passed by reference.

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSRayValue.h:21: const CSSIdentifierValue*
Size() const { return size_.Get(); }
On 2017/05/15 09:18:14, fs wrote:
> Nit: Maybe the two that are not supposed to (cannot) be null can return
> references instead?

Done.

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

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyOffsetPathUtils.cpp:53:
CSSValueList* arguments = CSSValueList::CreateSpaceSeparated();
On 2017/05/15 09:18:14, fs wrote:
> Rather than constructing a temporary list, could the three arguments be passed
> to CSSRayValue::Create(...) instead? (Three is still not "too many" I think.)

Done.

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyOffsetPathUtils.cpp:65:
CSSValueSides, CSSValueCover>(function_args)))
On 2017/05/15 09:18:14, fs wrote:
> I didn't see 'cover' in <size>? C'n'P?

Oops. Fixed.

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StylePath.cpp (right):

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StylePath.cpp:70: // FIXME(ericwilligers):
Implement animation for offset-path.
On 2017/05/15 09:18:14, fs wrote:
> TODO(...) per current guidelines

Done.

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleRay.cpp (right):

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleRay.cpp:53: PassRefPtr<StyleRay>
StyleRay::Create(const CSSRayValue& value) {
On 2017/05/15 09:18:14, fs wrote:
> Could we have this just take the three values, and let the style converter do
> the work?

Done.

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleRay.cpp:63: CSSRayValue*
StyleRay::ComputedCSSValue() const {
On 2017/05/15 09:18:14, fs wrote:
> I think this might as well go in ValueForBasicShape (the major reason
StylePath
> has this is to allow sharing of the path-data - something that isn't needed
> here.)

Done.

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleRay.cpp:77: return angle_ ==
other.angle_;
On 2017/05/15 09:18:14, fs wrote:
> Should also check size_ and contain_?

Done.

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleRay.cpp:82: // for rays as they may
have infinite length.
On 2017/05/15 09:18:14, fs wrote:
> This is kind of an indication that ray(...) isn't a great match for
> <basic-shape> in general. Doesn't prevent treating it as such internally of
> course.

Acknowledged.

https://codereview.chromium.org/2881673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleRay.cpp:87: // FIXME(ericwilligers):
Implement animation for offset-path.
On 2017/05/15 09:18:14, fs wrote:
> TODO

Done.

[commit-bot: I haz the power, 2017-05-15 10:52:33]

Dry run: CQ is trying da patch.

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

[fs, 2017-05-15 11:05:39]

lgtm

https://codereview.chromium.org/2881673003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSPropertyOffsetPathUtils.cpp
(right):

https://codereview.chromium.org/2881673003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyOffsetPathUtils.cpp:48:
DCHECK(range.Peek().FunctionId() == CSSValueRay);
Nit: DCHECK_EQ?

https://codereview.chromium.org/2881673003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/2881673003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:45:
#include "core/css/CSSRayValue.h"
Nit: No longer needed I think?

https://codereview.chromium.org/2881673003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:56:
#include "core/style/StyleRay.h"
Ditto.

[Eric Willigers, 2017-05-15 11:12:13]

https://codereview.chromium.org/2881673003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSPropertyOffsetPathUtils.cpp
(right):

https://codereview.chromium.org/2881673003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyOffsetPathUtils.cpp:48:
DCHECK(range.Peek().FunctionId() == CSSValueRay);
On 2017/05/15 11:05:39, fs wrote:
> Nit: DCHECK_EQ?

Done.

https://codereview.chromium.org/2881673003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/2881673003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:45:
#include "core/css/CSSRayValue.h"
On 2017/05/15 11:05:39, fs wrote:
> Nit: No longer needed I think?

Removed.

https://codereview.chromium.org/2881673003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:56:
#include "core/style/StyleRay.h"
On 2017/05/15 11:05:39, fs wrote:
> Ditto.

Removed.

[Eric Willigers, 2017-05-15 20:57:14]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2017-05-15 20:57:14]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/2881673003/#ps100001
(title: "DCHECK_EQ")

[commit-bot: I haz the power, 2017-05-15 20:58:15]

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-05-15 21:35:44]

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

[commit-bot: I haz the power, 2017-05-15 21:35:44]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Eric Willigers, 2017-05-15 22:35:16]

ericwilligers@chromium.org changed reviewers:
+ pdr@chromium.org

[Eric Willigers, 2017-05-15 22:35:16]

+pdr for platform OWNERS

[pdr., 2017-05-15 22:37:26]

LGTM

https://codereview.chromium.org/2881673003/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/css-parser/offset-path-parsing.html (right):

https://codereview.chromium.org/2881673003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css-parser/offset-path-parsing.html:1: <!DOCTYPE
html>
Should this be a web platform test that we can share with other vendors?

[Eric Willigers, 2017-05-15 23:42:53]

https://codereview.chromium.org/2881673003/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/css-parser/offset-path-parsing.html (right):

https://codereview.chromium.org/2881673003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css-parser/offset-path-parsing.html:1: <!DOCTYPE
html>
On 2017/05/15 22:37:26, pdr. wrote:
> Should this be a web platform test that we can share with other vendors?

Yes. I will next move all the motion path parsing tests to web platform tests.

[pdr., 2017-05-15 23:43:28]

On 2017/05/15 at 23:42:53, ericwilligers wrote:
>
https://codereview.chromium.org/2881673003/diff/100001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/css-parser/offset-path-parsing.html
(right):
> 
>
https://codereview.chromium.org/2881673003/diff/100001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/css-parser/offset-path-parsing.html:1:
<!DOCTYPE html>
> On 2017/05/15 22:37:26, pdr. wrote:
> > Should this be a web platform test that we can share with other vendors?
> 
> Yes. I will next move all the motion path parsing tests to web platform tests.

sweet, sounds great (still lgtm)

[Eric Willigers, 2017-05-16 00:01:37]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2017-05-16 00:03:26]

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-05-16 02:30:23]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494892897454570,
"parent_rev": "394d28a0fd538118bda1b530c1a37414ea28dbe0", "commit_rev":
"2319cee58d13a3ab8d876c263fc130b2405f725b"}

[commit-bot: I haz the power, 2017-05-16 02:30:40]

Description was changed from

==========
CSS Motion Path: Support parsing of ray(<angle>) paths

Spec: https://drafts.fxtf.org/motion-1/
offset-path can be ray(<angle> && <size> && contain?)

Intent To Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/uqDmsIj6zJY/cS-Vp_XG...

BUG=641245
==========

to

==========
CSS Motion Path: Support parsing of ray(<angle>) paths

Spec: https://drafts.fxtf.org/motion-1/
offset-path can be ray(<angle> && <size> && contain?)

Intent To Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/uqDmsIj6zJY/cS-Vp_XG...

BUG=641245

Review-Url: https://codereview.chromium.org/2881673003
Cr-Commit-Position: refs/heads/master@{#471987}
Committed:
https://chromium.googlesource.com/chromium/src/+/2319cee58d13a3ab8d876c263fc1...
==========

[commit-bot: I haz the power, 2017-05-16 02:30:41]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2319cee58d13a3ab8d876c263fc1...