Do not use assets for Ash window control button backgrounds

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}

[tdanderson, 2015-12-17 16:36:00]

Description was changed from

==========
[WIP] Do not use assets for Ash window control button backgrounds

WIP.

BUG=TBA
==========

to

==========
[WIP] Do not use assets for Ash window control button backgrounds

WIP.

BUG=569062
==========

[tdanderson, 2015-12-17 18:00:54]

tdanderson@chromium.org changed reviewers:
+ estade@chromium.org, pkasting@chromium.org

[tdanderson, 2015-12-17 18:00:55]

Peter and Evan, can you please take a look at my inline questions and let me
know your thoughts? Thanks.

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:29: const SkColor
kBackgroundPressedColor = SkColorSetARGB(0x24, 0x0, 0x0, 0x0);
Evan, are we in a position now where I can move these colors out of here and use
ThemeProvider to access them?

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
File ash/frame/default_header_painter.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:53: const int kButtonHeight = 33;
Peter, I'd like to not have these hard-coded here (and in the _unittests files)
but it seems #including layout_constants in ash/ would constitute a layering
violation. Do you have any suggestions other than giving ash its own separate
layout_constants?

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:114: gfx::Size(kButtonWidth,
kButtonHeight));
Peter, WDYT about adding a GetLayoutSize() to the layout_constants interface?

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:316: ?
IDR_AURA_WINDOW_CONTROL_ICON_CLOSE_WHITE
Evan, if this CL landed as-is, am I leaving things in a convenient state for you
to (a) vectorize the frame icons and replace the IDR_... IDs with VectorIcon
IDs, and (b) have the icon and hover/pressed background colors change as needed
for incognito?

https://codereview.chromium.org/1505223004/diff/20001/chrome/browser/ui/layou...
File chrome/browser/ui/layout_constants.h (right):

https://codereview.chromium.org/1505223004/diff/20001/chrome/browser/ui/layou...
chrome/browser/ui/layout_constants.h:12:
ASH_BROWSER_MAXIMIZED_HEADER_BUTTON_HEIGHT,
Peter, is it appropriate to put these here since they are Ash-specific?

[Peter Kasting, 2015-12-17 20:56:08]

Only replying to your questions, did not do a full review.

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:28: const SkColor
kBackgroundHoveredColor = SkColorSetARGB(0x14, 0x0, 0x0, 0x0);
Nit: Use SkColorSetA(SK_ColorBLACK, 0x14)

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
File ash/frame/default_header_painter.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:53: const int kButtonHeight = 33;
On 2015/12/17 18:00:54, tdanderson wrote:
> Peter, I'd like to not have these hard-coded here (and in the _unittests
files)
> but it seems #including layout_constants in ash/ would constitute a layering
> violation. Do you have any suggestions other than giving ash its own separate
> layout_constants?

I would create a separate set of layout constants in ash/.  That also allows you
to avoid putting Ash-only constants in GetLayoutConstant().

If you need to avoid a naming conflict you could name your new function
GetAshLayoutConstant().  (This suggests to me that the solution to the problem
of "we hoisted all the views-specific layout constants to c/b/ui/" is to divide
the file and call the views-specific thing GetViewsLayoutConstant().)

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:114: gfx::Size(kButtonWidth,
kButtonHeight));
On 2015/12/17 18:00:54, tdanderson wrote:
> Peter, WDYT about adding a GetLayoutSize() to the layout_constants interface?

Seems fine to me.

https://codereview.chromium.org/1505223004/diff/20001/chrome/browser/ui/layou...
File chrome/browser/ui/layout_constants.h (right):

https://codereview.chromium.org/1505223004/diff/20001/chrome/browser/ui/layou...
chrome/browser/ui/layout_constants.h:12:
ASH_BROWSER_MAXIMIZED_HEADER_BUTTON_HEIGHT,
On 2015/12/17 18:00:54, tdanderson wrote:
> Peter, is it appropriate to put these here since they are Ash-specific?

