Makes descriptors hold nullptr for properties not implemented in API.

Makes descriptors hold nullptr for properties not implemented in API.

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. 

This patch modifies make_css_property_apis.py and
CSSPropertyDescriptor.cpp.tmpl so that when the descriptors for a
property are initialised, properties that are not implemented are
set to nullptr. This removes the need for temporaryCanReadValue in
the descriptor struct.

The template is also changed so that it does not need to be modified
when new methods are added to the API.

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

[aazzam, 2017-02-01 00:11:12]

Description was changed from

==========
Makes descriptors hold nullptr for properties not implemented in API.

BUG=
==========

to

==========
Makes descriptors hold nullptr for properties not implemented in API.

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. 

This patch modifies make_css_property_apis.py and
CSSPropertyDescriptor.cpp.tmpl so that when the descriptors for a
property are initialised, properties that are not implemented are
set to nullptr. This removes the need for temporaryCanReadValue in
the descriptor struct.

The template is also changed so that it does not need to be modified
when new methods are added to the API.

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

[aazzam, 2017-02-01 00:12:54]

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

[aazzam, 2017-02-01 00:12:55]

@sashab, let me know what you think, I personally really like this, because it
makes adding a method to the API so much easier :)

[aazzam, 2017-02-01 00:13:12]

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

[aazzam, 2017-02-01 00:13:12]

also sending to kevin, ptal :)

[ktyliu, 2017-02-01 00:32:29]

cool!
seems great to make adding a new API method much simpler, though will leave to
Sasha to comment on the full implications of this

minor nits:

https://codereview.chromium.org/2670433002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/build/scripts/make_css_property_apis.py (right):

https://codereview.chromium.org/2670433002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:59:
'valid_api_methods': self.json5_file.parameters["api_methods"]["valid_values"],
would simply "api_methods" be a better name rather than "valid_api_methods"?

https://codereview.chromium.org/2670433002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl
(right):

https://codereview.chromium.org/2670433002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:28:
{{ api_class.classname }}:: {{ valid_api_method }},
remove extra space after {{ and before }} ?

[aazzam, 2017-02-01 06:02:28]

done, sasha ptal! :)

https://codereview.chromium.org/2670433002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/build/scripts/make_css_property_apis.py (right):

https://codereview.chromium.org/2670433002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:59:
'valid_api_methods': self.json5_file.parameters["api_methods"]["valid_values"],
On 2017/02/01 at 00:32:29, ktyliu wrote:
> would simply "api_methods" be a better name rather than "valid_api_methods"?

renamed :)

https://codereview.chromium.org/2670433002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:59:
'valid_api_methods': self.json5_file.parameters["api_methods"]["valid_values"],
On 2017/02/01 at 00:32:29, ktyliu wrote:
> would simply "api_methods" be a better name rather than "valid_api_methods"?

renamed :)

https://codereview.chromium.org/2670433002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl
(right):

https://codereview.chromium.org/2670433002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:28:
{{ api_class.classname }}:: {{ valid_api_method }},
On 2017/02/01 at 00:32:29, ktyliu wrote:
> remove extra space after {{ and before }} ?

done :)

[ktyliu, 2017-02-01 06:07:22]

lgtm

[aazzam, 2017-02-01 06:22:17]

aazzam@google.com changed reviewers:
+ gozzard@google.com

[aazzam, 2017-02-01 06:22:18]

PTAL gozz :P

[aazzam, 2017-02-01 06:22:18]

aazzam@google.com changed reviewers:
+ gozzard@google.com

[aazzam, 2017-02-01 06:22:19]

PTAL gozz :P

[gozzard, 2017-02-01 06:37:49]

lgtm

https://codereview.chromium.org/2670433002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl
(right):

https://codereview.chromium.org/2670433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:28:
{{api_class.classname}}:: {{api_method}},
nit: remove space after ::

[aazzam, 2017-02-01 06:38:32]

On 2017/02/01 at 06:37:49, gozzard wrote:
> lgtm
> 
>
https://codereview.chromium.org/2670433002/diff/20001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl
(right):
> 
>
https://codereview.chromium.org/2670433002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:28:
{{api_class.classname}}:: {{api_method}},
> nit: remove space after ::

done :P

[sashab, 2017-02-01 17:50:20]

I thought you removed a dependency here, but you've just moved it -- the list of
valid_values must now be complete and is order-sensitive. This is OK for now but
write a warning about it, and we can move to IDL or something to fix this.

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_css_property_apis.py (right):

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:34:
api_methods_for_class = defaultdict(set)
# Temporary map of API classname -> list of method names this API class
implements

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:39:
api_methods_for_class[classname].update(property['api_methods'])
api_methods_for_class[classname] = property['api_methods']

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:43: # Stores a
list of classes with elements (index, classname, [propertyIDs, ..],
api_methods).
[api_methods, ...]

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:59:
'api_methods': self.json5_file.parameters["api_methods"]["valid_values"],
Single quotes for strings

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl
(right):

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:18:
{
YAY getting rid of code dependencies!! \o/

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:27:
{% if api_method in api_class.api_methods %}
You can merge these:

{% for foo in bar if baz %}

Also be careful with your indenting -- see other tmpl files -- the indenting
needs to match C++, not Jinja2, so don't indent {%'s unless the C++ should be
indented :)

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:30:
nullptr,
One thing I'm worried about here is ordering. Since the descriptor is not
generated, we don't know the order these APIs will appear in, e.g if valid
values is [parseShorthand, parseSingleValue] all this code will fail.

