Merge .in files for css/svg properties into a single file

Merge .in files for css/svg properties into a single file

This patch merges the various files for specifying css and svg
properties into a single location, CSSProperties.in. These files are
CSSPropertyNames.in, SVGCSSPropertyNames.in, CSSProperties.in and
CSSShorthands.in. I've added documentation at the top of the merged .in
file on what the various arguments mean.

As a result, adding new properties is now slightly easier. Adding new
code generators for properties is also much easier, for example a
generator to replace the functions isInheritedProperty/etc. should be
much easier to add now.

[Timothy Loh, 2014-07-04 04:59:53]

(+nbarth since I'm touching a lot of python)

To save me some future rebasing hassle, this patch is written on top of
https://codereview.chromium.org/350333003.

[Nils Barth (inactive), 2014-07-07 16:43:29]

Python style/structure comments only; mostly nits.

Most substantive point is that dict comprehensions need to be replaced by
dict(...), since they're a 2.7 feature.
Also str.partition.

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/css...
File Source/build/scripts/css_properties.py (right):

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/css...
Source/build/scripts/css_properties.py:15: 'font': False,
alpha?

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/css...
Source/build/scripts/css_properties.py:32: 'svg': [True, False],
list -> tuple?

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/css...
Source/build/scripts/css_properties.py:47: self._aliases = {property['name']:
property['alias_for'] for property in properties if property['alias_for']}
line break?

This needs to be dict((property['name'], property['alias_for']) ...)
unless we've migrated to Python 2.7.
(Last I recall we were using Python 2.6 on Mac + Windows, and dict
comprehensions don't work on Python 2.6.)

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/css...
Source/build/scripts/css_properties.py:48: properties = [property for property
in properties if not property['alias_for']]
line break?

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/css...
Source/build/scripts/css_properties.py:54: property['property_id'] =
self._css_name_to_enum(property['name'])
Would this list be clearer alphabetically?
(other than upper/lower)

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/css...
Source/build/scripts/css_properties.py:67: @staticmethod
Any reason to use static methods instead of just functions?

(Generally just a function is simpler; only compelling reason for static methods
are if you're inheriting and over-riding, which I don't think you're doing
here.)

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/css...
Source/build/scripts/css_properties.py:77: return "CSSProperty" +
CSSProperties._camelcase_css_name(css_name)
Single quotes.

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
File Source/build/scripts/make_css_property_names.py (right):

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_css_property_names.py:7: import license
alpha

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_css_property_names.py:181: class_name =
"CSSPropertyNames"
Could you single-quotify, while you're here?

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_css_property_names.py:185: self._outputs =
{(self.class_name + ".h"): self.generate_header,
ditto

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_css_property_names.py:190: return "    %(property_id)s
= %(enum_value)s," % property
ditto

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_css_property_names.py:194: 'license':
license.license_for_generated_cpp(),
alpha while here

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_css_property_names.py:196: 'property_enums':
"\n".join(map(self._enum_declaration, self._properties_list)),
quote

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_css_property_names.py:208: current_offset +=
len(property["name"]) + 1
quote

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_css_property_names.py:210: css_name_and_enum_pairs =
[(property['name'], property_id) for property_id, property in
self._properties.items()]
line breaks?

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_css_property_names.py:217: 'property_name_strings':
'\n'.join(map(lambda property: '    "%(name)s\\0"' % property,
self._properties_list)),
generator expressions instead of map+lamba

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_css_property_names.py:230: if __name__ == "__main__":
quote

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
File Source/build/scripts/make_style_shorthands.py (right):

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_style_shorthands.py:50: self._properties =
{property_id: property for property_id, property in self._properties.items() if
property['longhands']}
dict()

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_style_shorthands.py:53:
property['camel_case_longhands'] = map(self._css_name_to_enum,
property['longhands'].split(';'))
list comp

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_style_shorthands.py:55:
property['runtime_enabled_shorthand'] =
lower_first(property['runtime_enabled_shorthand'])
FWIW, if special-cased None in lower_first, could skip this check.

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/mak...
Source/build/scripts/make_style_shorthands.py:63: 'longhands_dictionary':
self._longhand_dictionary,
alpha?

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/tem...
File Source/build/scripts/templates/StylePropertyShorthand.cpp.tmpl (right):

https://codereview.chromium.org/371443003/diff/60002/Source/build/scripts/tem...
Source/build/scripts/templates/StylePropertyShorthand.cpp.tmpl:50: {% if
property.lower_camel_name != "marker" %}
The |if| can be merged into the |for| statement, right?

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

https://codereview.chromium.org/371443003/diff/60002/Source/devtools/scripts/...
Source/devtools/scripts/generate_supported_css.py:55: name = line.split(" ")[0]
str.partition is a bit better:
name = line.partition(" ")[0]
https://docs.python.org/2/library/stdtypes.html#str.partition

https://codereview.chromium.org/371443003/diff/60002/Source/devtools/scripts/...
Source/devtools/scripts/generate_supported_css.py:57: if "longhands=" in line:
longhands_value = line.partition("longhands=")[2]
if longhands_value:
    ...

[eseidel, 2014-07-07 17:12:45]

This change is clearly more than just merging two files.  Can you either update
the description to better explain all of what you are trying to do, or better
yet break this down into smaller changes?

[esprehn, 2014-07-07 17:19:47]

On 2014/07/07 at 17:12:45, eseidel wrote:
> This change is clearly more than just merging two files.  Can you either
update the description to better explain all of what you are trying to do, or
better yet break this down into smaller changes?

Yeah, this change seems really big. Can we do this in parts?

[Timothy Loh, 2014-07-08 00:47:35]

On 2014/07/07 17:19:47, esprehn wrote:
> On 2014/07/07 at 17:12:45, eseidel wrote:
> > This change is clearly more than just merging two files.  Can you either
> update the description to better explain all of what you are trying to do, or
> better yet break this down into smaller changes?
> 
> Yeah, this change seems really big. Can we do this in parts?

Sorry! I didn't really expect this to encompass so many little things when I
started it. I should definitely be able to split parts of this out, although a
lot of the change is just making the three scripts use the same variable names
from the base class.

The change here breaks down roughly into:
1) Tweak StyleBuilder flags
1.1) Rename some flags to sb_
1.2) Remove unneeded arguments (type_name mostly)
1.3) Remove unused use_handlers_for
2) Merge .in files and sort
2.1) Add base class for the .in files
2.2) Update gn/gyp files
2.3) Update the two other places which parse these files...
2.4) Small changes to flags, mostly camel_case_name -> lower_camel_name
3) Remove width/height as shorthands from shorthands .in file (used in viewport
descriptor parsing) and handle in CSSPropertyParser.cpp

