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

Issue 2668393003: Fix using theme colors for the download shelf's close button. (Closed)

Created:
3 years, 10 months ago by Sidney San Martín
Modified:
3 years, 10 months ago
Reviewers:
Robert Sesek, spqchan
CC:
chromium-reviews, asanka, dbeam+watch-downloads_chromium.org, mac-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix using theme colors for the download shelf's close button. The theme world is messy: - ThemeProvider encapsulates some state like incognito status, but not other state like whether the active or inactive version of a color should be used (e.g. COLOR_TAB_TEXT vs. COLOR_BACKGROUND_TAB_TEXT). - A browser window always(?) has a ThemeProvider, but since some views try to access theme information when they're not in a window, there are null checks all over. - Some views listen for theme callbacks. Others, like HoverCloseButton, expect to be told to redraw when the theme changes. - -[TabView iconColor] modifies a theme color by tweaking its alpha and wants its children to use it. It also returns a hard-coded color that isn't in the theme at all for "dark" themes. This isn't really supported by the theme system. The issue that this CL fixes was the result of HoverCloseButton expecting to be a child of a TabView and falling back to a default color. This fix doesn't really change that dependency, but it lets anyone set the color instead of assuming a TabView parent. It will still use a default color if nobody tells it otherwise, since it doesn't look at the theme itself. A more complex solution would let HoverCloseButton pull a color from the theme but allow that color to be overridden (by TabView), but that doesn't seem worth it. I'd be a fan of rethinking details of how we deal with themes to make issues like this harder to come by. BUG=656553 Review-Url: https://codereview.chromium.org/2668393003 Cr-Commit-Position: refs/heads/master@{#447826} Committed: https://chromium.googlesource.com/chromium/src/+/524a361c86c23bb4f5384c1738f786532a296e46

Patch Set 1 #

Total comments: 4

Patch Set 2 : Comment iconColor, move the default icon color into a constant. #

Total comments: 2

Patch Set 3 : Delete more unused includes. #

Patch Set 4 : Add @private. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -25 lines) Patch
M chrome/app/nibs/DownloadShelf.xib View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm View 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/hover_close_button.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/hover_close_button.mm View 1 2 6 chunks +13 lines, -17 lines 0 comments Download
M chrome/browser/ui/cocoa/tabs/tab_view.h View 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/tabs/tab_view.mm View 3 chunks +6 lines, -4 lines 0 comments Download

Messages

Total messages: 24 (18 generated)
spqchan
lgtm with a couple of nits https://codereview.chromium.org/2668393003/diff/1/chrome/browser/ui/cocoa/hover_close_button.h File chrome/browser/ui/cocoa/hover_close_button.h (right): https://codereview.chromium.org/2668393003/diff/1/chrome/browser/ui/cocoa/hover_close_button.h#newcode23 chrome/browser/ui/cocoa/hover_close_button.h:23: @property(nonatomic) SkColor iconColor; ...
3 years, 10 months ago (2017-02-02 17:36:44 UTC) #7
Sidney San Martín
rsesek@: PTAL spqchan@: Thanks! Fixed. I didn't expect you to see this before I published ...
3 years, 10 months ago (2017-02-02 18:58:14 UTC) #10
Robert Sesek
LGTM w/ a nit https://codereview.chromium.org/2668393003/diff/20001/chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h File chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h (right): https://codereview.chromium.org/2668393003/diff/20001/chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h#newcode17 chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h:17: @interface DownloadShelfView : AnimatableView { ...
3 years, 10 months ago (2017-02-02 19:32:24 UTC) #15
Sidney San Martín
Thanks! https://codereview.chromium.org/2668393003/diff/20001/chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h File chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h (right): https://codereview.chromium.org/2668393003/diff/20001/chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h#newcode17 chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h:17: @interface DownloadShelfView : AnimatableView { On 2017/02/02 19:32:24, ...
3 years, 10 months ago (2017-02-02 19:41:12 UTC) #18
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/2668393003/60001
3 years, 10 months ago (2017-02-02 19:42:07 UTC) #21
commit-bot: I haz the power
3 years, 10 months ago (2017-02-02 20:05:11 UTC) #24
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/524a361c86c23bb4f5384c1738f7...

Powered by Google App Engine
This is Rietveld 408576698