Merge CSSProperties.in and CSSShorthands.in

Merge CSSProperties.in and CSSShorthands.in

This patch is part 1 of merging the CSS .in files (part 2 is also up, at
https://codereview.chromium.org/415613002). Having a single .in file for
listing CSS properties makes it both easier to add new properties and
add new property flags.

I've renamed some of the StyleBuilder flags to make it more obvious what
these refer to and also re-ordered CSSProperties.in in some sort of
sensible fashion (see part 2 patch for what this will look like with the
remaining properties); the high/low priority split doesn't make too much
sense right now but will of course make sense once CSSPropertyNames.in
is merged into the same file.

BUG=396992
TBR=pfeldman

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180020

[Timothy Loh, 2014-07-24 07:46:51]

Part 1 of merging CSS .in files (split off from
https://codereview.chromium.org/371443003/). I've written this on top of
codereview.chromium.org/350333003 (cascade declared property values), which
makes some changes to CSSProperties.in and CSSPropertyNames.in.

[lushnikov, 2014-07-24 08:08:35]

https://codereview.chromium.org/410953002/diff/20001/Source/devtools/scripts/...
File Source/devtools/scripts/generate_supported_css.py (right):

https://codereview.chromium.org/410953002/diff/20001/Source/devtools/scripts/...
Source/devtools/scripts/generate_supported_css.py:65: longhands =
line.partition("longhands=")[2]
I've just found the following in CSSProperties.in:

flood-color svg, sb_converter=convertSVGColor

Which implies that longhands are not always the third part of the string.

[Timothy Loh, 2014-07-24 08:27:43]

https://codereview.chromium.org/410953002/diff/20001/Source/devtools/scripts/...
File Source/devtools/scripts/generate_supported_css.py (right):

https://codereview.chromium.org/410953002/diff/20001/Source/devtools/scripts/...
Source/devtools/scripts/generate_supported_css.py:65: longhands =
line.partition("longhands=")[2]
On 2014/07/24 08:08:35, lushnikov wrote:
> I've just found the following in CSSProperties.in:
> 
> flood-color svg, sb_converter=convertSVGColor
> 
> Which implies that longhands are not always the third part of the string.

The way str.partition works means that the |longhands| variable will be empty
here if "longhands=" isn't present in the line.

[lushnikov, 2014-07-24 09:26:30]

https://codereview.chromium.org/410953002/diff/20001/Source/devtools/scripts/...
File Source/devtools/scripts/generate_supported_css.py (right):

https://codereview.chromium.org/410953002/diff/20001/Source/devtools/scripts/...
Source/devtools/scripts/generate_supported_css.py:65: longhands =
line.partition("longhands=")[2]
My bad.
One more thing: this relies on "longhands" being the last property argument in
CSSProperties.in, however this condition is not explicitly stated in
CSSProperties.h

Should we better use line.partition("longhands=")[2].partition(", ")[0] to be on
the safe side here?

[Timothy Loh, 2014-07-25 03:36:48]

https://codereview.chromium.org/410953002/diff/20001/Source/devtools/scripts/...
File Source/devtools/scripts/generate_supported_css.py (right):

https://codereview.chromium.org/410953002/diff/20001/Source/devtools/scripts/...
Source/devtools/scripts/generate_supported_css.py:65: longhands =
line.partition("longhands=")[2]
On 2014/07/24 09:26:30, lushnikov wrote:
> My bad.
> One more thing: this relies on "longhands" being the last property argument in
> CSSProperties.in, however this condition is not explicitly stated in
> CSSProperties.h
> 
> Should we better use line.partition("longhands=")[2].partition(", ")[0] to be
on
> the safe side here?

Sure, done. FYI the followup patch https://codereview.chromium.org/415613002/
basically rewrites the file (since it will then only need to read one file) if
you haven't seen it.

[Timothy Loh, 2014-07-25 03:44:37]

+alancutter could you take a look at this and the followup
https://codereview.chromium.org/415613002 ? Thanks :)

[lushnikov, 2014-07-25 11:34:59]

On 2014/07/25 03:36:48, Timothy Loh wrote:
>
https://codereview.chromium.org/410953002/diff/20001/Source/devtools/scripts/...
> File Source/devtools/scripts/generate_supported_css.py (right):
> 
>
https://codereview.chromium.org/410953002/diff/20001/Source/devtools/scripts/...
> Source/devtools/scripts/generate_supported_css.py:65: longhands =
> line.partition("longhands=")[2]
> On 2014/07/24 09:26:30, lushnikov wrote:
> > My bad.
> > One more thing: this relies on "longhands" being the last property argument
in
> > CSSProperties.in, however this condition is not explicitly stated in
> > CSSProperties.h
> > 
> > Should we better use line.partition("longhands=")[2].partition(", ")[0] to
be
> on
> > the safe side here?
> 
> Sure, done. FYI the followup patch https://codereview.chromium.org/415613002/
> basically rewrites the file (since it will then only need to read one file) if
> you haven't seen it.

Thanks, devtools lgtm

[apavlov, 2014-07-25 12:13:23]

Nice!

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:12: // sufficient, we should prefer to use
sb_converter, and failing that defining
Sorry if I'm wrong, but it looks like "defining" should be "define" (to agree
with "prefer")

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:23: // - name_for_methods=BorderWidth
It would be a lot easier to understand the implications of the name choice if
BorderWidth were e.g. FooBar, and all bullet points below also used FooBar
(like, "initialFooBar" etc.)

https://codereview.chromium.org/410953002/diff/40001/Source/devtools/scripts/...
File Source/devtools/scripts/generate_supported_css.py (right):

https://codereview.chromium.org/410953002/diff/40001/Source/devtools/scripts/...
Source/devtools/scripts/generate_supported_css.py:64: shorthand =
line.partition(" ")[0]
could we put this line under "if longhands:" to avoid necessary computations?

[apavlov, 2014-07-25 12:14:16]

+pfeldman for devtools changes

[alancutter (OOO until 2018), 2014-07-28 01:09:10]

lgtm with nits, looking forward to this merge.

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:10: // The remaining properties are used for
the StyleBuilder and allow us to
s/properties/arguments/ otherwise it gets confusing with CSS properties.

Why are some of these arguments prefixed with sb_ and others not? This comment
suggests all the following arguments relate to the StyleBuilder.
Given this comment section I don't think it's necessary to distinguish using a
prefix.

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:37: // Call the given function on
StyleBuilderConverter to get a value to set
"Specify a function on StyleBuilderConverter to be used by the StyleBuilder for
converting a CSSValue to an appropriate platform value."

[Timothy Loh, 2014-07-28 02:54:19]

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:10: // The remaining properties are used for
the StyleBuilder and allow us to
On 2014/07/28 01:09:09, alancutter wrote:
> s/properties/arguments/ otherwise it gets confusing with CSS properties.
> 
> Why are some of these arguments prefixed with sb_ and others not? This comment
> suggests all the following arguments relate to the StyleBuilder.
> Given this comment section I don't think it's necessary to distinguish using a
> prefix.

As per in person discussion, sb_ removed from everything except skip/unreachable
(patch2), which will become builder_skip, builder_unreachable.

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:12: // sufficient, we should prefer to use
sb_converter, and failing that defining
On 2014/07/25 12:13:23, apavlov wrote:
> Sorry if I'm wrong, but it looks like "defining" should be "define" (to agree
> with "prefer")

