Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2766933002:
Unify logic for properties and nonproperties in computed_style_base.py (Closed)
|
Created:
3 years, 9 months ago by shend Modified:
3 years, 9 months ago Reviewers:
alancutter (OOO until 2018) CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnify logic for properties and nonproperties in computed_style_base.py
As nonproperty fields get more complex, the logic to create nonproperty
fields becomes very similar to property fields. This patch merges the
two code paths so that properties and nonproperties share as much logic
as possible.
BUG=628043
Review-Url: https://codereview.chromium.org/2766933002
Cr-Commit-Position: refs/heads/master@{#459231}
Committed: https://chromium.googlesource.com/chromium/src/+/bc0808a25d54a0138ce2d61e838191a5b1f4f3e0
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 23
Patch Set 3 : Address comments #
Total comments: 2
Patch Set 4 : Address comments #Messages
Total messages: 34 (20 generated)
shend@chromium.org changed reviewers: + alancutter@chromium.org
Hi Alan, PTAL
The CQ bit was checked by shend@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:90: self.is_nonproperty = field_role == 'nonproperty' We should aim to not store the field_role anywhere and instead add Field members that generalise over all roles so that templates are unaware of the Field role. Let's add a TODO for this here. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:94: if self.is_property or self.is_nonproperty: Use "if not self.is_inherited_flag" as this is about not putting inherited flags on inherited flags. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:166: # Must be a boolean flag This comment should be an assert instead. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:202: Create ComputedStyle fields from properties or nonproperties and return a list of Field objects. Instead of describing the input (which is a detail of the call site) just describe that the output is a list of Fields including is_inherited flag fields. As a side note the name nonproperties is confusing in that it doesn't include the is_inherited flags which are "nonproperties" themselves. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:264: property_['field_type_path'] = property_.get('field_type_path') I'd rather see it crash if it's missing than be defensive here. When the "nonproperties" are put in a JSON5 file this can be handled by the JSON5 reader automatically. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:266: property_['type_name'] = property_.get('type_name', 'E' + enum_type_name(property_['name_for_methods'])) Let's use "if attr not in object: object[attr] = default" instead. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:280: _create_fields('nonproperty', NONPROPERTIES)) We don't really need these "create X" comments, the code is self explanatory enough. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:311: self._property_values = property_values No need to upgrade the local variable as a member, leave as is.
Replied to some comments. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:94: if self.is_property or self.is_nonproperty: On 2017/03/22 at 07:01:44, alancutter wrote: > Use "if not self.is_inherited_flag" as this is about not putting inherited flags on inherited flags. Done. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:166: # Must be a boolean flag On 2017/03/22 at 07:01:44, alancutter wrote: > This comment should be an assert instead. Done. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:202: Create ComputedStyle fields from properties or nonproperties and return a list of Field objects. On 2017/03/22 at 07:01:44, alancutter wrote: > Instead of describing the input (which is a detail of the call site) just describe that the output is a list of Fields including is_inherited flag fields. > > As a side note the name nonproperties is confusing in that it doesn't include the is_inherited flags which are "nonproperties" themselves. Yes I don't like the name nonproperties. Do you have a suggestion for an alternate name? https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:264: property_['field_type_path'] = property_.get('field_type_path') On 2017/03/22 at 07:01:44, alancutter wrote: > I'd rather see it crash if it's missing than be defensive here. When the "nonproperties" are put in a JSON5 file this can be handled by the JSON5 reader automatically. Hmm, so I'm trying to figure out what you mean here. So you're suggesting that, if an entry in the NONPROPERTIES dict has no field_type_path, it should explicitly specify "'field_type_path': None" at the top of the file instead of omitting it? https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:266: property_['type_name'] = property_.get('type_name', 'E' + enum_type_name(property_['name_for_methods'])) On 2017/03/22 at 07:01:44, alancutter wrote: > Let's use "if attr not in object: object[attr] = default" instead. Same here, would we write "if object[attr] is None: object[attr] = default" instead? https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:280: _create_fields('nonproperty', NONPROPERTIES)) On 2017/03/22 at 07:01:44, alancutter wrote: > We don't really need these "create X" comments, the code is self explanatory enough. Done. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:311: self._property_values = property_values On 2017/03/22 at 07:01:44, alancutter wrote: > No need to upgrade the local variable as a member, leave as is. self._property_values is actually used in a separate method (generate_css_value_mappings, see below).
https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:202: Create ComputedStyle fields from properties or nonproperties and return a list of Field objects. On 2017/03/22 at 22:29:18, shend wrote: > On 2017/03/22 at 07:01:44, alancutter wrote: > > Instead of describing the input (which is a detail of the call site) just describe that the output is a list of Fields including is_inherited flag fields. > > > > As a side note the name nonproperties is confusing in that it doesn't include the is_inherited flags which are "nonproperties" themselves. > > Yes I don't like the name nonproperties. Do you have a suggestion for an alternate name? Actually, the term nonproperty was already in the codebase: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com...
https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:202: Create ComputedStyle fields from properties or nonproperties and return a list of Field objects. On 2017/03/23 at 00:05:29, shend wrote: > On 2017/03/22 at 22:29:18, shend wrote: > > On 2017/03/22 at 07:01:44, alancutter wrote: > > > Instead of describing the input (which is a detail of the call site) just describe that the output is a list of Fields including is_inherited flag fields. > > > > > > As a side note the name nonproperties is confusing in that it doesn't include the is_inherited flags which are "nonproperties" themselves. > > > > Yes I don't like the name nonproperties. Do you have a suggestion for an alternate name? > > Actually, the term nonproperty was already in the codebase: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com... The name nonproperties itself is okay it's just strange that in this context it excludes inherited flags. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:264: property_['field_type_path'] = property_.get('field_type_path') On 2017/03/22 at 22:29:18, shend wrote: > On 2017/03/22 at 07:01:44, alancutter wrote: > > I'd rather see it crash if it's missing than be defensive here. When the "nonproperties" are put in a JSON5 file this can be handled by the JSON5 reader automatically. > > Hmm, so I'm trying to figure out what you mean here. So you're suggesting that, if an entry in the NONPROPERTIES dict has no field_type_path, it should explicitly specify "'field_type_path': None" at the top of the file instead of omitting it? Ah, sorry it does make sense to have this then. I'd prefer that it used if instead of get to default the value though. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:266: property_['type_name'] = property_.get('type_name', 'E' + enum_type_name(property_['name_for_methods'])) On 2017/03/22 at 22:29:18, shend wrote: > On 2017/03/22 at 07:01:44, alancutter wrote: > > Let's use "if attr not in object: object[attr] = default" instead. > > Same here, would we write "if object[attr] is None: object[attr] = default" instead? Doesn't this default only apply if the attr isn't in the object? https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:311: self._property_values = property_values On 2017/03/22 at 22:29:18, shend wrote: > On 2017/03/22 at 07:01:44, alancutter wrote: > > No need to upgrade the local variable as a member, leave as is. > > self._property_values is actually used in a separate method (generate_css_value_mappings, see below). That method can continue to use self._properties.values(). It's redundant to have _property_values as a member when you already have access to that.
Addressed comments. Regarding nonproperties vs properties, I'll overhaul that part in a later patch since I still need to think it a bit more. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:202: Create ComputedStyle fields from properties or nonproperties and return a list of Field objects. On 2017/03/23 at 02:13:41, alancutter wrote: > On 2017/03/23 at 00:05:29, shend wrote: > > On 2017/03/22 at 22:29:18, shend wrote: > > > On 2017/03/22 at 07:01:44, alancutter wrote: > > > > Instead of describing the input (which is a detail of the call site) just describe that the output is a list of Fields including is_inherited flag fields. > > > > > > > > As a side note the name nonproperties is confusing in that it doesn't include the is_inherited flags which are "nonproperties" themselves. > > > > > > Yes I don't like the name nonproperties. Do you have a suggestion for an alternate name? > > > > Actually, the term nonproperty was already in the codebase: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com... > > The name nonproperties itself is okay it's just strange that in this context it excludes inherited flags. Hmm it might be possible to unify the code paths for nonproperties and inheritance flags as well, so that an inheritance flag is just one type of nonproperty. I'll look into it. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:266: property_['type_name'] = property_.get('type_name', 'E' + enum_type_name(property_['name_for_methods'])) On 2017/03/23 at 02:13:41, alancutter wrote: > On 2017/03/22 at 22:29:18, shend wrote: > > On 2017/03/22 at 07:01:44, alancutter wrote: > > > Let's use "if attr not in object: object[attr] = default" instead. > > > > Same here, would we write "if object[attr] is None: object[attr] = default" instead? > > Doesn't this default only apply if the attr isn't in the object? Yeah don't worry, your previous comment clarified everything. https://codereview.chromium.org/2766933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:311: self._property_values = property_values On 2017/03/23 at 02:13:41, alancutter wrote: > On 2017/03/22 at 22:29:18, shend wrote: > > On 2017/03/22 at 07:01:44, alancutter wrote: > > > No need to upgrade the local variable as a member, leave as is. > > > > self._property_values is actually used in a separate method (generate_css_value_mappings, see below). > > That method can continue to use self._properties.values(). It's redundant to have _property_values as a member when you already have access to that. Yep okay, done.
The CQ bit was checked by shend@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...
lgtm https://codereview.chromium.org/2766933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2766933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:311: self._property_values = property_values This member is now unused and can be removed.
The CQ bit was unchecked by shend@chromium.org
The CQ bit was checked by shend@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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2766933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2766933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:311: self._property_values = property_values On 2017/03/23 at 03:02:47, alancutter wrote: > This member is now unused and can be removed. Oops, done.
The CQ bit was checked by shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2766933002/#ps60001 (title: "Address comments")
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by shend@chromium.org
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by shend@chromium.org
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": 60001, "attempt_start_ts": 1490303668284620,
"parent_rev": "1dc8c4b75eb928fdd9b4984c1f99a64c029301d8", "commit_rev":
"bc0808a25d54a0138ce2d61e838191a5b1f4f3e0"}
Message was sent while issue was closed.
Description was changed from ========== Unify logic for properties and nonproperties in computed_style_base.py As nonproperty fields get more complex, the logic to create nonproperty fields becomes very similar to property fields. This patch merges the two code paths so that properties and nonproperties share as much logic as possible. BUG=628043 ========== to ========== Unify logic for properties and nonproperties in computed_style_base.py As nonproperty fields get more complex, the logic to create nonproperty fields becomes very similar to property fields. This patch merges the two code paths so that properties and nonproperties share as much logic as possible. BUG=628043 Review-Url: https://codereview.chromium.org/2766933002 Cr-Commit-Position: refs/heads/master@{#459231} Committed: https://chromium.googlesource.com/chromium/src/+/bc0808a25d54a0138ce2d61e8381... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bc0808a25d54a0138ce2d61e8381... |