So what I'll do is
- merge 1.3 into https://codereview.chromium.org/350333003/, since that removes
usage of it
- patch for 1.1/1.2
- patch for 3
- patch to remove runtime-enabled-shorthand (and special case
text-decoration-line in .cpp.tmpl); should simplify this slightly
- patch for 2.4?
and we'll see how it looks after those.

[eseidel, 2014-07-08 00:59:49]

Wonderful.  My l-g-t-ming finger is standing ready to r+ any and all small
patches!

[Timothy Loh, 2014-07-24 07:28:55]

On 2014/07/08 00:47:35, Timothy Loh wrote:
> On 2014/07/07 17:19:47, esprehn wrote:
> > On 2014/07/07 at 17:12:45, eseidel wrote:
> > > This change is clearly more than just merging two files.  Can you either
> > update the description to better explain all of what you are trying to do,
or
> > better yet break this down into smaller changes?
> > 
> > Yeah, this change seems really big. Can we do this in parts?
> 
> Sorry! I didn't really expect this to encompass so many little things when I
> started it. I should definitely be able to split parts of this out, although a
> lot of the change is just making the three scripts use the same variable names
> from the base class.
> 
> The change here breaks down roughly into:
> 1) Tweak StyleBuilder flags
> 1.1) Rename some flags to sb_
> 1.2) Remove unneeded arguments (type_name mostly)
> 1.3) Remove unused use_handlers_for
> 2) Merge .in files and sort
> 2.1) Add base class for the .in files
> 2.2) Update gn/gyp files
> 2.3) Update the two other places which parse these files...
> 2.4) Small changes to flags, mostly camel_case_name -> lower_camel_name
> 3) Remove width/height as shorthands from shorthands .in file (used in
viewport
> descriptor parsing) and handle in CSSPropertyParser.cpp
> 
> So what I'll do is
> - merge 1.3 into https://codereview.chromium.org/350333003/, since that
removes
> usage of it
> - patch for 1.1/1.2
> - patch for 3
> - patch to remove runtime-enabled-shorthand (and special case
> text-decoration-line in .cpp.tmpl); should simplify this slightly
> - patch for 2.4?
> and we'll see how it looks after those.

OK so I've landed patches for the small side things which I originally had in
the patch, so the change now is basically just touching python/.in/gyp/gn files,
and minor changes to .tmpl files due to variable renames. I've further split
this into two patches (merge CSSShorthands.in into CSSProperties.in, merge
(SVG)CSSPropertyNames.in into CSSProperties.in), which maybe makes this a bit
easier to review; https://codereview.chromium.org/410953002/
https://codereview.chromium.org/415613002/

[Timothy Loh, 2014-07-24 07:33:57]

On 2014/07/07 16:43:29, Nils Barth wrote:
> Python style/structure comments only; mostly nits.
> 
> Most substantive point is that dict comprehensions need to be replaced by
> dict(...), since they're a 2.7 feature.
> Also str.partition.

I've done some of the suggested changes here, although I left out tweaking style
in nearby code to keep the size of this down. I also left in the 80+ col lines,
which seems OK as per the Blink style guide (also it makes no comment on
single/double quotes)?

The change is now broken into two changes (see above comment) and I haven't
added you to the reviewers list for these since you're no longer working on
Chrome, but feel free to leave comments if you want to.

[Nils Barth (inactive), 2014-07-24 14:15:56]

On 2014/07/24 07:33:57, Timothy Loh wrote:
> I've done some of the suggested changes here, although I left out tweaking
style
> in nearby code to keep the size of this down.

No problem, that's understandable.

> I also left in the 80+ col lines, which seems OK as per the Blink style guide

n/p: 80+ col lines are ok (not banned), but if there's a convenient line break
opportunity it makes it a bit more legible (in Rietveld) esp., hence why I
requested.

> (also it makes no comment on single/double quotes)?

Neither's preferred, though consistency within a file is specified:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=String...
"""
Be consistent with your choice of string quote character within a file. Pick '
or " and stick with it. It is okay to use the other quote character on a string
to avoid the need to \ escape within the string. GPyLint enforces this.
...
Prefer """ for multi-line strings rather than '''.
"""

(Personally I like ' to avoid a shift, but I understand " esp. as consistency
with other languages.)

> The change is now broken into two changes (see above comment) and I haven't
> added you to the reviewers list for these since you're no longer working on
> Chrome, but feel free to leave comments if you want to.

Thanks Tim!
(I doubt I'd have further comments regardless ;)