Covered in my other reply.

[Evan Stade, 2015-12-17 21:52:37]

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:112: color =
SkColorSetA(color, hovered_background_alpha);
hmm, it seems like you have two distinct values here:
- HoveredColor (black)
- HoveredAlpha (0x14)

You can combine them into one constant, but IMO this code is a bit easier to
follow as

int hovered_bg_alpha = hover_animation_->is_animating() ?
hover_animation_->CurrentValueBetween(0, HoveredAlpha) : HoveredAlpha;
canvas->DrawColor(SkColorSetA(HoveredColor, hovered_bg_alpha));

p.s. Seems like the ternary shouldn't be necessary. If 1.0 * value != value,
CurrentValueBetween should do this checking instead.

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
File ash/frame/default_header_painter.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:316: ?
IDR_AURA_WINDOW_CONTROL_ICON_CLOSE_WHITE
On 2015/12/17 18:00:54, tdanderson wrote:
> Evan, if this CL landed as-is, am I leaving things in a convenient state for
you
> to (a) vectorize the frame icons and replace the IDR_... IDs with VectorIcon
> IDs, and (b) have the icon and hover/pressed background colors change as
needed
> for incognito?

I can't see how this gets us any farther away from that goal, so sure, go ahead.

[tdanderson, 2015-12-18 17:42:53]

Thanks for the feedback, replies inline.

Peter, mind taking another look (especially at how I have re-organized the Ash
constants) before I send this off to an ash/ OWNER?

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:28: const SkColor
kBackgroundHoveredColor = SkColorSetARGB(0x14, 0x0, 0x0, 0x0);
On 2015/12/17 20:56:08, Peter Kasting wrote:
> Nit: Use SkColorSetA(SK_ColorBLACK, 0x14)

Done.

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:112: color =
SkColorSetA(color, hovered_background_alpha);
On 2015/12/17 21:52:37, Evan Stade wrote:
> hmm, it seems like you have two distinct values here:
> - HoveredColor (black)
> - HoveredAlpha (0x14)
> 
> You can combine them into one constant, but IMO this code is a bit easier to
> follow as
> 
> int hovered_bg_alpha = hover_animation_->is_animating() ?
> hover_animation_->CurrentValueBetween(0, HoveredAlpha) : HoveredAlpha;
> canvas->DrawColor(SkColorSetA(HoveredColor, hovered_bg_alpha));

Done.

> 
> p.s. Seems like the ternary shouldn't be necessary. If 1.0 * value != value,
> CurrentValueBetween should do this checking instead.

To clarify this is a comment on how CurrentValueBetween() should ideally work,
and not a suggestion to change my implementation here?

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
File ash/frame/default_header_painter.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:53: const int kButtonHeight = 33;
On 2015/12/17 20:56:08, Peter Kasting wrote:
> On 2015/12/17 18:00:54, tdanderson wrote:
> > Peter, I'd like to not have these hard-coded here (and in the _unittests
> files)
> > but it seems #including layout_constants in ash/ would constitute a layering
> > violation. Do you have any suggestions other than giving ash its own
separate
> > layout_constants?
> 
> I would create a separate set of layout constants in ash/.  That also allows
you
> to avoid putting Ash-only constants in GetLayoutConstant().
> 
> If you need to avoid a naming conflict you could name your new function
> GetAshLayoutConstant().  (This suggests to me that the solution to the problem
> of "we hoisted all the views-specific layout constants to c/b/ui/" is to
divide
> the file and call the views-specific thing GetViewsLayoutConstant().)

Done.

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:114: gfx::Size(kButtonWidth,
kButtonHeight));
On 2015/12/17 20:56:08, Peter Kasting wrote:
> On 2015/12/17 18:00:54, tdanderson wrote:
> > Peter, WDYT about adding a GetLayoutSize() to the layout_constants
interface?
> 
> Seems fine to me.