This is OK for now because we can just add a comment to CSSProperties.json5, but
yet another reason to move to IDL. :)

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2046: if
(cssPropertyDesc.parseSingleValue)
omg awesome

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right):

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:30: //  
CSSProperties.json5.
Can you add a comment to CSSProperties.json5 too, since we now have a weird use
for valid values, like:

"api_methods": {
  // This list must be an ordered, complete list of methods whose names match
those defined in CSSPropertyDescriptor.h.
  "valid_values"

[aazzam, 2017-02-01 22:33:16]

ptal sasha :)

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_css_property_apis.py (right):

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:34:
api_methods_for_class = defaultdict(set)
On 2017/02/01 at 17:50:19, sashab wrote:
> # Temporary map of API classname -> list of method names this API class
implements

done :)

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:39:
api_methods_for_class[classname].update(property['api_methods'])
On 2017/02/01 at 17:50:19, sashab wrote:
> api_methods_for_class[classname] = property['api_methods']

done :)

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:43: # Stores a
list of classes with elements (index, classname, [propertyIDs, ..],
api_methods).
On 2017/02/01 at 17:50:19, sashab wrote:
> [api_methods, ...]

done :)

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_apis.py:59:
'api_methods': self.json5_file.parameters["api_methods"]["valid_values"],
On 2017/02/01 at 17:50:19, sashab wrote:
> Single quotes for strings

fixed :)

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl
(right):

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:27:
{% if api_method in api_class.api_methods %}
On 2017/02/01 at 17:50:20, sashab wrote:
> You can merge these:
> 
> {% for foo in bar if baz %}
> 
> Also be careful with your indenting -- see other tmpl files -- the indenting
needs to match C++, not Jinja2, so don't indent {%'s unless the C++ should be
indented :)

done :) that makes things look a bit nicer!

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:30:
nullptr,
On 2017/02/01 at 17:50:20, sashab wrote:
> One thing I'm worried about here is ordering. Since the descriptor is not
generated, we don't know the order these APIs will appear in, e.g if valid
values is [parseShorthand, parseSingleValue] all this code will fail.
> 
> This is OK for now because we can just add a comment to CSSProperties.json5,
but yet another reason to move to IDL. :)

ahhh I didn't even notice the ordering issue! that is a bit annoying

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2046: if
(cssPropertyDesc.parseSingleValue)
On 2017/02/01 at 17:50:20, sashab wrote:
> omg awesome

:D

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right):

https://codereview.chromium.org/2670433002/diff/30006/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:30: //  
CSSProperties.json5.
On 2017/02/01 at 17:50:20, sashab wrote:
> Can you add a comment to CSSProperties.json5 too, since we now have a weird
use for valid values, like:
> 
> "api_methods": {
>   // This list must be an ordered, complete list of methods whose names match
those defined in CSSPropertyDescriptor.h.
>   "valid_values"

done :)

[aazzam, 2017-02-02 02:55:49]

The CQ bit was checked by aazzam@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 02:56:14]

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-02-02 04:57:32]

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

[commit-bot: I haz the power, 2017-02-02 04:57:33]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[sashab, 2017-02-03 05:18:18]

lgtm

[aazzam, 2017-02-03 10:48:10]

The CQ bit was checked by aazzam@google.com

[aazzam, 2017-02-03 10:48:11]

The patchset sent to the CQ was uploaded after l-g-t-m from ktyliu@chromium.org,
gozzard@google.com
Link to the patchset: https://codereview.chromium.org/2670433002/#ps50001
(title: "Neatened up code")

[commit-bot: I haz the power, 2017-02-03 10:48:22]

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-02-03 13:35:34]

CQ is committing da patch.

Bot data: {"patchset_id": 50001, "attempt_start_ts": 1486118890601200,
"parent_rev": "a3c7cfd551d34f0edd4fc8c84d3aa172eba303c6", "commit_rev":
"d3b69783512361d59dade39d8e7a0812c8a2b32a"}

[commit-bot: I haz the power, 2017-02-03 13:36:20]

Description was changed from

==========
Makes descriptors hold nullptr for properties not implemented in API.

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. 

This patch modifies make_css_property_apis.py and
CSSPropertyDescriptor.cpp.tmpl so that when the descriptors for a
property are initialised, properties that are not implemented are
set to nullptr. This removes the need for temporaryCanReadValue in
the descriptor struct.

The template is also changed so that it does not need to be modified
when new methods are added to the API.

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

to

==========
Makes descriptors hold nullptr for properties not implemented in API.

A part of Project Ribbon, separating the parsing logic for CSS
properties from the parser into an API. 

This patch modifies make_css_property_apis.py and
CSSPropertyDescriptor.cpp.tmpl so that when the descriptors for a
property are initialised, properties that are not implemented are
set to nullptr. This removes the need for temporaryCanReadValue in
the descriptor struct.

The template is also changed so that it does not need to be modified
when new methods are added to the API.

BUG=668012

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

[commit-bot: I haz the power, 2017-02-03 13:36:22]

Committed patchset #4 (id:50001) as
https://chromium.googlesource.com/chromium/src/+/d3b69783512361d59dade39d8e7a...