Done.

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:23: // - name_for_methods=BorderWidth
On 2014/07/25 12:13:23, apavlov wrote:
> It would be a lot easier to understand the implications of the name choice if
> BorderWidth were e.g. FooBar, and all bullet points below also used FooBar
> (like, "initialFooBar" etc.)

I wanted the examples below to use actual properties so it would be clearer how
we went from border-bottom-left to borderBottomLeft or setBorderBottomLeft. I've
made this example a bit more consistent though, so have a look.

https://codereview.chromium.org/410953002/diff/40001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:37: // Call the given function on
StyleBuilderConverter to get a value to set
On 2014/07/28 01:09:09, alancutter wrote:
> "Specify a function on StyleBuilderConverter to be used by the StyleBuilder
for
> converting a CSSValue to an appropriate platform value."

Rewrote this comment, have a look.

https://codereview.chromium.org/410953002/diff/40001/Source/devtools/scripts/...
File Source/devtools/scripts/generate_supported_css.py (right):

https://codereview.chromium.org/410953002/diff/40001/Source/devtools/scripts/...
Source/devtools/scripts/generate_supported_css.py:64: shorthand =
line.partition(" ")[0]
On 2014/07/25 12:13:23, apavlov wrote:
> could we put this line under "if longhands:" to avoid necessary computations?

Probably, but this file is rewritten in the next patch so I won't bother here.

[alancutter (OOO until 2018), 2014-08-11 03:55:56]

lgtm after rebase.

[Timothy Loh, 2014-08-11 03:56:30]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2014-08-11 03:56:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/timloh@chromium.org/410953002/120001

[commit-bot: I haz the power, 2014-08-11 04:53:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[commit-bot: I haz the power, 2014-08-11 04:55:44]

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

[commit-bot: I haz the power, 2014-08-11 04:55:46]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[Timothy Loh, 2014-08-12 01:43:41]

On 2014/08/11 04:55:46, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

TBRing pfeldman for devtools OWNERS, lushnikov has provided devtools lgtm.

[Timothy Loh, 2014-08-12 01:44:04]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2014-08-12 01:44:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/timloh@chromium.org/410953002/120001

[commit-bot: I haz the power, 2014-08-12 01:49:02]

Change committed as 180020