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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 342883004: Using Enum in place of bool for better code readability (Closed)

Created:
6 years, 6 months ago by h.joshi
Modified:
6 years, 5 months ago
Reviewers:
Inactive, eae
CC:
blink-reviews, blink-reviews-css, ed+blinkwatch_opera.com, dglazkov+blink, apavlov+blink_chromium.org, darktears, rune+blink, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Code clean up, changing bool to Enum type for better code readability and some optimizations. (This is in reference to comments received in other patch) Patch handles following two cases: 1. Replace int with CSSValueID 2. Use new Enum in place of "bool shouldUseFixedDefaultSize" 3. Few Optimizations Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177314

Patch Set 1 #

Patch Set 2 : Replacing Bool with Emun for better code readabiliy #

Total comments: 12

Patch Set 3 : Comment fixes #

Total comments: 4

Patch Set 4 : Comment fixes #

Total comments: 2

Patch Set 5 : Comment fixes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -49 lines) Patch
M Source/core/css/CSSComputedStyleDeclaration.h View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M Source/core/css/CSSComputedStyleDeclaration.cpp View 1 2 3 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/css/FontSize.h View 1 2 chunks +5 lines, -2 lines 0 comments Download
M Source/core/css/FontSize.cpp View 1 2 2 chunks +17 lines, -10 lines 0 comments Download
M Source/core/css/resolver/FontBuilder.cpp View 1 2 3 7 chunks +12 lines, -10 lines 0 comments Download
M Source/core/editing/EditingStyle.h View 1 2 3 3 chunks +3 lines, -2 lines 0 comments Download
M Source/core/editing/EditingStyle.cpp View 1 2 3 12 chunks +18 lines, -19 lines 0 comments Download
M Source/platform/blink_platform.gypi View 1 1 chunk +1 line, -0 lines 0 comments Download
A Source/platform/fonts/FixedPitchFontType.h View 1 2 3 4 1 chunk +17 lines, -0 lines 0 comments Download
M Source/platform/fonts/FontDescription.h View 1 2 3 2 chunks +7 lines, -1 line 0 comments Download

Messages

Total messages: 16 (0 generated)
Inactive
https://codereview.chromium.org/342883004/diff/20001/Source/core/css/FontSize.cpp File Source/core/css/FontSize.cpp (right): https://codereview.chromium.org/342883004/diff/20001/Source/core/css/FontSize.cpp#newcode123 Source/core/css/FontSize.cpp:123: static int inline getRowFromMediumFontSizeInRange(const Settings* settings, bool quirksMode, FixedPitchFontType ...
6 years, 5 months ago (2014-06-26 19:43:11 UTC) #1
h.joshi
Made suggested changes, PTAL https://codereview.chromium.org/342883004/diff/20001/Source/core/css/FontSize.cpp File Source/core/css/FontSize.cpp (right): https://codereview.chromium.org/342883004/diff/20001/Source/core/css/FontSize.cpp#newcode123 Source/core/css/FontSize.cpp:123: static int inline getRowFromMediumFontSizeInRange(const Settings* ...
6 years, 5 months ago (2014-06-27 06:55:28 UTC) #2
Inactive
Once you fix those nits, I think you can go ahead and get this reviewed ...
6 years, 5 months ago (2014-06-27 12:57:53 UTC) #3
Inactive
Other CL description nits: - Please find a better title for your CL (once that ...
6 years, 5 months ago (2014-06-27 12:59:30 UTC) #4
h.joshi
Suggested changes done. https://codereview.chromium.org/342883004/diff/40001/Source/core/css/CSSComputedStyleDeclaration.cpp File Source/core/css/CSSComputedStyleDeclaration.cpp (right): https://codereview.chromium.org/342883004/diff/40001/Source/core/css/CSSComputedStyleDeclaration.cpp#newcode1201 Source/core/css/CSSComputedStyleDeclaration.cpp:1201: FixedPitchFontType CSSComputedStyleDeclaration::fixedPitchFont() const On 2014/06/27 12:57:53, ...
6 years, 5 months ago (2014-06-30 14:04:15 UTC) #5
h.joshi
@eae: PTAL
6 years, 5 months ago (2014-06-30 14:04:55 UTC) #6
eae
LGTM w/nit https://codereview.chromium.org/342883004/diff/60001/Source/platform/fonts/FixedPitchFontType.h File Source/platform/fonts/FixedPitchFontType.h (right): https://codereview.chromium.org/342883004/diff/60001/Source/platform/fonts/FixedPitchFontType.h#newcode17 Source/platform/fonts/FixedPitchFontType.h:17: #endif // GenericFamilyType_h FixedPitchFontType_h
6 years, 5 months ago (2014-06-30 15:41:30 UTC) #7
h.joshi
Thank you Emil, suggested changes are done. https://codereview.chromium.org/342883004/diff/60001/Source/platform/fonts/FixedPitchFontType.h File Source/platform/fonts/FixedPitchFontType.h (right): https://codereview.chromium.org/342883004/diff/60001/Source/platform/fonts/FixedPitchFontType.h#newcode17 Source/platform/fonts/FixedPitchFontType.h:17: #endif // ...
6 years, 5 months ago (2014-07-01 04:52:10 UTC) #8
h.joshi
The CQ bit was checked by h.joshi@samsung.com
6 years, 5 months ago (2014-07-01 04:55:30 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/342883004/80001
6 years, 5 months ago (2014-07-01 04:57:17 UTC) #10
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...
6 years, 5 months ago (2014-07-01 06:04:35 UTC) #11
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 5 months ago (2014-07-01 06:31:56 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/14239)
6 years, 5 months ago (2014-07-01 06:31:57 UTC) #13
h.joshi
The CQ bit was checked by h.joshi@samsung.com
6 years, 5 months ago (2014-07-01 14:02:13 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/342883004/80001
6 years, 5 months ago (2014-07-01 14:02:53 UTC) #15
commit-bot: I haz the power
6 years, 5 months ago (2014-07-01 15:06:42 UTC) #16
Message was sent while issue was closed. 
Change committed as 177314

Powered by Google App Engine
This is Rietveld 408576698