|
|
Created:
3 years, 9 months ago by shend Modified:
3 years, 9 months ago Reviewers:
alancutter (OOO until 2018) CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove naming related code in make_computed_style_base to name_utilities.
Currently make_computed_style_base is sprinkled with a lot of code
related to variable names and method names. These are common pieces of
logic that can be reused by other generators as well.
This patch tidies up make_computed_style_base by moving naming logic
to name_utilities.py.
BUG=628043
Review-Url: https://codereview.chromium.org/2756113002
Cr-Commit-Position: refs/heads/master@{#458635}
Committed: https://chromium.googlesource.com/chromium/src/+/a6bfb94ddcb95eef883d62b3d65de8e13192e90c
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase #Patch Set 3 : Rebase #Patch Set 4 : Rebase #
Total comments: 2
Patch Set 5 : Rebase #
Dependent Patchsets: Messages
Total messages: 41 (30 generated)
shend@chromium.org changed reviewers: + alancutter@chromium.org
Hi Alan, PTAL
https://codereview.chromium.org/2756113002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/name_utilities.py (right): https://codereview.chromium.org/2756113002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/name_utilities.py:138: # High level API Not sure if these comment headers really add much. https://codereview.chromium.org/2756113002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/name_utilities.py:141: def enumeration_name(name): Should this be property_name? https://codereview.chromium.org/2756113002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/name_utilities.py:145: def enumerator_name(name): enumeration and enumerator are a bit academic as terminology. I'd just call them enum_type_name and enum_value_name for clarity.
Description was changed from ========== Move naming related code in make_computed_style_base to name_utilities. Currently make_computed_style_base is sprinkled with a lot of code related to variable names and method names. These are common pieces of logic that can be reused by other generators as well. This patch tidies up make_computed_style_base by moving naming logic to name_utilities.py. It also refactors the code to simplify the handling of names. BUG=628043 ========== to ========== Move naming related code in make_computed_style_base to name_utilities. Currently make_computed_style_base is sprinkled with a lot of code related to variable names and method names. These are common pieces of logic that can be reused by other generators as well. This patch tidies up make_computed_style_base by moving naming logic to name_utilities.py. BUG=628043 ==========
Split CL and addressed comments, PTAL again Alan :) https://codereview.chromium.org/2756113002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/name_utilities.py (right): https://codereview.chromium.org/2756113002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/name_utilities.py:138: # High level API On 2017/03/20 at 05:10:16, alancutter wrote: > Not sure if these comment headers really add much. Fixed. https://codereview.chromium.org/2756113002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/name_utilities.py:141: def enumeration_name(name): On 2017/03/20 at 05:10:16, alancutter wrote: > Should this be property_name? I'm trying to keep this generic for any type of code, not just style code. https://codereview.chromium.org/2756113002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/name_utilities.py:145: def enumerator_name(name): On 2017/03/20 at 05:10:16, alancutter wrote: > enumeration and enumerator are a bit academic as terminology. I'd just call them enum_type_name and enum_value_name for clarity. Good idea. 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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.
lgtm https://codereview.chromium.org/2756113002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2756113002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:210: name_for_methods = property_['name'] It's weird to call this name_for_methods, it should just be called name since it's used for more than just methods.
https://codereview.chromium.org/2756113002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2756113002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:210: name_for_methods = property_['name'] On 2017/03/21 at 04:56:47, alancutter wrote: > It's weird to call this name_for_methods, it should just be called name since it's used for more than just methods. Oops, that was meant to read "name_for_methods = property_['name_for_methods']". Yeah unfortunately name_for_methods is shared with other generators, and "name" is reserved for the name of the property.
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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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/2756113002/#ps80001 (title: "Rebase")
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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
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: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
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.
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": 80001, "attempt_start_ts": 1490158580765180, "parent_rev": "a0f637af5fb75e1fbd58e4c4c03807c3e77fa213", "commit_rev": "a6bfb94ddcb95eef883d62b3d65de8e13192e90c"}
Message was sent while issue was closed.
Description was changed from ========== Move naming related code in make_computed_style_base to name_utilities. Currently make_computed_style_base is sprinkled with a lot of code related to variable names and method names. These are common pieces of logic that can be reused by other generators as well. This patch tidies up make_computed_style_base by moving naming logic to name_utilities.py. BUG=628043 ========== to ========== Move naming related code in make_computed_style_base to name_utilities. Currently make_computed_style_base is sprinkled with a lot of code related to variable names and method names. These are common pieces of logic that can be reused by other generators as well. This patch tidies up make_computed_style_base by moving naming logic to name_utilities.py. BUG=628043 Review-Url: https://codereview.chromium.org/2756113002 Cr-Commit-Position: refs/heads/master@{#458635} Committed: https://chromium.googlesource.com/chromium/src/+/a6bfb94ddcb95eef883d62b3d65d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/a6bfb94ddcb95eef883d62b3d65d... |