Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(56)

Issue 2890743003: Do not use overridden getter names for internal getters. (Closed)

Created:
3 years, 7 months ago by shend
Modified:
3 years, 7 months ago
CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Do not use overridden getter names for internal getters. Currently, we append the word 'internal' to the name of a field's getter to form the name of the internal getter. However, there are cases where this forms a bad name. For example, the getter for 'line-height' is overridden to be SpecifiedLineHeight, which contains complex logic. This means that the internal getter will be called SpecifiedLineHeightInternal. This is bad because the internal getter does not have any logic to do with specified line height. A more apt name would be just LineHeightInternal. This patch changes the internal getters to be <field name> + 'internal'. Diff of generated files: https://gist.github.com/780cea9912e024f157a97fba24ee5a0e/revisions BUG=628043 Review-Url: https://codereview.chromium.org/2890743003 Cr-Commit-Position: refs/heads/master@{#473031} Committed: https://chromium.googlesource.com/chromium/src/+/9f0d5e6a0694f57b9a0df2c0336f506bcf5de9e1

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -2 lines) Patch
M third_party/WebKit/Source/build/scripts/make_computed_style_base.py View 1 chunk +1 line, -1 line 1 comment Download
M third_party/WebKit/Source/core/style/ComputedStyle.h View 1 chunk +1 line, -1 line 0 comments Download

Dependent Patchsets:

Messages

Total messages: 22 (11 generated)
shend
Hi Naina, PTAL
3 years, 7 months ago (2017-05-18 05:41:24 UTC) #4
nainar
lgtm
3 years, 7 months ago (2017-05-18 05:42:32 UTC) #5
shend
Hi Eddy, PTAL
3 years, 7 months ago (2017-05-18 10:31:52 UTC) #9
shend
Subbing in Alan for Eddy. PTAL :)
3 years, 7 months ago (2017-05-19 01:03:19 UTC) #11
alancutter (OOO until 2018)
Is there a generated diff here?
3 years, 7 months ago (2017-05-19 01:18:14 UTC) #12
shend
On 2017/05/19 at 01:18:14, alancutter wrote: > Is there a generated diff here? Attached diff.
3 years, 7 months ago (2017-05-19 01:27:20 UTC) #14
alancutter (OOO until 2018)
lgtm
3 years, 7 months ago (2017-05-19 01:55:07 UTC) #15
alancutter (OOO until 2018)
https://codereview.chromium.org/2890743003/diff/1/third_party/WebKit/Source/build/scripts/make_computed_style_base.py File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2890743003/diff/1/third_party/WebKit/Source/build/scripts/make_computed_style_base.py#newcode153 third_party/WebKit/Source/build/scripts/make_computed_style_base.py:153: self.internal_setter_method_name = method_name(join_name(setter_method_name, 'Internal')) Should the same apply to ...
3 years, 7 months ago (2017-05-19 01:55:23 UTC) #16
shend
On 2017/05/19 at 01:55:23, alancutter wrote: > https://codereview.chromium.org/2890743003/diff/1/third_party/WebKit/Source/build/scripts/make_computed_style_base.py > File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): > > https://codereview.chromium.org/2890743003/diff/1/third_party/WebKit/Source/build/scripts/make_computed_style_base.py#newcode153 ...
3 years, 7 months ago (2017-05-19 01:57:11 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2890743003/1
3 years, 7 months ago (2017-05-19 01:59:08 UTC) #19
commit-bot: I haz the power
3 years, 7 months ago (2017-05-19 02:06:59 UTC) #22
Message was sent while issue was closed.
Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/9f0d5e6a0694f57b9a0df2c0336f...

Powered by Google App Engine
This is Rietveld 408576698