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

Issue 1505223004: Do not use assets for Ash window control button backgrounds (Closed)

Created:
5 years ago by tdanderson
Modified:
4 years, 11 months ago
CC:
chromium-reviews, kalyank, sadrul
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Do not use assets for Ash window control button backgrounds Paints the hovered/pressed background colors of the Ash frame caption buttons programmatically and deletes the now-obsolete asset files. Also introduces ash_layout_constants.cc in order to parameterize the constants which have different values for material design; this is used to specify the caption button dimensions which were previously given by assets. BUG=569062 TEST=ash_unittests.* Committed: https://crrev.com/b161d2c9aab0c7eff442ba9c90303d8bce22880f Cr-Commit-Position: refs/heads/master@{#367566}

Patch Set 1 #

Patch Set 2 : WIP for pkasting and estade feedback #

Total comments: 20

Patch Set 3 : ash layout constants file added #

Total comments: 28

Patch Set 4 : for owners review #

Total comments: 7

Patch Set 5 : for landing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+200 lines, -214 lines) Patch
M ash/ash.gyp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A ash/ash_layout_constants.h View 1 2 3 4 1 chunk +24 lines, -0 lines 0 comments Download
A ash/ash_layout_constants.cc View 1 2 3 4 1 chunk +36 lines, -0 lines 0 comments Download
M ash/frame/caption_buttons/frame_caption_button.h View 1 2 3 4 3 chunks +12 lines, -15 lines 0 comments Download
M ash/frame/caption_buttons/frame_caption_button.cc View 1 2 3 4 6 chunks +23 lines, -30 lines 0 comments Download
M ash/frame/caption_buttons/frame_caption_button_container_view.h View 1 2 3 3 chunks +9 lines, -21 lines 0 comments Download
M ash/frame/caption_buttons/frame_caption_button_container_view.cc View 1 2 3 4 chunks +15 lines, -42 lines 0 comments Download
M ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc View 1 2 3 4 6 chunks +12 lines, -12 lines 0 comments Download
M ash/frame/caption_buttons/frame_size_button_unittest.cc View 1 2 3 4 2 chunks +7 lines, -6 lines 0 comments Download
M ash/frame/custom_frame_view_ash_unittest.cc View 1 2 3 4 2 chunks +4 lines, -6 lines 0 comments Download
M ash/frame/default_header_painter.cc View 1 2 3 4 4 chunks +15 lines, -23 lines 0 comments Download
M ash/resources/ash_resources.grd View 1 1 chunk +0 lines, -2 lines 0 comments Download
D ash/resources/default_100_percent/common/window_control_background_hover.png View 1 Binary file 0 comments Download
D ash/resources/default_100_percent/common/window_control_background_pressed.png View 1 Binary file 0 comments Download
D ash/resources/default_200_percent/common/window_control_background_hover.png View 1 Binary file 0 comments Download
D ash/resources/default_200_percent/common/window_control_background_pressed.png View 1 Binary file 0 comments Download
D chrome/app/theme/default_100_percent/common/ash/browser_window_control_background_maximized_hover.png View 1 Binary file 0 comments Download
D chrome/app/theme/default_100_percent/common/ash/browser_window_control_background_maximized_pressed.png View 1 Binary file 0 comments Download
D chrome/app/theme/default_100_percent/common/ash/browser_window_control_background_restored_hover.png View 1 Binary file 0 comments Download
D chrome/app/theme/default_100_percent/common/ash/browser_window_control_background_restored_pressed.png View 1 Binary file 0 comments Download
D chrome/app/theme/default_200_percent/common/ash/browser_window_control_background_maximized_hover.png View 1 Binary file 0 comments Download
D chrome/app/theme/default_200_percent/common/ash/browser_window_control_background_maximized_pressed.png View 1 Binary file 0 comments Download
D chrome/app/theme/default_200_percent/common/ash/browser_window_control_background_restored_hover.png View 1 Binary file 0 comments Download
D chrome/app/theme/default_200_percent/common/ash/browser_window_control_background_restored_pressed.png View 1 Binary file 0 comments Download
M chrome/app/theme/theme_resources.grd View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_header_painter_ash.h View 1 2 3 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_header_painter_ash.cc View 1 2 3 4 3 chunks +24 lines, -43 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/frame/web_app_left_header_view_ash.cc View 1 2 2 chunks +6 lines, -8 lines 0 comments Download

Messages

