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

Issue 2382443007: Clean up NativeTheme (particularly CommonTheme). (Closed)

Created:
4 years, 2 months ago by Evan Stade
Modified:
4 years, 2 months ago
Reviewers:
tdanderson, sky
CC:
chromium-reviews, tfarina, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, Peter Kasting
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Clean up NativeTheme (particularly CommonTheme). - discard pre-MD colors - consolidate color logic that didn't change from pre-MD to MD - remove some colors that either aren't used, or are only used in one place, or otherwise don't make sense to be part of NativeTheme BUG=648281 Committed: https://crrev.com/f02568a216b975130849079183cc0f7173d6b832 Cr-Commit-Position: refs/heads/master@{#422665}

Patch Set 1 #

Patch Set 2 : two slight fixes #

Total comments: 25

Patch Set 3 : tdandersonreview #

Total comments: 13

Patch Set 4 : share color constant #

Unified diffs Side-by-side diffs Delta from patch set Stats (+110 lines, -325 lines) Patch
M chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm View 1 2 3 2 chunks +3 lines, -5 lines 0 comments Download
M chrome/browser/ui/libgtk2ui/gtk2_ui.cc View 3 chunks +4 lines, -7 lines 0 comments Download
M chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc View 4 chunks +0 lines, -12 lines 0 comments Download
M chrome/browser/ui/profile_chooser_constants.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc View 2 chunks +3 lines, -5 lines 0 comments Download
M chrome/browser/ui/views/profiles/profile_chooser_view.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/toolbar/app_menu.h View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/ui/views/toolbar/app_menu.cc View 1 2 3 3 chunks +5 lines, -19 lines 0 comments Download
M chrome/browser/ui/views/toolbar/toolbar_action_view.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M ui/native_theme/common_theme.cc View 1 2 3 5 chunks +66 lines, -208 lines 0 comments Download
M ui/native_theme/native_theme.h View 3 chunks +0 lines, -6 lines 0 comments Download
M ui/native_theme/native_theme_dark_aura.cc View 5 chunks +0 lines, -12 lines 0 comments Download
M ui/native_theme/native_theme_mac.mm View 4 chunks +1 line, -11 lines 0 comments Download
M ui/native_theme/native_theme_win.cc View 3 chunks +0 lines, -8 lines 0 comments Download
M ui/views/controls/button/label_button_unittest.cc View 1 chunk +8 lines, -7 lines 0 comments Download
M ui/views/controls/label.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M ui/views/controls/menu/menu_delegate.h View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M ui/views/controls/menu/menu_delegate.cc View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M ui/views/controls/menu/menu_item_view.cc View 1 2 3 1 chunk +5 lines, -6 lines 0 comments Download
M ui/views/style/platform_style.cc View 1 1 chunk +0 lines, -4 lines 0 comments Download
M ui/views/style/platform_style_mac.mm View 1 1 chunk +1 line, -3 lines 0 comments Download

Messages

Total messages: 33 (21 generated)
Evan Stade
I've added comments to clarify/justify some of the less obvious changes. https://codereview.chromium.org/2382443007/diff/20001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (left): ...
4 years, 2 months ago (2016-09-30 18:02:36 UTC) #8
Evan Stade
Actually, to better balance review load, perhaps Terry should take a look at this instead ...
4 years, 2 months ago (2016-09-30 18:14:01 UTC) #12
tdanderson
lgtm with some comments below. https://chromiumcodereview.appspot.com/2382443007/diff/20001/chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (left): https://chromiumcodereview.appspot.com/2382443007/diff/20001/chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc#oldcode327 chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:327: // return GetTextColor(GetButton(), NORMAL); ...
4 years, 2 months ago (2016-09-30 21:00:56 UTC) #15
Evan Stade
On 2016/09/30 21:00:56, tdanderson wrote: > lgtm with some comments below. > > https://chromiumcodereview.appspot.com/2382443007/diff/20001/chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc > ...
4 years, 2 months ago (2016-09-30 22:18:06 UTC) #16
Evan Stade
+sky for owners https://codereview.chromium.org/2382443007/diff/20001/chrome/browser/ui/views/profiles/profile_chooser_view.cc File chrome/browser/ui/views/profiles/profile_chooser_view.cc (left): https://codereview.chromium.org/2382443007/diff/20001/chrome/browser/ui/views/profiles/profile_chooser_view.cc#oldcode224 chrome/browser/ui/views/profiles/profile_chooser_view.cc:224: ui::NativeTheme::kColorId_ButtonHoverBackgroundColor)); On 2016/09/30 21:00:56, tdanderson wrote: ...
4 years, 2 months ago (2016-09-30 22:19:00 UTC) #18
tdanderson
https://codereview.chromium.org/2382443007/diff/20001/ui/views/controls/button/label_button_unittest.cc File ui/views/controls/button/label_button_unittest.cc (right): https://codereview.chromium.org/2382443007/diff/20001/ui/views/controls/button/label_button_unittest.cc#newcode92 ui/views/controls/button/label_button_unittest.cc:92: styled_highlight_text_color_ = styled_normal_text_color_ = On 2016/09/30 22:19:00, Evan Stade ...
4 years, 2 months ago (2016-09-30 22:39:37 UTC) #21
sky
https://codereview.chromium.org/2382443007/diff/40001/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2382443007/diff/40001/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm#newcode851 chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:851: // TODO(estade): share this with the Views version of ...
4 years, 2 months ago (2016-10-03 16:12:05 UTC) #24
Evan Stade
https://codereview.chromium.org/2382443007/diff/40001/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2382443007/diff/40001/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm#newcode851 chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:851: // TODO(estade): share this with the Views version of ...
4 years, 2 months ago (2016-10-03 19:46:44 UTC) #25
sky
LGTM https://codereview.chromium.org/2382443007/diff/40001/ui/views/style/platform_style_mac.mm File ui/views/style/platform_style_mac.mm (right): https://codereview.chromium.org/2382443007/diff/40001/ui/views/style/platform_style_mac.mm#newcode84 ui/views/style/platform_style_mac.mm:84: colors[Button::STATE_PRESSED] = SK_ColorWHITE; On 2016/10/03 19:46:44, Evan Stade ...
4 years, 2 months ago (2016-10-03 21:25:15 UTC) #26
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/2382443007/60001
4 years, 2 months ago (2016-10-04 00:25:50 UTC) #29
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 2 months ago (2016-10-04 01:58:39 UTC) #31
commit-bot: I haz the power
4 years, 2 months ago (2016-10-04 02:02:40 UTC) #33
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/f02568a216b975130849079183cc0f7173d6b832
Cr-Commit-Position: refs/heads/master@{#422665}

Powered by Google App Engine
This is Rietveld 408576698