Acknowledged.

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/default_heade...
ash/frame/default_header_painter.cc:316: ?
IDR_AURA_WINDOW_CONTROL_ICON_CLOSE_WHITE
On 2015/12/17 21:52:37, Evan Stade wrote:
> On 2015/12/17 18:00:54, tdanderson wrote:
> > Evan, if this CL landed as-is, am I leaving things in a convenient state for
> you
> > to (a) vectorize the frame icons and replace the IDR_... IDs with VectorIcon
> > IDs, and (b) have the icon and hover/pressed background colors change as
> needed
> > for incognito?
> 
> I can't see how this gets us any farther away from that goal, so sure, go
ahead.

Acknowledged.

https://codereview.chromium.org/1505223004/diff/20001/chrome/browser/ui/layou...
File chrome/browser/ui/layout_constants.h (right):

https://codereview.chromium.org/1505223004/diff/20001/chrome/browser/ui/layou...
chrome/browser/ui/layout_constants.h:12:
ASH_BROWSER_MAXIMIZED_HEADER_BUTTON_HEIGHT,
On 2015/12/17 20:56:08, Peter Kasting wrote:
> On 2015/12/17 18:00:54, tdanderson wrote:
> > Peter, is it appropriate to put these here since they are Ash-specific?
> 
> Covered in my other reply.

Acknowledged.

[Peter Kasting, 2015-12-18 18:59:15]

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:112: color =
SkColorSetA(color, hovered_background_alpha);
On 2015/12/18 17:42:53, tdanderson wrote:
> On 2015/12/17 21:52:37, Evan Stade wrote:
> > hmm, it seems like you have two distinct values here:
> > - HoveredColor (black)
> > - HoveredAlpha (0x14)
> > 
> > You can combine them into one constant, but IMO this code is a bit easier to
> > follow as
> > 
> > int hovered_bg_alpha = hover_animation_->is_animating() ?
> > hover_animation_->CurrentValueBetween(0, HoveredAlpha) : HoveredAlpha;
> > canvas->DrawColor(SkColorSetA(HoveredColor, hovered_bg_alpha));
> 
> Done.
> 
> > 
> > p.s. Seems like the ternary shouldn't be necessary. If 1.0 * value != value,
> > CurrentValueBetween should do this checking instead.
> 
> To clarify this is a comment on how CurrentValueBetween() should ideally work,
> and not a suggestion to change my implementation here?

No, he's suggesting changing your implementation.

That suggestion is only correct if when the hover animation isn't animating its
value is 1, or else in the cases where its value is 0 you actually want fully
transparent and not the normal alpha from the constant.  That said, if that
isn't the case today, then things are confusing, because the animation value can
be out of sync with your desired alpha, so I'd fix things so that the comment
applies.

https://codereview.chromium.org/1505223004/diff/40001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/ash.gyp#newcode37
ash/ash.gyp:37: 'ash_layout_constants.h',
Don't you need to touch a .gn file also?