Total messages: 25 (9 generated)
tdanderson
Peter and Evan, can you please take a look at my inline questions and let ...
5 years ago (2015-12-17 18:00:55 UTC) #3
Peter Kasting
Only replying to your questions, did not do a full review. https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_buttons/frame_caption_button.cc File ash/frame/caption_buttons/frame_caption_button.cc (right): ...
5 years ago (2015-12-17 20:56:08 UTC) #4
Evan Stade
https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_buttons/frame_caption_button.cc File ash/frame/caption_buttons/frame_caption_button.cc (right): https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_buttons/frame_caption_button.cc#newcode112 ash/frame/caption_buttons/frame_caption_button.cc:112: color = SkColorSetA(color, hovered_background_alpha); hmm, it seems like you ...
5 years ago (2015-12-17 21:52:37 UTC) #5
tdanderson
Thanks for the feedback, replies inline. Peter, mind taking another look (especially at how I ...
5 years ago (2015-12-18 17:42:53 UTC) #6
Peter Kasting
https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_buttons/frame_caption_button.cc File ash/frame/caption_buttons/frame_caption_button.cc (right): https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_buttons/frame_caption_button.cc#newcode112 ash/frame/caption_buttons/frame_caption_button.cc:112: color = SkColorSetA(color, hovered_background_alpha); On 2015/12/18 17:42:53, tdanderson wrote: ...
5 years ago (2015-12-18 18:59:15 UTC) #7
Evan Stade
https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_buttons/frame_caption_button.cc File ash/frame/caption_buttons/frame_caption_button.cc (right): https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_buttons/frame_caption_button.cc#newcode112 ash/frame/caption_buttons/frame_caption_button.cc:112: color = SkColorSetA(color, hovered_background_alpha); On 2015/12/18 18:59:15, Peter Kasting ...
5 years ago (2015-12-18 19:12:04 UTC) #8
Peter Kasting
https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_buttons/frame_caption_button.cc File ash/frame/caption_buttons/frame_caption_button.cc (right): https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_buttons/frame_caption_button.cc#newcode112 ash/frame/caption_buttons/frame_caption_button.cc:112: color = SkColorSetA(color, hovered_background_alpha); On 2015/12/18 19:12:04, Evan Stade ...
5 years ago (2015-12-18 19:36:38 UTC) #9
tdanderson
Scott, can you please take a look? Peter and Evan, I have addressed your comments ...
4 years, 11 months ago (2016-01-04 23:11:15 UTC) #13
Evan Stade
(looked around a bit but didn't do a full review, no need to wait for ...
4 years, 11 months ago (2016-01-04 23:53:17 UTC) #14
Peter Kasting
LGTM
4 years, 11 months ago (2016-01-05 00:29:35 UTC) #15
sky
LGTM https://codereview.chromium.org/1505223004/diff/60001/ash/ash_layout_constants.h File ash/ash_layout_constants.h (right): https://codereview.chromium.org/1505223004/diff/60001/ash/ash_layout_constants.h#newcode11 ash/ash_layout_constants.h:11: enum AshLayoutSize { nit: use enum class so ...
4 years, 11 months ago (2016-01-05 00:49:16 UTC) #16
tdanderson
https://codereview.chromium.org/1505223004/diff/60001/ash/ash_layout_constants.h File ash/ash_layout_constants.h (right): https://codereview.chromium.org/1505223004/diff/60001/ash/ash_layout_constants.h#newcode11 ash/ash_layout_constants.h:11: enum AshLayoutSize { On 2016/01/05 00:49:16, sky wrote: > ...
4 years, 11 months ago (2016-01-05 16:47:27 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1505223004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1505223004/80001
4 years, 11 months ago (2016-01-05 16:48:07 UTC) #20
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 11 months ago (2016-01-05 17:50:50 UTC) #22
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/b161d2c9aab0c7eff442ba9c90303d8bce22880f Cr-Commit-Position: refs/heads/master@{#367566}
4 years, 11 months ago (2016-01-05 17:51:52 UTC) #24
Evan Stade
4 years, 11 months ago (2016-01-05 19:41:57 UTC) #25
Message was sent while issue was closed.
https://codereview.chromium.org/1505223004/diff/60001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/1505223004/diff/60001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:107: if
(hover_animation().is_animating() || state() == STATE_HOVERED) {
On 2016/01/05 16:47:26, tdanderson wrote:
> On 2016/01/04 23:53:17, Evan Stade wrote:
> > further refined as:
> > 
> > SkAlpha bg_alpha = SK_AlphaTRANSPARENT;
> > if (hover_animation().is_animating())
> >   bg_alpha = hover_animation().CurrentValueBetween(0, kHoveredAlpha));
> > else if (state() == STATE_HOVERED)
> >   bg_alpha = kHoveredAlpha;
> > else if (state() == STATE_PRESSED)
> >   bg_alpha = kPressedAlpha;
> > 
> > if (bg_alpha != SK_AlphaTRANSPARENT)
> >   canvas->DrawColor(SkColorSetA(kHoveredPressedColor, bg_alpha));
> 
> Done. (Though technically this would cause a problem if we ever changed either
> of the alphas to be transparent.)

how so? if fully transparent, we might as well not draw anything

Powered by Google App Engine
This is Rietveld 408576698