Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2879563002:
Refactor code generation to allow us to diff generic expressions and not just fields (Closed)
DescriptionRefactor code generation to allow us to diff generic expressions and not just fields
This patch refactors the diffing generator code to allow us to diff
field getters and other expressions instead of just fields themselves in
the future.
It does so by creating a tree of groups to diff (DiffGroups)
that store the subgroups to diff as well as the expressions to diff.
For now the expressions are just the field accessors. In the future,
they will contain expressions like BorderLeftWidth() - a public getter
for border-left-width, and HasTransform() - which is not a field or
its getter.
Diff: https://gist.github.com/d14529581f729e810842254cbf7dc1f5/revisions
BUG=710938
Review-Url: https://codereview.chromium.org/2879563002
Cr-Commit-Position: refs/heads/master@{#471666}
Committed: https://chromium.googlesource.com/chromium/src/+/0066ff300549155904a9d1708488fc617f705404
Patch Set 1 #
Total comments: 12
Patch Set 2 : shend@'s changes #
Total comments: 10
Patch Set 3 : shend@'s suggestions #
Total comments: 6
Patch Set 4 : alancutter@'s suggestions #Patch Set 5 : Formatting #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 43 (30 generated)
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...
nainar@chromium.org changed reviewers: + shend@chromium.org
PTAL? https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (left): https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:54: oops. Added this in.
Awesome. Some comments about moving the recursion up one level to simplify the code. https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:82: expressions: List of functions that are on this group that need to be diffed. "List of expressions" https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:185: def _create_field(field_name, all_properties): Hmm, I'm not quite sure why this is needed. I think you should be able to do all the diff group logic with just root_group, without creating any new Field instances and stuff. https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:191: def _create_expression(field): maybe _getter_expression? Or even put this as a member of Field? https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:211: def _create_diff_groups(diff_function_inputs, all_properties): As mentioned below, I believe this code would be easier if this returned a map of name -> DF, not name -> DFs. The code should be very similar to the original jinja template. Something like: _create_diff_group(root_group, fields_to_diff): DG = DiffGroup() for subgroup in root_group.subgroups: if subgroup.all_fields and fields_to_diff intersect DG.subgroups.append(_create_diff_groups(subgroup, fields_to_diff)) for field in group.fields that are in fields_to_diff DG.exprs.append(getter_expression(field)). return DG. https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl (right): https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:90: {% macro fieldwise_diff(groups_to_diff) %} Would this be easier if you took in a single group (i.e. the root DF)? Like move up a level of recursion. Something like the original jinja code: fieldwise_diff(df) for subgroup in df.subgroups fieldwise_diff(subgroup) endfor for expr in df.exprs endfor
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
made the changes you asked for. PTAL? https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (left): https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:54: Removed. https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:82: expressions: List of functions that are on this group that need to be diffed. Done. https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:185: def _create_field(field_name, all_properties): Didn't realize root_group was a thing https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:191: def _create_expression(field): Added it as a member on field https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:211: def _create_diff_groups(diff_function_inputs, all_properties): Done. https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl (right): https://codereview.chromium.org/2879563002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:90: {% macro fieldwise_diff(groups_to_diff) %} Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
fantastic, thanks. Just one more suggestion. https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:156: self.getter_expression = self.group_name + '_data_->' + class_member_name(self.name) I think you can use 'self.group_member_name' instead of self.group_name + '_data'. https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:183: no_group = {} No longer needed I believe. https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:200: for field in subgroup.all_fields: This is the intersection code right? I think you can just do a one-liner: if any(field.property_name in subgroup.all_fields for field in fields_to_diff): append or if len(set(subgroup.all_fields) & set(fields_to_diff)) > 0: https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:2: // This file specifies the fields we want to diff in the various diff functions nit: indent https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:3: //in ComputedStyle. nit: space
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Made the changes you asked for, PTAL? Thanks! https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:156: self.getter_expression = self.group_name + '_data_->' + class_member_name(self.name) Done. https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:183: no_group = {} Done. https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:200: for field in subgroup.all_fields: Done. With protest https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:2: // This file specifies the fields we want to diff in the various diff functions Done. https://codereview.chromium.org/2879563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:3: //in ComputedStyle. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
nainar@chromium.org changed reviewers: + alancutter@chromium.org
@alancutter for OWNERS, PTAL? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:89: Two newlines between class definitions. https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:90: bool ScrollAnchorDisablingPropertyChanged(const ComputedStyleBase& other) const; Can we generate these function names instead? https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:17: }, No need to include this in this patch if it's unused.
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...
alancutter@, PTAL? Thanks! https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:89: Done. https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:90: bool ScrollAnchorDisablingPropertyChanged(const ComputedStyleBase& other) const; On 2017/05/12 at 04:04:51, alancutter wrote: > Can we generate these function names instead? Made this CL: https://codereview.chromium.org/2875233002 a parent to fix this issue. https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2879563002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:17: }, Removed.
lgtm
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 Link to the patchset: https://codereview.chromium.org/2879563002/#ps60001 (title: "alancutter@'s suggestions")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2875233002 Patch 20001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
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 ========== Refactor code generation to allow us to diff generic expressions and not just fields This patch refactors the diffing generator code to allow us to diff field getters and other expressions instead of just fields themselves in the future. It does so by creating a tree of groups to diff (DiffGroups) that store the subgroups to diff as well as the expressions to diff. For now the expressions are just the field accessors. In the future, they will contain expressions like BorderLeftWidth() - a public getter for border-left-width, and HasTransform() - which is not a field or its getter. Diff: https://gist.github.com/a951c87d079cefd8cf08940423a76c52/revisions BUG=710938 ========== to ========== Refactor code generation to allow us to diff generic expressions and not just fields This patch refactors the diffing generator code to allow us to diff field getters and other expressions instead of just fields themselves in the future. It does so by creating a tree of groups to diff (DiffGroups) that store the subgroups to diff as well as the expressions to diff. For now the expressions are just the field accessors. In the future, they will contain expressions like BorderLeftWidth() - a public getter for border-left-width, and HasTransform() - which is not a field or its getter. Diff: https://gist.github.com/d14529581f729e810842254cbf7dc1f5/revisions BUG=710938 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 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 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 alancutter@chromium.org, shend@chromium.org Link to the patchset: https://codereview.chromium.org/2879563002/#ps80001 (title: "Formatting")
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": 80001, "attempt_start_ts": 1494821980191730,
"parent_rev": "e0255eded6d4fb960323b8aa66065783ea9ae5e1", "commit_rev":
"0066ff300549155904a9d1708488fc617f705404"}
Message was sent while issue was closed.
Description was changed from ========== Refactor code generation to allow us to diff generic expressions and not just fields This patch refactors the diffing generator code to allow us to diff field getters and other expressions instead of just fields themselves in the future. It does so by creating a tree of groups to diff (DiffGroups) that store the subgroups to diff as well as the expressions to diff. For now the expressions are just the field accessors. In the future, they will contain expressions like BorderLeftWidth() - a public getter for border-left-width, and HasTransform() - which is not a field or its getter. Diff: https://gist.github.com/d14529581f729e810842254cbf7dc1f5/revisions BUG=710938 ========== to ========== Refactor code generation to allow us to diff generic expressions and not just fields This patch refactors the diffing generator code to allow us to diff field getters and other expressions instead of just fields themselves in the future. It does so by creating a tree of groups to diff (DiffGroups) that store the subgroups to diff as well as the expressions to diff. For now the expressions are just the field accessors. In the future, they will contain expressions like BorderLeftWidth() - a public getter for border-left-width, and HasTransform() - which is not a field or its getter. Diff: https://gist.github.com/d14529581f729e810842254cbf7dc1f5/revisions BUG=710938 Review-Url: https://codereview.chromium.org/2879563002 Cr-Commit-Position: refs/heads/master@{#471666} Committed: https://chromium.googlesource.com/chromium/src/+/0066ff300549155904a9d1708488... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0066ff300549155904a9d1708488... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
