Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567473002:
Made a generator for CSSPropertyDescriptor.cpp (Closed)
DescriptionMade a generator for CSSPropertyDescriptor.cpp
Added a python file make_css_property_api.py and template file
CSSPropertyDescriptor.cpp.tmpl to generate CSSPropertyDescriptor.cpp.
This means that when we add new functions to our API, we only need to
modify the template. Also, when we add new CSS properties, they only
need to be added to the CSSProperties.in file.
BUG=668012
Committed: https://crrev.com/6f1e7b703e16eaf40a18ba63fb8abdcc1b1d1e9f
Cr-Commit-Position: refs/heads/master@{#438384}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Fixed style and renamed file #Patch Set 3 : removed accidental comment #
Total comments: 19
Patch Set 4 : Fixed comments #Patch Set 5 : rebasing #Patch Set 6 : changed method of grouping #Patch Set 7 : fixed bug #
Total comments: 11
Patch Set 8 : renamed padding to webkitpadding #Patch Set 9 : fixed some formatting #
Total comments: 6
Patch Set 10 : fixed some formatting #
Total comments: 2
Patch Set 11 : changed comments #Patch Set 12 : format #
Total comments: 17
Patch Set 13 : fixed some formatting #
Total comments: 2
Patch Set 14 : format #
Total comments: 1
Patch Set 15 : moved comment outside of loop #Dependent Patchsets: Messages
Total messages: 46 (19 generated)
aazzam@google.com changed reviewers: + sashab@chromium.org
Made a new CL with just the generator for CSSPropertyDescriptor.cpp without the .h files :)
Nice work Anna! Looking good :) https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/css_properties.py (right): https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/css_properties.py:31: 'custom_value': False, Typo in CL description: properties https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_css_property_api.py (right): https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:17: self._outputs = {"CSSPropertyDescriptor.cpp": self.generate_property_descriptor_h} Move this line and move newline to after it to be consistent with other make files https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:18: self._property_groups = [] Add comments explaining what these variables are for... What is a property group? I think there's a better name, like api_filenames or api_classes https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:23: self._property_groups.append(property['upper_camel_name']) It's not clear that the other branch is not False. Maybe add comments explaining what these branches are for, you had good comments before https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:24: else: I would change to elif property['..'] is not False or elif type(property) == 'str' or however you check types in python :) https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:28: self._property_groups = list(set(self._property_groups)) Add comment explaining this is to remove duplicates https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:29: for i, property in enumerate(self._properties_by_index): Add comment explaining what this loop is for https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:35: 'propertyGroups': sorted(self._property_groups), Check other files - I think they use underscore_naming Also don't call sorted here - if you want it to be sorted, call it in main() This function is potentially called many times and should be 0 cost https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:41: {% for property in properties %} Ahhhh this template file is so nice now ~^_^~ https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSProperties.in:29: // * When we want to generate the .h file for a property, we add this flag. Small nit: instead of saying "we", phrase stuff like following a recipe, e.g. * To add a property to the API, add this flag to the property Also we're not generating the .h files in this patch anymore :) https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSProperties.in:32: // a group, the value of that flag should be set to the group name - all of Remove double space before value Also explain that the name here has to be the name of the file containing the API class, as well as the name of that class The comment doesn't have to be long, it just has to be thorough :)
Description was changed from ========== Made a generator for CSSPropertyDescriptor.cpp Added a python file make_css_property_api.py and template file CSSPropertyDescriptor.cpp.tmpl to generate CSSPropertyDescriptor.cpp. This means that when we add new functions to our API, we only need to modify the template. Also, when we add new CSS proeprties, they only need to be added to the CSSProperties.in file. BUG=668012 ========== to ========== Made a generator for CSSPropertyDescriptor.cpp Added a python file make_css_property_api.py and template file CSSPropertyDescriptor.cpp.tmpl to generate CSSPropertyDescriptor.cpp. This means that when we add new functions to our API, we only need to modify the template. Also, when we add new CSS properties, they only need to be added to the CSSProperties.in file. BUG=668012 ==========
PTAL! :) https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/css_properties.py (right): https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/css_properties.py:31: 'custom_value': False, On 2016/12/08 at 23:56:35, sashab wrote: > Typo in CL description: properties done :) https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_css_property_api.py (right): https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:17: self._outputs = {"CSSPropertyDescriptor.cpp": self.generate_property_descriptor_h} On 2016/12/08 at 23:56:35, sashab wrote: > Move this line and move newline to after it to be consistent with other make files done https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:18: self._property_groups = [] On 2016/12/08 at 23:56:35, sashab wrote: > Add comments explaining what these variables are for... What is a property group? I think there's a better name, like api_filenames or api_classes done https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:23: self._property_groups.append(property['upper_camel_name']) On 2016/12/08 at 23:56:35, sashab wrote: > It's not clear that the other branch is not False. Maybe add comments explaining what these branches are for, you had good comments before done https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:24: else: On 2016/12/08 at 23:56:35, sashab wrote: > I would change to elif property['..'] is not False > or elif type(property) == 'str' or however you check types in python :) done :) https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:28: self._property_groups = list(set(self._property_groups)) On 2016/12/08 at 23:56:35, sashab wrote: > Add comment explaining this is to remove duplicates done https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:29: for i, property in enumerate(self._properties_by_index): On 2016/12/08 at 23:56:35, sashab wrote: > Add comment explaining what this loop is for done https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_css_property_api.py:35: 'propertyGroups': sorted(self._property_groups), On 2016/12/08 at 23:56:35, sashab wrote: > Check other files - I think they use underscore_naming > > Also don't call sorted here - if you want it to be sorted, call it in main() > > This function is potentially called many times and should be 0 cost done https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:41: {% for property in properties %} On 2016/12/08 at 23:56:35, sashab wrote: > Ahhhh this template file is so nice now ~^_^~ :D https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2567473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSProperties.in:29: // * When we want to generate the .h file for a property, we add this flag. On 2016/12/08 at 23:56:35, sashab wrote: > Small nit: instead of saying "we", phrase stuff like following a recipe, e.g. > > * To add a property to the API, add this flag to the property > > Also we're not generating the .h files in this patch anymore :) tried re-wording, ptal :)
This is so nice now!! well done anna :) Lg*m only because this is another pivotal patch and I'm reluctant to land it just yet. Let's see what another reviewer has to say first :). https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:41: # are stored and accessed in the cssPropertyDescriptors array in CSSPropertyDescriptors.cpp. Beautiful!! This whole file is awesome!
aazzam@google.com changed reviewers: + alancutter@chromium.org - sashab@chromium.org
Hi Alan, please take a look at this generator CL :)
https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:38: self._api_classnames = sorted(list(set(self._api_classnames))) No need for list(). https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:49: 'properties': self._properties_by_index, Use the same names between the script and the template file, the translation step reduces readability. https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:21: { nullptr, false } We should avoid using macros in code templates given that we've got a much nicer alternative (jinja). https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:32: // need to know which properties CSSPropertyAPI has been implemented for. Too much low level implementation detail and takes too long to answer the question "what is this parameter for?". How about: Specifies the existance (and optionally name) of a CSSPropertyAPI implementation for the property to be used by make_css_property_descriptor.py. See core/css/properties/CSSPropertyAPI.h for API details. https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:33: // * Add this flag if the API has been implemented for this property. Redundant with the third bullet point. https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:35: // value for this flag. An example of this is when properties have similar parsing Give explicit examples of the default and the override behaviour of this parameter. https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:37: // this flag inherit from a class with this name. No need for use case example. https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:38: // * Properties which have not been implemented do not get this flag. s/which have not been implemented/without a CSSPropertyAPI implementation/ https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:41: // 'not_implemented' flag instead. Awesome. https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:492: -webkit-padding-after direction_aware, api_class=WebkitPadding Let's make the name explicitly CSSPropertyAPIWebkitPadding for easier searchability and less magic generator behaviour for users to have to look up in order to use/read this.
PTAL :) https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:38: self._api_classnames = sorted(list(set(self._api_classnames))) On 2016/12/12 at 00:00:04, alancutter wrote: > No need for list(). done :) https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:49: 'properties': self._properties_by_index, On 2016/12/12 at 00:00:04, alancutter wrote: > Use the same names between the script and the template file, the translation step reduces readability. done :) https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:32: // need to know which properties CSSPropertyAPI has been implemented for. On 2016/12/12 at 00:00:07, alancutter wrote: > Too much low level implementation detail and takes too long to answer the question "what is this parameter for?". > > How about: > Specifies the existance (and optionally name) of a CSSPropertyAPI implementation for the property to be used by make_css_property_descriptor.py. > See core/css/properties/CSSPropertyAPI.h for API details. done :) https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:33: // * Add this flag if the API has been implemented for this property. On 2016/12/12 at 00:00:07, alancutter wrote: > Redundant with the third bullet point. done :) https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:35: // value for this flag. An example of this is when properties have similar parsing On 2016/12/12 at 00:00:07, alancutter wrote: > Give explicit examples of the default and the override behaviour of this parameter. done :) https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:37: // this flag inherit from a class with this name. On 2016/12/12 at 00:00:04, alancutter wrote: > No need for use case example. done :) https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:38: // * Properties which have not been implemented do not get this flag. On 2016/12/12 at 00:00:04, alancutter wrote: > s/which have not been implemented/without a CSSPropertyAPI implementation/ done :) https://codereview.chromium.org/2567473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:492: -webkit-padding-after direction_aware, api_class=WebkitPadding On 2016/12/12 at 00:00:04, alancutter wrote: > Let's make the name explicitly CSSPropertyAPIWebkitPadding for easier searchability and less magic generator behaviour for users to have to look up in order to use/read this. The reason I haven't done it this way is that in some cases in the tmpl file I need an ID name e.g. "CSSPropertyWebkitPadding", and in others I need a class name e.g. "CSSPropertyAPIWebkitPadding". Do you think it's okay to keep it this way, or do you have an alternative suggestion? :)
PTAL alan :)
https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:24: if property['api_class'] is not None: The indent level is getting a bit much here. Invert this to if is None: continue. https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:29: elif isinstance(property['api_class'], str): Assert this condition, the else: case should never be reached. https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:31: classnames[property['api_class']].append("CSSProperty" + property['upper_camel_name']) I think something like classname = get_classname(api_class) properties_for_class[classname].append(property_id) would be a less heavy to read. Also use ' over ". https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:36: api_class = namedtuple('api_class', ('index', 'classname', 'propertyIDs')) Use CamelCase for types. I don't think a namedtuple is necessary anyway. self._api_classes.append({ 'index': i + 1, ... }) would be perfectly fine. https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:38: self._api_classes.append(api_class(index=i + 1, classname=classname, propertyIDs=classnames[classname])) Use snake_case for propertyIDs. https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:21: { nullptr, false } Again I don't think we should use macros in jinja templates.
PTAL :) https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:24: if property['api_class'] is not None: On 2016/12/13 at 03:29:54, alancutter wrote: > The indent level is getting a bit much here. Invert this to if is None: continue. done :) https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:29: elif isinstance(property['api_class'], str): On 2016/12/13 at 03:29:54, alancutter wrote: > Assert this condition, the else: case should never be reached. done :) https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:36: api_class = namedtuple('api_class', ('index', 'classname', 'propertyIDs')) On 2016/12/13 at 03:29:54, alancutter wrote: > Use CamelCase for types. > > I don't think a namedtuple is necessary anyway. > self._api_classes.append({ > 'index': i + 1, > ... > }) > would be perfectly fine. The reason we thought of using a namedtuple instead of a hash map since it's immutable (which is better in this case, since we aren't changing it). Also, the fields which we have in each classname are better defined - if we have it as a hashmap you can access any key from it, it'll just have an error. And from what I've seen (and what sasha has told me) there's nowhere in our jinja templates which access a key of a hashmap from within the .tmpl file. Does that make sense? :) https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:38: self._api_classes.append(api_class(index=i + 1, classname=classname, propertyIDs=classnames[classname])) On 2016/12/13 at 03:29:54, alancutter wrote: > Use snake_case for propertyIDs. done! :) https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:21: { nullptr, false } On 2016/12/13 at 03:29:54, alancutter wrote: > Again I don't think we should use macros in jinja templates. done :)
lgtm after comments addressed. https://codereview.chromium.org/2567473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:27: properties_for_class[classname].append('CSSProperty' + property['upper_camel_name']) Use property['property_id'] instead of constructing this string, here and elsewhere. https://codereview.chromium.org/2567473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:41: else: No need for else after return. https://codereview.chromium.org/2567473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:17: { nullptr, false }, This should probably have a comment explaining what it is now.
https://codereview.chromium.org/2567473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:27: properties_for_class[classname].append('CSSProperty' + property['upper_camel_name']) On 2016/12/13 at 05:55:34, alancutter wrote: > Use property['property_id'] instead of constructing this string, here and elsewhere. done :) https://codereview.chromium.org/2567473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:41: else: On 2016/12/13 at 05:55:34, alancutter wrote: > No need for else after return. done :) https://codereview.chromium.org/2567473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:17: { nullptr, false }, On 2016/12/13 at 05:55:34, alancutter wrote: > This should probably have a comment explaining what it is now. done :)
aazzam@google.com changed reviewers: + gozzard@google.com - alancutter@chromium.org
please take a look Gozz!
alancutter@chromium.org changed reviewers: + alancutter@chromium.org
https://codereview.chromium.org/2567473002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:24: // CSSPropertyAPI, also add them to the struct initaliser below. These two comments need to be reworded for their new context. They sound very strange now.
lgtm
https://codereview.chromium.org/2567473002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:24: // CSSPropertyAPI, also add them to the struct initaliser below. On 2016/12/13 at 22:35:22, alancutter wrote: > These two comments need to be reworded for their new context. They sound very strange now. I changed them a bit, does it sound better now?
aazzam@google.com changed reviewers: + sashab@chromium.org - gozzard@google.com
please take a look sasha! :)
sashab@chromium.org changed reviewers: + gozzard@google.com
Well done Anna, this is looking awesome! https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:11: from collections import namedtuple, defaultdict Check other .py files -- I thnik you might want a newline before this line to separate system imports from local imports. Also maybe move it to after import sys https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:18: 'CSSPropertyDescriptor.cpp': self.generate_property_descriptor_h, Rename to generate_property_descriptor_cpp https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:21: # Temporary map of classname to list of propertyIDs. What class name? :) Maybe: # Temporary map of API classname to list of propertyIDs that the API class is for. https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:22: properties_for_class = defaultdict(list) Nice variable name https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:37: def get_classname(self, property): Nice!! This doesn't need to be a class method -- can make it a function at the top of the file that just takes a property. https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:42: assert isinstance(property['api_class'], str) Assert is great! Maybe add an error message: assert ..., "api_class value for " + property['api_class'] + " should be None or a string" https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:23: // are added to CSSPropertyAPI, also add them to the struct initaliser below. These comments - \o/ https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:40: {% endfor %} Careful with your indenting here -- it's not ideal, but maybe it could look like: switch (id) { {% for api_class in api_classes %} {% for property_id in api_class.property_ids %} case {{property_id}}: {% endfor %} return cssPropertyDescriptors[{{api_class.index}}]; {% endfor %} So it's a bit clearer where the loops are. https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSProperties.in:27: // Specifies the existance (and optionally name) of a CSSPropertyAPI implementation for the property to be used by make_css_property_descriptor.py. Wrap to 80 (check?) characters Copy rest of file https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSProperties.in:483: -webkit-padding-end direction_aware, api_class=CSSPropertyAPIWebkitPadding This is so much better!! Yay!
aazzam@google.com changed reviewers: - gozzard@google.com
PTAL :) https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:11: from collections import namedtuple, defaultdict On 2016/12/13 at 23:10:38, sashab wrote: > Check other .py files -- I thnik you might want a newline before this line to separate system imports from local imports. Also maybe move it to after import sys done :) https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:18: 'CSSPropertyDescriptor.cpp': self.generate_property_descriptor_h, On 2016/12/13 at 23:10:38, sashab wrote: > Rename to generate_property_descriptor_cpp done :) https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:21: # Temporary map of classname to list of propertyIDs. On 2016/12/13 at 23:10:38, sashab wrote: > What class name? :) Maybe: > > # Temporary map of API classname to list of propertyIDs that the API class is for. done :) https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:37: def get_classname(self, property): On 2016/12/13 at 23:10:38, sashab wrote: > Nice!! > > This doesn't need to be a class method -- can make it a function at the top of the file that just takes a property. done :) https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:42: assert isinstance(property['api_class'], str) On 2016/12/13 at 23:10:38, sashab wrote: > Assert is great! Maybe add an error message: > > assert ..., "api_class value for " + property['api_class'] + " should be None or a string" done https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:40: {% endfor %} On 2016/12/13 at 23:10:38, sashab wrote: > Careful with your indenting here -- it's not ideal, but maybe it could look like: > > switch (id) { > {% for api_class in api_classes %} > {% for property_id in api_class.property_ids %} > case {{property_id}}: > {% endfor %} > return cssPropertyDescriptors[{{api_class.index}}]; > {% endfor %} > > > So it's a bit clearer where the loops are. done :) https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2567473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSProperties.in:27: // Specifies the existance (and optionally name) of a CSSPropertyAPI implementation for the property to be used by make_css_property_descriptor.py. On 2016/12/13 at 23:10:38, sashab wrote: > Wrap to 80 (check?) characters > > Copy rest of file done :)
sashab@chromium.org changed reviewers: + gozzard@google.com
Yay this is so awesome! Nice work anna. LGTM https://codereview.chromium.org/2567473002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:46: self._api_classes.append(ApiClass(index=i + 1, classname=classname, property_ids=properties_for_class[classname])) You can reformat this to be a little easier to read: self._api_classes.append(ApiClass( index=i + 1, classname=classname, property_ids=properties_for_class[classname] ))
The CQ bit was checked by aazzam@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from alancutter@chromium.org, gozzard@google.com, sashab@chromium.org Link to the patchset: https://codereview.chromium.org/2567473002/#ps260001 (title: "format")
aazzam@google.com changed reviewers: - alancutter@chromium.org, gozzard@google.com
https://codereview.chromium.org/2567473002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py (right): https://codereview.chromium.org/2567473002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_descriptor.py:46: self._api_classes.append(ApiClass(index=i + 1, classname=classname, property_ids=properties_for_class[classname])) On 2016/12/13 at 23:32:32, sashab wrote: > You can reformat this to be a little easier to read: > > self._api_classes.append(ApiClass( > index=i + 1, > classname=classname, > property_ids=properties_for_class[classname] > )) looks much nicer :)
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
alancutter@chromium.org changed reviewers: + alancutter@chromium.org, gozzard@google.com
https://codereview.chromium.org/2567473002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl (right): https://codereview.chromium.org/2567473002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/CSSPropertyDescriptor.cpp.tmpl:23: // are added to CSSPropertyAPI, also add them to the struct initaliser below. I don't think you want this comment appearing for every single property descriptor. Also I would drop the first sentence. I think prose versions of code in comments should be avoided, they run the risk of getting out of date (comments aren't tested and therefore are doomed to regress over time) and instead of explaining "why" they only say "what" so they bring little value to the code base.
The CQ bit was unchecked by aazzam@google.com
The CQ bit was checked by aazzam@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, gozzard@google.com, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2567473002/#ps280001 (title: "moved comment outside of loop")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1481673143224040,
"parent_rev": "d8bbb8c1b2f28aa2297e023648227739ac0797b5", "commit_rev":
"315eb08cb674714179a4a0a18243cc3f90eb0b44"}
Message was sent while issue was closed.
Description was changed from ========== Made a generator for CSSPropertyDescriptor.cpp Added a python file make_css_property_api.py and template file CSSPropertyDescriptor.cpp.tmpl to generate CSSPropertyDescriptor.cpp. This means that when we add new functions to our API, we only need to modify the template. Also, when we add new CSS properties, they only need to be added to the CSSProperties.in file. BUG=668012 ========== to ========== Made a generator for CSSPropertyDescriptor.cpp Added a python file make_css_property_api.py and template file CSSPropertyDescriptor.cpp.tmpl to generate CSSPropertyDescriptor.cpp. This means that when we add new functions to our API, we only need to modify the template. Also, when we add new CSS properties, they only need to be added to the CSSProperties.in file. BUG=668012 Review-Url: https://codereview.chromium.org/2567473002 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Made a generator for CSSPropertyDescriptor.cpp Added a python file make_css_property_api.py and template file CSSPropertyDescriptor.cpp.tmpl to generate CSSPropertyDescriptor.cpp. This means that when we add new functions to our API, we only need to modify the template. Also, when we add new CSS properties, they only need to be added to the CSSProperties.in file. BUG=668012 Review-Url: https://codereview.chromium.org/2567473002 ========== to ========== Made a generator for CSSPropertyDescriptor.cpp Added a python file make_css_property_api.py and template file CSSPropertyDescriptor.cpp.tmpl to generate CSSPropertyDescriptor.cpp. This means that when we add new functions to our API, we only need to modify the template. Also, when we add new CSS properties, they only need to be added to the CSSProperties.in file. BUG=668012 Committed: https://crrev.com/6f1e7b703e16eaf40a18ba63fb8abdcc1b1d1e9f Cr-Commit-Position: refs/heads/master@{#438384} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/6f1e7b703e16eaf40a18ba63fb8abdcc1b1d1e9f Cr-Commit-Position: refs/heads/master@{#438384} |
