[CSS Typed OM] Specify and check separators for repeated properties

[CSS Typed OM] Specify and check separators for repeated properties

Previously, we just assumed everything was space separated, which is
obviously wrong. This CL lets us specify a separator for a property in
CSSProperties.json5 (replacing the boolian is_repeated), which is used
in a new function CSSPropertyMetadata::repetitionSeparator(propertyID).

Diff in generated CSSPropertyMetadata.cpp: https://gist.github.com/wilddamon/e17ee686869b2cd33da6de769cec7103/revisions

BUG=545318
Review-Url: https://codereview.chromium.org/2722503002
Cr-Commit-Position: refs/heads/master@{#454102}
Committed: https://chromium.googlesource.com/chromium/src/+/89f793921f307ce8de9a1f0e4a25902e2a77140b

[meade_UTC10, 2017-02-27 08:05:08]

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

[rjwright, 2017-02-28 05:22:47]

On 2017/02/27 08:05:08, Eddy wrote:

LGTM

[meade_UTC10, 2017-02-28 22:12:32]

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

[meade_UTC10, 2017-02-28 23:02:41]

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

[meade_UTC10, 2017-02-28 23:02:41]

Hey Sasha,

Could you please sanity check the usage of the generators for this?

Thanks!
Eddy

[sashab, 2017-02-28 23:20:49]

Can you make a gist of the resultant 'CSSPropertyMetadata.cpp' and add it to the
CL description?  I get the feeling its going to have two functions in it that
are almost identical.

https://codereview.chromium.org/2722503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_css_property_metadata.py
(right):

https://codereview.chromium.org/2722503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_metadata.py:31:
('separator', 'propertyIsRepeated')
Not sure about this part. Do we need propertyIsRepeated anymore? Seems confusing
that the other generated methods from this have similar names to their key.

I'd either rename this to propertyHasSeparator, or just delete it altogether and
update callsites to use repetitionSeparator().

Alternatively, you could update the generator to support returning 'char' as
well as 'bool', and this method could be fully generated :)

[meade_UTC10, 2017-03-01 06:20:28]

Gist of generated code with diff:
https://gist.github.com/wilddamon/4413030b29264cefa344aaa7b3530abd/revisions

https://codereview.chromium.org/2722503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_css_property_metadata.py
(right):

https://codereview.chromium.org/2722503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_metadata.py:31:
('separator', 'propertyIsRepeated')
On 2017/02/28 23:20:47, sashab wrote:
> Not sure about this part. Do we need propertyIsRepeated anymore? Seems
confusing
> that the other generated methods from this have similar names to their key.
> 
> I'd either rename this to propertyHasSeparator, or just delete it altogether
and
> update callsites to use repetitionSeparator().
> 
> Alternatively, you could update the generator to support returning 'char' as
> well as 'bool', and this method could be fully generated :)

It's really nice to have both - there are lots of places that only check whether
the property is repeated, and it would be annoying to have to check the value
separator explicitly each time. However, I can add a non-generated function that
just calls the repetitionSeparator() function instead.

[meade_UTC10, 2017-03-01 06:27:34]

Updated to not generate propertyIsRepeated any more; there is still a function
with that name, but it calls repetitionSeparator and checks the return value.

Gist showing diff of generated code:
https://gist.github.com/wilddamon/e17ee686869b2cd33da6de769cec7103/revisions

[meade_UTC10, 2017-03-01 06:28:17]

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

[commit-bot: I haz the power, 2017-03-01 06:28:28]

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-03-01 07:49:32]

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

[commit-bot: I haz the power, 2017-03-01 07:49:32]

Dry run: This issue passed the CQ dry run.

[sashab, 2017-03-01 23:04:15]

Ahh much better!! I prefer this a lot :D
And thanks for the great gist link with diff, that was really smart - could you
add this to the CL description?

[sashab, 2017-03-01 23:04:24]

Oh and LGTM of course :)

[sashab, 2017-03-01 23:05:03]

+shend fyi for the gist link, and attitude towards generator code (see previous
comments)

[meade_UTC10, 2017-03-01 23:43:23]

Description was changed from

==========
[CSS Typed OM] Specify and check separators for repeated properties

Previously, we just assumed everything was space separated, which is
obviously wrong. This CL lets us specify a separator for a property in
CSSProperties.json5 (replacing the boolian is_repeated), which is used
in a new function CSSPropertyMetadata::repetitionSeparator(propertyID).

BUG=545318
==========

to

==========
[CSS Typed OM] Specify and check separators for repeated properties

Previously, we just assumed everything was space separated, which is
obviously wrong. This CL lets us specify a separator for a property in
CSSProperties.json5 (replacing the boolian is_repeated), which is used
in a new function CSSPropertyMetadata::repetitionSeparator(propertyID).

Diff in generated CSSPropertyMetadata.cpp:
https://gist.github.com/wilddamon/e17ee686869b2cd33da6de769cec7103/revisions

BUG=545318
==========

[meade_UTC10, 2017-03-01 23:43:47]

The CQ bit was checked by meade@chromium.org

[meade_UTC10, 2017-03-01 23:43:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjwright@chromium.org
Link to the patchset: https://codereview.chromium.org/2722503002/#ps40001
(title: "Don't generate function")

[commit-bot: I haz the power, 2017-03-01 23:44:20]

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

[meade_UTC10, 2017-03-01 23:45:53]

Thanks Sasha!

To get a diff like that in a gist, you make a gist with the original file, then
edit it to have your new file. Unfortunately it doesn't handle multiple edits so
well in the diffs, so if you make more changes, I prefer to make a new gist so
you can always see the diff between the original version and your latest
version. Ask me questions in person or via chat if you want more instructions!

[commit-bot: I haz the power, 2017-03-01 23:51:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488411827590290,
"parent_rev": "71ca55f2b59fe14c1a803f00c2c219502fd5d9bb", "commit_rev":
"89f793921f307ce8de9a1f0e4a25902e2a77140b"}

[commit-bot: I haz the power, 2017-03-01 23:52:08]

Description was changed from

==========
[CSS Typed OM] Specify and check separators for repeated properties

Previously, we just assumed everything was space separated, which is
obviously wrong. This CL lets us specify a separator for a property in
CSSProperties.json5 (replacing the boolian is_repeated), which is used
in a new function CSSPropertyMetadata::repetitionSeparator(propertyID).

Diff in generated CSSPropertyMetadata.cpp:
https://gist.github.com/wilddamon/e17ee686869b2cd33da6de769cec7103/revisions

BUG=545318
==========

to

==========
[CSS Typed OM] Specify and check separators for repeated properties

Previously, we just assumed everything was space separated, which is
obviously wrong. This CL lets us specify a separator for a property in
CSSProperties.json5 (replacing the boolian is_repeated), which is used
in a new function CSSPropertyMetadata::repetitionSeparator(propertyID).

Diff in generated CSSPropertyMetadata.cpp:
https://gist.github.com/wilddamon/e17ee686869b2cd33da6de769cec7103/revisions

BUG=545318

Review-Url: https://codereview.chromium.org/2722503002
Cr-Commit-Position: refs/heads/master@{#454102}
Committed:
https://chromium.googlesource.com/chromium/src/+/89f793921f307ce8de9a1f0e4a25...
==========

[commit-bot: I haz the power, 2017-03-01 23:52:09]

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