Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2826653002:
Generate getters/setters for some fields on groups in ComputedStyle (Closed)
DescriptionGenerate getters/setters for some fields on groups in ComputedStyle
This patch generates getters/setters as needed for monotic_flags,
external and storage_only fields on groups in ComputedStyle. In a
following patch we will use this to access members of groups through
these getters/setters instead of directly as <group_name>.<field_name>.
Generated diff here: https://gist.github.com/nainar/bf6ca12c12ff7283d90ebed7640cf7be/revisions
Please note these functions are unused for now.
BUG=710938
Review-Url: https://codereview.chromium.org/2826653002
Cr-Commit-Position: refs/heads/master@{#465919}
Committed: https://chromium.googlesource.com/chromium/src/+/7185f26bb323f0650f8831f6e2d2c976fd373e55
Patch Set 1 #Patch Set 2 : Adds protected methods for all fields that need it #
Total comments: 8
Patch Set 3 : Append Internal to all protected methods #
Total comments: 4
Patch Set 4 : Incorporating shend@'s suggestions #
Total comments: 22
Patch Set 5 : Add new macro argument_type to be used in setter macros #
Total comments: 10
Patch Set 6 : suzyh@'s comments #
Total comments: 2
Patch Set 7 : Clean up nits prelanding #Dependent Patchsets: Messages
Total messages: 51 (36 generated)
Description was changed from ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. BUG=710938 ========== to ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/30d5e31afab3e0e4e35046fca420a4d3/revisions BUG=710938 ==========
nainar@chromium.org changed reviewers: + shend@chromium.org
Darren, PTAL? More specifically curious about the spurious new lines. Thanks!
Description was changed from ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/30d5e31afab3e0e4e35046fca420a4d3/revisions BUG=710938 ========== to ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/2bd0bb3d35275e21a18d73a5dd108167/revisions BUG=710938 ==========
Description was changed from ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/2bd0bb3d35275e21a18d73a5dd108167/revisions BUG=710938 ========== to ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/2bd0bb3d35275e21a18d73a5dd108167/revisions Please note these functions are unused for now. BUG=710938 ==========
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/2bd0bb3d35275e21a18d73a5dd108167/revisions Please note these functions are unused for now. BUG=710938 ========== to ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/4f15334b9ec625eb2b2c7d42c5d504fc/revisions Please note these functions are unused for now. BUG=710938 ==========
https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:99: {% endif %} The public methods have a comment with the property name, and an extra blank line after each set of methods. Do you think we need them here as well? https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl (right): https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl:3: inline static {{field.type_name}} {{field.initial_method_name}}() { There's a lot of code duplication here now :( Maybe in a follow-up patch, we could split base.decl_public_methods into decl_initial, decl_getter, decl_mutable and decl_setter. https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl:18: inline static {{field.type_name}} {{field.initial_method_name}}() { We probably won't need protected initial and resetter. https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl (right): https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl:6: {% if field.is_bit_field != 1 %} You can do: "{% if field.is_bit_field %} ... {% else %}"
https://codereview.chromium.org/2826653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2826653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:97: {% if field.field_template == 'storage_only' or field.field_template == 'monotonic_flag' or field.field_template == 'external' %} nit: I would do either: {% if field.field_template in ('storage_only', 'monotonic_flag', 'external') %} or {% if field.has_protected_methods %} and hide the logic in the generator. https://codereview.chromium.org/2826653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl (right): https://codereview.chromium.org/2826653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:17: {% macro getter_expression(field) %} Hmm, these macros only make sense in the presence of groups (it queries the field.group_member_name attribute). You could delete these but then you'd have to update all the getters/setters. You could also depend on the groups patch again :/
Description was changed from ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/4f15334b9ec625eb2b2c7d42c5d504fc/revisions Please note these functions are unused for now. BUG=710938 ========== to ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/26314ba4c574ae57f011467b84071a4e/revisions Please note these functions are unused for now. BUG=710938 ==========
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
shend@ PTAL? Thanks! https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:99: {% endif %} Makes sense. Done https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl (right): https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl:3: inline static {{field.type_name}} {{field.initial_method_name}}() { Will take a look at this in a later patch https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl:18: inline static {{field.type_name}} {{field.initial_method_name}}() { Removed. https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl (right): https://codereview.chromium.org/2826653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl:6: {% if field.is_bit_field != 1 %} Yup. That is better. https://codereview.chromium.org/2826653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2826653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:97: {% if field.field_template == 'storage_only' or field.field_template == 'monotonic_flag' or field.field_template == 'external' %} Went with option 1 https://codereview.chromium.org/2826653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl (right): https://codereview.chromium.org/2826653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:17: {% macro getter_expression(field) %} Done.
lgtm
nainar@chromium.org changed reviewers: + suzyh@chromium.org
Suzy, PTAL? Let me know if you don't have enough context and I can reassign.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I don't really have enough familiarity with it to give a good review, but I can make some hopefully-useful comments ^.^ Might be best to get a final sign off from someone with more context, though. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:138: self.internal_mutable_method_name = method_name(join_name('Mutable', name_for_methods, 'Internal')) The class doc says name_for_methods is the name used to form getter_method_name and setter_method_name. Why are {getter,setter}_method_name passed in as arguments, in that case? https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:97: {% if field.field_template in ('storage_only', 'monotonic_flag', 'external') %} Why 'external'? This is not mentioned in the CL description. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl:3: inline static {{field.type_name}} {{field.initial_method_name}}() { Why the change from return_type(field) to field.type_name? https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl:9: void {{field.setter_method_name}}(const {{field.type_name}}& v) { This is resulting in a rather odd-looking "const bool&" in the generated diff. Are you sure this is what you want? https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl:18: {{return_type(field)}} {{field.internal_getter_method_name}}() const { On first glance it's not clear why there's an internal and external version of these functions, since they appear to be the same. I think it may only be the same here as a base implementation, but I could be totally confused. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl:1: {% from 'fields/field.tmpl' import decode, encode, return_type %} Your change only seems to introduce a usage of 'decode'; are you sure you need the other ones? Also, line 3 seems redundant now. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:21: {{field.type_name}}& I'm not qualified to judge whether this is wise.
Description was changed from ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/26314ba4c574ae57f011467b84071a4e/revisions Please note these functions are unused for now. BUG=710938 ========== to ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags, external and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/26314ba4c574ae57f011467b84071a4e/revisions Please note these functions are unused for now. BUG=710938 ==========
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:138: self.internal_mutable_method_name = method_name(join_name('Mutable', name_for_methods, 'Internal')) That's because we need them as input to form the name for the protected getters/setters. They need to have the word Internal appended to them https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:97: {% if field.field_template in ('storage_only', 'monotonic_flag', 'external') %} On 2017/04/19 at 08:03:33, suzyh_UTC10 wrote: > Why 'external'? This is not mentioned in the CL description. Forgot to update the CL - done now. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl:3: inline static {{field.type_name}} {{field.initial_method_name}}() { On 2017/04/19 at 08:03:33, suzyh_UTC10 wrote: > Why the change from return_type(field) to field.type_name? Mistake. Fixed. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl:9: void {{field.setter_method_name}}(const {{field.type_name}}& v) { On 2017/04/19 at 08:03:33, suzyh_UTC10 wrote: > This is resulting in a rather odd-looking "const bool&" in the generated diff. Are you sure this is what you want? Fixed by using new argument_type macro https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/base.tmpl:18: {{return_type(field)}} {{field.internal_getter_method_name}}() const { On 2017/04/19 at 08:03:33, suzyh_UTC10 wrote: > On first glance it's not clear why there's an internal and external version of these functions, since they appear to be the same. I think it may only be the same here as a base implementation, but I could be totally confused. They are the same except the ones I am adding are protected. I could as shend@ suggested in a follow up CL refactor this out to have less repetition. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl:1: {% from 'fields/field.tmpl' import decode, encode, return_type %} On 2017/04/19 at 08:03:33, suzyh_UTC10 wrote: > Your change only seems to introduce a usage of 'decode'; are you sure you need the other ones? Also, line 3 seems redundant now. Deleted this line. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:21: {{field.type_name}}& On 2017/04/19 at 08:03:33, suzyh_UTC10 wrote: > I'm not qualified to judge whether this is wise. Need to pull this out for the mutable getter. This will be determined in the template itself. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/monotonic_flag.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/monotonic_flag.tmpl:1: {% from 'fields/field.tmpl' import encode, decode, return_type %} This is extra as well.
Description was changed from ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags, external and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/26314ba4c574ae57f011467b84071a4e/revisions Please note these functions are unused for now. BUG=710938 ========== to ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags, external and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/bf6ca12c12ff7283d90ebed7640cf7be/revisions Please note these functions are unused for now. BUG=710938 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A few more nits. Typo in CL description "we will be use this". https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:138: self.internal_mutable_method_name = method_name(join_name('Mutable', name_for_methods, 'Internal')) On 2017/04/19 at 08:49:35, nainar wrote: > That's because we need them as input to form the name for the protected getters/setters. They need to have the word Internal appended to them Clarification: I meant why not construct {getter,setter}_method_name here in this function from name_for_methods? https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:97: {% if field.field_template in ('storage_only', 'monotonic_flag', 'external') %} On 2017/04/19 at 08:49:35, nainar wrote: > On 2017/04/19 at 08:03:33, suzyh_UTC10 wrote: > > Why 'external'? This is not mentioned in the CL description. > > Forgot to update the CL - done now. The headline of the CL description does not mention external. It's already very long though, so maybe in the headline just say "some fields" https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl (right): https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl:11: {{field.type_name}}& {{field.internal_mutable_method_name}}() { return_type? https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl (right): https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:29: {{field.type_name}}& Did you mean a const & here? https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl (right): https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl:14: {{field.type_name}}& {{field.internal_mutable_method_name}}() { return_type?
Description was changed from ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags, external and storage_only fields on groups in ComputedStyle. In a following patch we will be use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/bf6ca12c12ff7283d90ebed7640cf7be/revisions Please note these functions are unused for now. BUG=710938 ========== to ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags, external and storage_only fields on groups in ComputedStyle. In a following patch we will use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/bf6ca12c12ff7283d90ebed7640cf7be/revisions Please note these functions are unused for now. BUG=710938 ==========
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Suzy, Made the changes you asked for. PTAL? Thanks! https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:138: self.internal_mutable_method_name = method_name(join_name('Mutable', name_for_methods, 'Internal')) On 2017/04/20 at 00:12:18, suzyh_UTC10 wrote: > On 2017/04/19 at 08:49:35, nainar wrote: > > That's because we need them as input to form the name for the protected getters/setters. They need to have the word Internal appended to them > > Clarification: I meant why not construct {getter,setter}_method_name here in this function from name_for_methods? So turns out I was stupid in explaining it to you twice. We are passing in {getter/setter}_method_name as they can be overriden. See here() for an example. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSPr... https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl (right): https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl:11: {{field.type_name}}& {{field.internal_mutable_method_name}}() { Done. https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl (right): https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:29: {{field.type_name}}& Done. https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl (right): https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl:14: {{field.type_name}}& {{field.internal_mutable_method_name}}() { Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm after final comments addressed. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:138: self.internal_mutable_method_name = method_name(join_name('Mutable', name_for_methods, 'Internal')) On 2017/04/20 at 02:53:04, nainar wrote: > On 2017/04/20 at 00:12:18, suzyh_UTC10 wrote: > > On 2017/04/19 at 08:49:35, nainar wrote: > > > That's because we need them as input to form the name for the protected getters/setters. They need to have the word Internal appended to them > > > > Clarification: I meant why not construct {getter,setter}_method_name here in this function from name_for_methods? > > So turns out I was stupid in explaining it to you twice. We are passing in {getter/setter}_method_name as they can be overriden. See here() for an example. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSPr... Aha! OK then. Ideally we'd tidy this up, so optionally consider filing a bug/adding a TODO. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:97: {% if field.field_template in ('storage_only', 'monotonic_flag', 'external') %} On 2017/04/20 at 00:12:18, suzyh_UTC10 wrote: > On 2017/04/19 at 08:49:35, nainar wrote: > > On 2017/04/19 at 08:03:33, suzyh_UTC10 wrote: > > > Why 'external'? This is not mentioned in the CL description. > > > > Forgot to update the CL - done now. > > The headline of the CL description does not mention external. It's already very long though, so maybe in the headline just say "some fields" Missed this comment? https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl (right): https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl:11: {{field.type_name}}& {{field.internal_mutable_method_name}}() { On 2017/04/20 at 02:53:04, nainar wrote: > Done. Er, isn't it a function call? return_type(field)? https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl (right): https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl:14: {{field.type_name}}& {{field.internal_mutable_method_name}}() { On 2017/04/20 at 02:53:04, nainar wrote: > Done. Ditto: return_type(field)?
https://codereview.chromium.org/2826653002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl (right): https://codereview.chromium.org/2826653002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl:11: {{return_type}} {{field.internal_mutable_method_name}}() { I think this one can stay as {{field.type_name}}&. External fields can never be a bit field. Hence, we would always be returning by reference. return_type is only useful when field can be a bit field or a non bit field (e.g. storage_only fields).
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:138: self.internal_mutable_method_name = method_name(join_name('Mutable', name_for_methods, 'Internal')) Done. https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2826653002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:97: {% if field.field_template in ('storage_only', 'monotonic_flag', 'external') %} On 2017/04/20 at 03:48:02, suzyh_UTC10 wrote: > On 2017/04/20 at 00:12:18, suzyh_UTC10 wrote: > > On 2017/04/19 at 08:49:35, nainar wrote: > > > On 2017/04/19 at 08:03:33, suzyh_UTC10 wrote: > > > > Why 'external'? This is not mentioned in the CL description. > > > > > > Forgot to update the CL - done now. > > > > The headline of the CL description does not mention external. It's already very long though, so maybe in the headline just say "some fields" > > Missed this comment? Edited it to some fields. And yes I did miss it. https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl (right): https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl:11: {{field.type_name}}& {{field.internal_mutable_method_name}}() { Fixed. Scratches head about how this built. https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl (right): https://codereview.chromium.org/2826653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/fields/storage_only.tmpl:14: {{field.type_name}}& {{field.internal_mutable_method_name}}() { Done. https://codereview.chromium.org/2826653002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl (right): https://codereview.chromium.org/2826653002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/fields/external.tmpl:11: {{return_type}} {{field.internal_mutable_method_name}}() { Done.
Description was changed from ========== Generate getters/setters as needed for monotic_flags and storage_only fields on groups in ComputedStyle. This patch generates getters/setters as needed for monotic_flags, external and storage_only fields on groups in ComputedStyle. In a following patch we will use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/bf6ca12c12ff7283d90ebed7640cf7be/revisions Please note these functions are unused for now. BUG=710938 ========== to ========== Generate getters/setters for some fields on groups in ComputedStyle This patch generates getters/setters as needed for monotic_flags, external and storage_only fields on groups in ComputedStyle. In a following patch we will use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/bf6ca12c12ff7283d90ebed7640cf7be/revisions Please note these functions are unused for now. BUG=710938 ==========
The CQ bit was unchecked by nainar@chromium.org
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shend@chromium.org, suzyh@chromium.org Link to the patchset: https://codereview.chromium.org/2826653002/#ps120001 (title: "Clean up nits prelanding")
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": 120001, "attempt_start_ts": 1492665792735900,
"parent_rev": "e6704a0c2b29585fab489399ba0396104cff7d85", "commit_rev":
"7185f26bb323f0650f8831f6e2d2c976fd373e55"}
Message was sent while issue was closed.
Description was changed from ========== Generate getters/setters for some fields on groups in ComputedStyle This patch generates getters/setters as needed for monotic_flags, external and storage_only fields on groups in ComputedStyle. In a following patch we will use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/bf6ca12c12ff7283d90ebed7640cf7be/revisions Please note these functions are unused for now. BUG=710938 ========== to ========== Generate getters/setters for some fields on groups in ComputedStyle This patch generates getters/setters as needed for monotic_flags, external and storage_only fields on groups in ComputedStyle. In a following patch we will use this to access members of groups through these getters/setters instead of directly as <group_name>.<field_name>. Generated diff here: https://gist.github.com/nainar/bf6ca12c12ff7283d90ebed7640cf7be/revisions Please note these functions are unused for now. BUG=710938 Review-Url: https://codereview.chromium.org/2826653002 Cr-Commit-Position: refs/heads/master@{#465919} Committed: https://chromium.googlesource.com/chromium/src/+/7185f26bb323f0650f8831f6e2d2... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/7185f26bb323f0650f8831f6e2d2... |
