Implements CSSPropertyAPI for the d property

Implements CSSPropertyAPI for the d property

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. This patch removes
CSSPropertyD from the switch statement in
parseSingleValue, and calls the API instead.

A function pointer to the parseSingleValue function from the API
for the d property is stored in a CSSPropertyDescriptor, and is
called from CSSPropertyParser.

This patch:
- Adds CSSPropertyAPID.cpp to the BUILD.gn file.
- Adds parseSingleValue to "d" in CSSProperties.json5
  so that it will be added to the generated files for the api.
- Moves the parsing logic for d from CSSPropertyParser.cpp to
  CSSPropertyAPID.cpp, which implements
  CSSPropertyAPI.h.

BUG=668012
Review-Url: https://codereview.chromium.org/2642033002
Cr-Commit-Position: refs/heads/master@{#471220}
Committed: https://chromium.googlesource.com/chromium/src/+/a90e416a49db962781bf5e3fc5cfd263346e5375

[Eric Willigers, 2017-01-19 02:42:22]

ericwilligers@chromium.org changed reviewers:
+ aazzam@google.com

[aazzam, 2017-01-19 23:06:56]

https://codereview.chromium.org/2642033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2642033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:538:
CSSValue* consumePath(CSSParserTokenRange& range) {
can you explain why you put consumePath in CSSPropertyParserHelpers? it looks to
me like it's only used by one method?

https://codereview.chromium.org/2642033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/properties/CSSPropertyAPID.cpp (right):

https://codereview.chromium.org/2642033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/properties/CSSPropertyAPID.cpp:12: const
CSSValue* CSSPropertyAPID::parseSingleValue(
is the logic here now duplicated from consumePathOrNone in CSSPropertyParser?

[Eric Willigers, 2017-01-19 23:24:46]

https://codereview.chromium.org/2642033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2642033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:538:
CSSValue* consumePath(CSSParserTokenRange& range) {
On 2017/01/19 23:06:56, aazzam wrote:
> can you explain why you put consumePath in CSSPropertyParserHelpers? it looks
to
> me like it's only used by one method? 

consumePath is useful for 'd' and 'offset-path'

'd' is a path or none.

'offset-path' is about to receive a whole lot more options. For example, I
intend to support ray(<angle>) behind a flag this quarter.
https://drafts.fxtf.org/motion-1/#offset-path-property

https://codereview.chromium.org/2642033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/properties/CSSPropertyAPID.cpp (right):

https://codereview.chromium.org/2642033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/properties/CSSPropertyAPID.cpp:12: const
CSSValue* CSSPropertyAPID::parseSingleValue(
On 2017/01/19 23:06:56, aazzam wrote:
> is the logic here now duplicated from consumePathOrNone in CSSPropertyParser?

Temporarily.

I intent to rename consumePathOrNone to consumeOffsetPath in
CSSPropertyAPIOffsetPath.cpp anonymous namespace when implementing offset-path.

consumeOffsetPath will diverge from 'd' real-soon-now as more path syntaxes are
supported.

[Eric Willigers, 2017-05-11 04:18:19]

Description was changed from

==========
Implements CSSPropertyAPI for the d property

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. This patch removes
CSSPropertyD from the switch statement in
parseSingleValue, and calls the API instead.

A function pointer to the parseSingleValue function from the API for the
d property is stored in a CSSPropertyDescriptor, and is
called from CSSPropertyParser.

This patch:
- Adds CSSPropertyAPID.cpp to the BUILD.gn file.
- Adds api_class flag to CSSProperties.in, which indicates that
  CSSPropertyAPID.h is generated.
- Moves the parsing logic for d from CSSPropertyParser.cpp to
  CSSPropertyAPID.cpp, which implements
  CSSPropertyAPI.h.

BUG=668012
==========

to

==========
Implements CSSPropertyAPI for the d property

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. This patch removes
CSSPropertyD from the switch statement in
parseSingleValue, and calls the API instead.

A function pointer to the parseSingleValue function from the API for the
d property is stored in a CSSPropertyDescriptor, and is
called from CSSPropertyParser.

This patch:
- Adds CSSPropertyAPID.cpp to the BUILD.gn file.
- Adds parseSingleValue to "d" in CSSProperties.json5 so that it will be added
to the generated files for the api.
- Moves the parsing logic for d from CSSPropertyParser.cpp to
  CSSPropertyAPID.cpp, which implements
  CSSPropertyAPI.h.

BUG=668012
==========

[Eric Willigers, 2017-05-11 05:00:45]

ericwilligers@chromium.org changed reviewers:
+ bugsnash@chromium.org
- aazzam@google.com

[Eric Willigers, 2017-05-11 05:00:46]

Diffs in generated files:
https://gist.github.com/02809d79e0370f387453901ea9ace261/revisions

[Eric Willigers, 2017-05-11 05:00:58]

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

[commit-bot: I haz the power, 2017-05-11 05:03:04]

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-11 05:17:01]

Description was changed from

==========
Implements CSSPropertyAPI for the d property

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. This patch removes
CSSPropertyD from the switch statement in
parseSingleValue, and calls the API instead.

A function pointer to the parseSingleValue function from the API for the
d property is stored in a CSSPropertyDescriptor, and is
called from CSSPropertyParser.

This patch:
- Adds CSSPropertyAPID.cpp to the BUILD.gn file.
- Adds parseSingleValue to "d" in CSSProperties.json5 so that it will be added
to the generated files for the api.
- Moves the parsing logic for d from CSSPropertyParser.cpp to
  CSSPropertyAPID.cpp, which implements
  CSSPropertyAPI.h.

BUG=668012
==========

to

==========
Implements CSSPropertyAPI for the d property

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. This patch removes
CSSPropertyD from the switch statement in
parseSingleValue, and calls the API instead.

A function pointer to the parseSingleValue function from the API
for the d property is stored in a CSSPropertyDescriptor, and is
called from CSSPropertyParser.

This patch:
- Adds CSSPropertyAPID.cpp to the BUILD.gn file.
- Adds parseSingleValue to "d" in CSSProperties.json5
  so that it will be added to the generated files for the api.
- Moves the parsing logic for d from CSSPropertyParser.cpp to
  CSSPropertyAPID.cpp, which implements
  CSSPropertyAPI.h.

BUG=668012
==========

[commit-bot: I haz the power, 2017-05-11 09:12:23]

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

[commit-bot: I haz the power, 2017-05-11 09:12:23]

Dry run: This issue passed the CQ dry run.

[Bugs Nash, 2017-05-11 23:14:54]

lgtm with changes. Please also add the new .h file to core/BUILD.gn

https://codereview.chromium.org/2642033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/properties/CSSPropertyAPID.cpp (right):

https://codereview.chromium.org/2642033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyAPID.cpp:8: 
I would forward declare the classes CSSValue, CSSParserTokenRange,
CSSParserContext, and CSSPropertyID (even though it currently compiles, future
changes to other files could cause a compile error here).

[Eric Willigers, 2017-05-12 00:15:27]

On 2017/05/11 23:14:54, Bugs Nash wrote:
> ... add the new .h file to core/BUILD.gn

Done

[Eric Willigers, 2017-05-12 00:16:13]

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

[Eric Willigers, 2017-05-12 00:16:14]

https://codereview.chromium.org/2642033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/properties/CSSPropertyAPID.cpp (right):

https://codereview.chromium.org/2642033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyAPID.cpp:8: 
On 2017/05/11 23:14:53, Bugs Nash wrote:
> I would forward declare the classes CSSValue, CSSParserTokenRange,
> CSSParserContext, and CSSPropertyID (even though it currently compiles, future
> changes to other files could cause a compile error here).

I respectfully disagree.

Is it OK that CSSPropertyAPID.h is the first include in CSSPropertyAPID.cpp? I
assume yes.

Is CSSPropertyAPID::parseSingleValue declared in CSSPropertyAPID.h? Yes.

Thus CSSValue, CSSParserTokenRange, CSSParserContext must be forward
declared or defined, and CSSPropertyID must be defined, by the time
class CSSPropertyAPID is defined in CSSPropertyAPID.h.

Forward declaring again in CSSPropertyAPID.cpp can only ever be
redundant while these types appear in the method signature
const CSSValue* CSSPropertyAPID::parseSingleValue(
CSSParserTokenRange& range, const CSSParserContext&, CSSPropertyID)

[Bugs Nash, 2017-05-12 00:19:00]

On 2017/05/12 at 00:16:14, ericwilligers wrote:
>
https://codereview.chromium.org/2642033002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/properties/CSSPropertyAPID.cpp
(right):
> 
>
https://codereview.chromium.org/2642033002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/properties/CSSPropertyAPID.cpp:8: 
> On 2017/05/11 23:14:53, Bugs Nash wrote:
> > I would forward declare the classes CSSValue, CSSParserTokenRange,
> > CSSParserContext, and CSSPropertyID (even though it currently compiles,
future
> > changes to other files could cause a compile error here).
> 
> I respectfully disagree.
> 
> Is it OK that CSSPropertyAPID.h is the first include in CSSPropertyAPID.cpp? I
assume yes.
> 
> Is CSSPropertyAPID::parseSingleValue declared in CSSPropertyAPID.h? Yes.
> 
> Thus CSSValue, CSSParserTokenRange, CSSParserContext must be forward
> declared or defined, and CSSPropertyID must be defined, by the time
> class CSSPropertyAPID is defined in CSSPropertyAPID.h.
> 
> Forward declaring again in CSSPropertyAPID.cpp can only ever be
> redundant while these types appear in the method signature
> const CSSValue* CSSPropertyAPID::parseSingleValue(
> CSSParserTokenRange& range, const CSSParserContext&, CSSPropertyID)

sgtm, I actually forgot that this *wasn't* a header file :\

[Eric Willigers, 2017-05-12 00:22:16]

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

[commit-bot: I haz the power, 2017-05-12 00:22:58]

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 02:17:48]

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

[commit-bot: I haz the power, 2017-05-12 02:17:50]

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2017-05-12 04:25:34]

rs lgtm

[Eric Willigers, 2017-05-12 04:26:01]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2017-05-12 04:26:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
bugsnash@chromium.org
Link to the patchset: https://codereview.chromium.org/2642033002/#ps40001
(title: "add the new generated .h file to core/BUILD.gn")

[commit-bot: I haz the power, 2017-05-12 04:26: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-12 04:31:13]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494563161588710,
"parent_rev": "9dc275c518d70c4fb4652fdfb5b908d060b53296", "commit_rev":
"a90e416a49db962781bf5e3fc5cfd263346e5375"}

[commit-bot: I haz the power, 2017-05-12 04:31:24]

Description was changed from

==========
Implements CSSPropertyAPI for the d property

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. This patch removes
CSSPropertyD from the switch statement in
parseSingleValue, and calls the API instead.

A function pointer to the parseSingleValue function from the API
for the d property is stored in a CSSPropertyDescriptor, and is
called from CSSPropertyParser.

This patch:
- Adds CSSPropertyAPID.cpp to the BUILD.gn file.
- Adds parseSingleValue to "d" in CSSProperties.json5
  so that it will be added to the generated files for the api.
- Moves the parsing logic for d from CSSPropertyParser.cpp to
  CSSPropertyAPID.cpp, which implements
  CSSPropertyAPI.h.

BUG=668012
==========

to

==========
Implements CSSPropertyAPI for the d property

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. This patch removes
CSSPropertyD from the switch statement in
parseSingleValue, and calls the API instead.

A function pointer to the parseSingleValue function from the API
for the d property is stored in a CSSPropertyDescriptor, and is
called from CSSPropertyParser.

This patch:
- Adds CSSPropertyAPID.cpp to the BUILD.gn file.
- Adds parseSingleValue to "d" in CSSProperties.json5
  so that it will be added to the generated files for the api.
- Moves the parsing logic for d from CSSPropertyParser.cpp to
  CSSPropertyAPID.cpp, which implements
  CSSPropertyAPI.h.

BUG=668012

Review-Url: https://codereview.chromium.org/2642033002
Cr-Commit-Position: refs/heads/master@{#471220}
Committed:
https://chromium.googlesource.com/chromium/src/+/a90e416a49db962781bf5e3fc5cf...
==========

[commit-bot: I haz the power, 2017-05-12 04:31:25]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a90e416a49db962781bf5e3fc5cf...