|
|
DescriptionExtract default naming logic in StyleBuilderWriter to a method.
Currently, StyleBuilderWriter augments its input with attributes related
to naming, which are used in its subclasses. However, this logic is in
the StyleBuilderWriter constructor, which makes it difficult to invok .
For example, it's difficult for a subclass to read a separate JSON file
and apply this naming logic on it.
This patch moves the naming logic out into a separate method that
subclasses can invoke at anytime. The separate method assumes the
existence of keys like 'getter' and 'custom_all', which do not exist
in NONPROPERTIES. Instead of going through all those keys and setting
their value to None, we just use a defaultdict so that any access to
missing keys returns None.
To show that it's working, we apply the same naming logic on both
properties and nonproperties in make_computed_style_base, despite the
two being stored separately.
BUG=628043
Review-Url: https://codereview.chromium.org/2811253002
Cr-Commit-Position: refs/heads/master@{#465944}
Committed: https://chromium.googlesource.com/chromium/src/+/efefb24c1c0d9c7af36c2af86933e5e6e312c5a8
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase #Patch Set 3 : Rebase #Patch Set 4 : Rebase #
Total comments: 4
Patch Set 5 : Address comments #
Total comments: 2
Patch Set 6 : Add TODO #
Dependent Patchsets: Messages
Total messages: 36 (23 generated)
shend@chromium.org changed reviewers: + bugsnash@chromium.org
Hi Bugs, PTAL
https://codereview.chromium.org/2811253002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (left): https://codereview.chromium.org/2811253002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:312: if 'field_type_path' not in property_: this field_type_path logic does not seem to be in _apply_property_naming_defaults
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...
https://codereview.chromium.org/2811253002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (left): https://codereview.chromium.org/2811253002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:312: if 'field_type_path' not in property_: On 2017/04/12 at 23:31:45, Bugs Nash wrote: > this field_type_path logic does not seem to be in _apply_property_naming_defaults This line should be fine because all it's doing is setting property_['field_type_path'] to None if it doesn't exist, which is now subsumed by the defaultdict. In retrospect, I probably should've split this patch into two because it's kinda doing two somewhat related things.
https://codereview.chromium.org/2811253002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (left): https://codereview.chromium.org/2811253002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:312: if 'field_type_path' not in property_: On 2017/04/12 at 23:41:03, shend wrote: > On 2017/04/12 at 23:31:45, Bugs Nash wrote: > > this field_type_path logic does not seem to be in _apply_property_naming_defaults > > This line should be fine because all it's doing is setting property_['field_type_path'] to None if it doesn't exist, which is now subsumed by the defaultdict. In retrospect, I probably should've split this patch into two because it's kinda doing two somewhat related things. The default dict doesn't add field_type_path at all though correct?
https://codereview.chromium.org/2811253002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (left): https://codereview.chromium.org/2811253002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:312: if 'field_type_path' not in property_: On 2017/04/12 at 23:52:37, Bugs Nash wrote: > On 2017/04/12 at 23:41:03, shend wrote: > > On 2017/04/12 at 23:31:45, Bugs Nash wrote: > > > this field_type_path logic does not seem to be in _apply_property_naming_defaults > > > > This line should be fine because all it's doing is setting property_['field_type_path'] to None if it doesn't exist, which is now subsumed by the defaultdict. In retrospect, I probably should've split this patch into two because it's kinda doing two somewhat related things. > > The default dict doesn't add field_type_path at all though correct? Yeah, it doesn't add field_type_path. So if property_ originally had field_type_path, then it'll keep that value, but if it didn't, then it'll be None, not a KeyError.
lgtm
Description was changed from ========== Extract default naming logic in StyleBuilderWriter to a method. Currently, StyleBuilderWriter augments its input with attributes related to naming, which are used in its subclasses. However, this logic is in the StyleBuilderWriter constructor, which makes it difficult to invok . For example, it's difficult for a subclass to read a separate JSON file and apply this naming logic on it. This patch moves the naming logic out into a separate method that subclasses can invoke at anytime. To show that it's working, we apply the same naming logic on both properties and nonproperties in make_computed_style_base, despite the two being stored separately. BUG=628043 ========== to ========== Extract default naming logic in StyleBuilderWriter to a method. Currently, StyleBuilderWriter augments its input with attributes related to naming, which are used in its subclasses. However, this logic is in the StyleBuilderWriter constructor, which makes it difficult to invok . For example, it's difficult for a subclass to read a separate JSON file and apply this naming logic on it. This patch moves the naming logic out into a separate method that subclasses can invoke at anytime. The separate method assumes the existence of keys like 'getter' and 'custom_all', which do not exist in NONPROPERTIES. Instead of going through all those keys and setting their value to None, we just use a defaultdict so that any access to missing keys returns None. To show that it's working, we apply the same naming logic on both properties and nonproperties in make_computed_style_base, despite the two being stored separately. BUG=628043 ==========
shend@chromium.org changed reviewers: + alancutter@chromium.org
Hi Alan, PTAL
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 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: 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 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2811253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2811253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:314: nonproperties = [defaultdict(lambda: None, item) for item in NONPROPERTIES] Not sure I'm in favour of a defaultdict. This makes it harder to catch unintended reads. I would rather see the difference between NONPROPERTIES and _properties captured in the logic here that sets None on specific keys. https://codereview.chromium.org/2811253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_style_builder.py (right): https://codereview.chromium.org/2811253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_style_builder.py:53: def _apply_property_naming_defaults(self, property_): self is unused, make this a static method or utility function.
Hi Alan, PTAL again :) https://codereview.chromium.org/2811253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2811253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:314: nonproperties = [defaultdict(lambda: None, item) for item in NONPROPERTIES] On 2017/04/19 at 04:36:35, alancutter wrote: > Not sure I'm in favour of a defaultdict. This makes it harder to catch unintended reads. > I would rather see the difference between NONPROPERTIES and _properties captured in the logic here that sets None on specific keys. Done in an explicit loop now. https://codereview.chromium.org/2811253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_style_builder.py (right): https://codereview.chromium.org/2811253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_style_builder.py:53: def _apply_property_naming_defaults(self, property_): On 2017/04/19 at 04:36:35, alancutter wrote: > self is unused, make this a static method or utility function. Done.
lgtm https://codereview.chromium.org/2811253002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_style_builder.py (right): https://codereview.chromium.org/2811253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_style_builder.py:47: simple_type_name = str(property_['type_name']).split('::')[-1] Nit: The interlacing of these statements is a bit arbitrary. Let's separate assignment/set_if_none() cleanly as a visual aid for reading this code.
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...
https://codereview.chromium.org/2811253002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_style_builder.py (right): https://codereview.chromium.org/2811253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_style_builder.py:47: simple_type_name = str(property_['type_name']).split('::')[-1] On 2017/04/20 at 04:39:48, alancutter wrote: > Nit: The interlacing of these statements is a bit arbitrary. Let's separate assignment/set_if_none() cleanly as a visual aid for reading this code. Added a TODO.
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 shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2811253002/#ps100001 (title: "Add TODO")
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": 100001, "attempt_start_ts": 1492675646063430, "parent_rev": "dde63c1ac61bff5c828f389c12a1c612d1d3ae7f", "commit_rev": "efefb24c1c0d9c7af36c2af86933e5e6e312c5a8"}
Message was sent while issue was closed.
Description was changed from ========== Extract default naming logic in StyleBuilderWriter to a method. Currently, StyleBuilderWriter augments its input with attributes related to naming, which are used in its subclasses. However, this logic is in the StyleBuilderWriter constructor, which makes it difficult to invok . For example, it's difficult for a subclass to read a separate JSON file and apply this naming logic on it. This patch moves the naming logic out into a separate method that subclasses can invoke at anytime. The separate method assumes the existence of keys like 'getter' and 'custom_all', which do not exist in NONPROPERTIES. Instead of going through all those keys and setting their value to None, we just use a defaultdict so that any access to missing keys returns None. To show that it's working, we apply the same naming logic on both properties and nonproperties in make_computed_style_base, despite the two being stored separately. BUG=628043 ========== to ========== Extract default naming logic in StyleBuilderWriter to a method. Currently, StyleBuilderWriter augments its input with attributes related to naming, which are used in its subclasses. However, this logic is in the StyleBuilderWriter constructor, which makes it difficult to invok . For example, it's difficult for a subclass to read a separate JSON file and apply this naming logic on it. This patch moves the naming logic out into a separate method that subclasses can invoke at anytime. The separate method assumes the existence of keys like 'getter' and 'custom_all', which do not exist in NONPROPERTIES. Instead of going through all those keys and setting their value to None, we just use a defaultdict so that any access to missing keys returns None. To show that it's working, we apply the same naming logic on both properties and nonproperties in make_computed_style_base, despite the two being stored separately. BUG=628043 Review-Url: https://codereview.chromium.org/2811253002 Cr-Commit-Position: refs/heads/master@{#465944} Committed: https://chromium.googlesource.com/chromium/src/+/efefb24c1c0d9c7af36c2af86933... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/efefb24c1c0d9c7af36c2af86933... |