https://codereview.chromium.org/1505223004/diff/40001/ash/ash_layout_constants.h
File ash/ash_layout_constants.h (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/ash_layout_constant...
ash/ash_layout_constants.h:11: // Size of a header button in a maximized browser
window.
At least on Windows, these are called "caption buttons".  Unless Ash
consistently refers to them as "header buttons", I'd use that terminology
everywhere.

https://codereview.chromium.org/1505223004/diff/40001/ash/ash_layout_constant...
ash/ash_layout_constants.h:18: DEFAULT_HEADER_BUTTON,
Nit: I might name this "NON_BROWSER_" instead of "DEFAULT_" since browser
windows are much more common than non-browser windows, and it's kinda weird to
have "default" for the minority case.

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:30: const SkColor
kHoveredColor = SK_ColorBLACK;
Nit: It seems OK to me to just hardcode SK_ColorBLACK below to avoid two color
constants with the same value (or just declare one such color constant).

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:43: size_(gfx::Size()),
No need for this

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.h (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.h:86: // The image and image id
used to paint the button's icon.
Nit: "id and image" to make the comment order these like the declaration order.

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:160: if
(buttons[i]->icon() == icon) {
Nit: {} not necessary

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:216: 
Nit: I wouldn't add this blank line

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:301:
std::map<CaptionButtonIcon, int>::const_iterator it =
Nit: Use auto.

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button_container_view.h (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button_container_view.h:139: // Mapping
of the image needed to paint a button for each of the values of
Nit: "image ID" maybe?

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:416: }
Nit: Shorter:

  int size_icon_id = IDR_AURA_WINDOW_CONTROL_ICON_MAXIMIZE;
  gfx::Size button_size(GetAshLayoutSize(BROWSER_RESTORED_HEADER_BUTTON));
  if (frame_->IsMaximized() || frame_->IsFullscreen()) {
    size_icon_id = IDR_AURA_WINDOW_CONTROL_ICON_RESTORE;
    button_size = GetAshLayoutSize(BROWSER_MAXIMIZED_HEADER_BUTTON);
  }

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:420:
caption_button_container_->SetButtonSize(button_size);
Nit: I'd put this call either above or below all the SetButtonImage() calls.

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.h (right):

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.h:84: // Updates the
size and icons used for the minimize, restore,
Nit: Wrap as close to 80 columns as possible

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:185: } else {
Nit: No else after return.  (This also allows undoing the reindenting to the
expression below that makes it harder to read.)

[Evan Stade, 2015-12-18 19:12:04]

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:112: color =
SkColorSetA(color, hovered_background_alpha);
On 2015/12/18 18:59:15, Peter Kasting wrote:
> On 2015/12/18 17:42:53, tdanderson wrote:
> > On 2015/12/17 21:52:37, Evan Stade wrote:
> > > hmm, it seems like you have two distinct values here:
> > > - HoveredColor (black)
> > > - HoveredAlpha (0x14)
> > > 
> > > You can combine them into one constant, but IMO this code is a bit easier
to
> > > follow as
> > > 
> > > int hovered_bg_alpha = hover_animation_->is_animating() ?
> > > hover_animation_->CurrentValueBetween(0, HoveredAlpha) : HoveredAlpha;
> > > canvas->DrawColor(SkColorSetA(HoveredColor, hovered_bg_alpha));
> > 
> > Done.
> > 
> > > 
> > > p.s. Seems like the ternary shouldn't be necessary. If 1.0 * value !=
value,
> > > CurrentValueBetween should do this checking instead.
> > 
> > To clarify this is a comment on how CurrentValueBetween() should ideally
work,
> > and not a suggestion to change my implementation here?
> 
> No, he's suggesting changing your implementation.
> 
> That suggestion is only correct if when the hover animation isn't animating
its
> value is 1, or else in the cases where its value is 0 you actually want fully
> transparent and not the normal alpha from the constant.  That said, if that
> isn't the case today, then things are confusing, because the animation value
can
> be out of sync with your desired alpha, so I'd fix things so that the comment
> applies.

Assuming I'm understanding how this all works (and the motivation for the
ternary[1]), I was suggesting changing CurrentValueBetween() which would then
allow you to change this line as well. I do believe hover_animation_'s current
value will always be the intuitive value, based on [1], but I guess it's
possible that's not the case so it should be checked.

[1] I assumed the motivation was concern about floating point arithmetic
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

[Peter Kasting, 2015-12-18 19:36:38]

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/1505223004/diff/20001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:112: color =
SkColorSetA(color, hovered_background_alpha);
On 2015/12/18 19:12:04, Evan Stade wrote:
> On 2015/12/18 18:59:15, Peter Kasting wrote:
> > On 2015/12/18 17:42:53, tdanderson wrote:
> > > On 2015/12/17 21:52:37, Evan Stade wrote:
> > > > hmm, it seems like you have two distinct values here:
> > > > - HoveredColor (black)
> > > > - HoveredAlpha (0x14)
> > > > 
> > > > You can combine them into one constant, but IMO this code is a bit
easier
> to
> > > > follow as
> > > > 
> > > > int hovered_bg_alpha = hover_animation_->is_animating() ?
> > > > hover_animation_->CurrentValueBetween(0, HoveredAlpha) : HoveredAlpha;
> > > > canvas->DrawColor(SkColorSetA(HoveredColor, hovered_bg_alpha));
> > > 
> > > Done.
> > > 
> > > > 
> > > > p.s. Seems like the ternary shouldn't be necessary. If 1.0 * value !=
> value,
> > > > CurrentValueBetween should do this checking instead.
> > > 
> > > To clarify this is a comment on how CurrentValueBetween() should ideally
> work,
> > > and not a suggestion to change my implementation here?
> > 
> > No, he's suggesting changing your implementation.
> > 
> > That suggestion is only correct if when the hover animation isn't animating
> its
> > value is 1, or else in the cases where its value is 0 you actually want
fully
> > transparent and not the normal alpha from the constant.  That said, if that
> > isn't the case today, then things are confusing, because the animation value
> can
> > be out of sync with your desired alpha, so I'd fix things so that the
comment
> > applies.
> 
> Assuming I'm understanding how this all works (and the motivation for the
> ternary[1]), I was suggesting changing CurrentValueBetween() which would then
> allow you to change this line as well. I do believe hover_animation_'s current
> value will always be the intuitive value, based on [1], but I guess it's
> possible that's not the case so it should be checked.
> 
> [1] I assumed the motivation was concern about floating point arithmetic

Oh.  I assumed the motivation was that it's possible for the hover animation to
have a value other than 1 when not animating, but that we still wanted those
cases to use the final opaque alpha value.

If whoever wrote the original code was worried about floating-point arithmetic
they need not be -- 1.0 * x is always equal to x as long as x isn't NaN or some
similar thing.  I don't think any change in CurrentValueBetween() is necessary,
because its current functionality will always do the right thing.

[tdanderson, 2016-01-04 23:07:25]

Description was changed from

==========
[WIP] Do not use assets for Ash window control button backgrounds

WIP.

BUG=569062
==========

to

==========
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.

BUG=569062
TEST=ash_unittests.*
==========

[tdanderson, 2016-01-04 23:10:19]

Description was changed from

==========
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.

BUG=569062
TEST=ash_unittests.*
==========

to

==========
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.*
==========

[tdanderson, 2016-01-04 23:11:14]

tdanderson@chromium.org changed reviewers:
+ sky@chromium.org

[tdanderson, 2016-01-04 23:11:15]

Scott, can you please take a look?

Peter and Evan, I have addressed your comments on my previous patch set.

https://codereview.chromium.org/1505223004/diff/40001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/ash.gyp#newcode37
ash/ash.gyp:37: 'ash_layout_constants.h',
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Don't you need to touch a .gn file also?

Looking at BUILD.gn in src/ash, it doesn't seem like I need to change anything
for gn (the gn trybots are green, too).

https://codereview.chromium.org/1505223004/diff/40001/ash/ash_layout_constants.h
File ash/ash_layout_constants.h (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/ash_layout_constant...
ash/ash_layout_constants.h:11: // Size of a header button in a maximized browser
window.
On 2015/12/18 18:59:15, Peter Kasting wrote:
> At least on Windows, these are called "caption buttons".  Unless Ash
> consistently refers to them as "header buttons", I'd use that terminology
> everywhere.

Done.

https://codereview.chromium.org/1505223004/diff/40001/ash/ash_layout_constant...
ash/ash_layout_constants.h:18: DEFAULT_HEADER_BUTTON,
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: I might name this "NON_BROWSER_" instead of "DEFAULT_" since browser
> windows are much more common than non-browser windows, and it's kinda weird to
> have "default" for the minority case.

Done.

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:30: const SkColor
kHoveredColor = SK_ColorBLACK;
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: It seems OK to me to just hardcode SK_ColorBLACK below to avoid two color
> constants with the same value (or just declare one such color constant).

Done.

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.cc:43: size_(gfx::Size()),
On 2015/12/18 18:59:15, Peter Kasting wrote:
> No need for this

Done.

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.h (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.h:86: // The image and image id
used to paint the button's icon.
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: "id and image" to make the comment order these like the declaration
order.

Done.

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:160: if
(buttons[i]->icon() == icon) {
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: {} not necessary

Done.

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:216: 
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: I wouldn't add this blank line

Done.

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:301:
std::map<CaptionButtonIcon, int>::const_iterator it =
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: Use auto.

Done.

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button_container_view.h (right):

https://codereview.chromium.org/1505223004/diff/40001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button_container_view.h:139: // Mapping
of the image needed to paint a button for each of the values of
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: "image ID" maybe?

Done.

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:416: }
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: Shorter:
> 
>   int size_icon_id = IDR_AURA_WINDOW_CONTROL_ICON_MAXIMIZE;
>   gfx::Size button_size(GetAshLayoutSize(BROWSER_RESTORED_HEADER_BUTTON));
>   if (frame_->IsMaximized() || frame_->IsFullscreen()) {
>     size_icon_id = IDR_AURA_WINDOW_CONTROL_ICON_RESTORE;
>     button_size = GetAshLayoutSize(BROWSER_MAXIMIZED_HEADER_BUTTON);
>   }

Done.

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:420:
caption_button_container_->SetButtonSize(button_size);
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: I'd put this call either above or below all the SetButtonImage() calls.

Done.

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.h (right):

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.h:84: // Updates the
size and icons used for the minimize, restore,
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: Wrap as close to 80 columns as possible

Done.

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/1505223004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:185: } else {
On 2015/12/18 18:59:15, Peter Kasting wrote:
> Nit: No else after return.  (This also allows undoing the reindenting to the
> expression below that makes it harder to read.)

Done.

[Evan Stade, 2016-01-04 23:53:17]

(looked around a bit but didn't do a full review, no need to wait for my l g t
m)

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) {
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));

[Peter Kasting, 2016-01-05 00:29:35]

LGTM

[sky, 2016-01-05 00:49:16]

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_constant...
ash/ash_layout_constants.h:11: enum AshLayoutSize {
nit: use enum class so that you can forward declare the enum in places.

https://codereview.chromium.org/1505223004/diff/60001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.h (right):

https://codereview.chromium.org/1505223004/diff/60001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.h:78: // The size of the button's
hit region.
Isn't this just the size of the button, not just the hit region?

[tdanderson, 2016-01-05 16:47:27]

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_constant...
ash/ash_layout_constants.h:11: enum AshLayoutSize {
On 2016/01/05 00:49:16, sky wrote:
> nit: use enum class so that you can forward declare the enum in places.

Done.

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/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.)

https://codereview.chromium.org/1505223004/diff/60001/ash/frame/caption_butto...
File ash/frame/caption_buttons/frame_caption_button.h (right):

https://codereview.chromium.org/1505223004/diff/60001/ash/frame/caption_butto...
ash/frame/caption_buttons/frame_caption_button.h:78: // The size of the button's
hit region.
On 2016/01/05 00:49:16, sky wrote:
> Isn't this just the size of the button, not just the hit region?

Changed the comment.

[tdanderson, 2016-01-05 16:47:33]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-01-05 16:47:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1505223004/#ps80001
(title: "for landing")

[commit-bot: I haz the power, 2016-01-05 16:48:07]

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

[commit-bot: I haz the power, 2016-01-05 17:50:49]

Description was changed from

==========
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.*
==========

to

==========
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.*
==========

[commit-bot: I haz the power, 2016-01-05 17:50:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-05 17:51:51]

Description was changed from

==========
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.*
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-01-05 17:51:52]

Patchset 5 (id:??) landed as
https://crrev.com/b161d2c9aab0c7eff442ba9c90303d8bce22880f
Cr-Commit-Position: refs/heads/master@{#367566}

[Evan Stade, 2016-01-05 19:41:57]

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