Render the tab close button as a vector-based icon.

Render the tab close button as a vector-based icon.

Also change the media indicator button to be colored the same way as the tab close button, instead of like the toolbar buttons.  With custom themes, this works better; see screenshots and comments on bug.

BUG=525666
TEST=none

Committed: https://crrev.com/c17704c9a1f65e0a61060188faab6b130f7f755f
Cr-Commit-Position: refs/heads/master@{#350477}

[Peter Kasting, 2015-09-18 01:00:45]

pkasting@chromium.org changed reviewers:
+ estade@chromium.org, sgabriel@chromium.org, sky@chromium.org

[Peter Kasting, 2015-09-18 01:00:46]

estade: ui/gfx/
sky: c/b/ui/views/tabs/
sgabriel: Design approval (see bug for details)

https://codereview.chromium.org/1354823002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab.cc (left):

https://codereview.chromium.org/1354823002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab.cc:1060:
title_->SetVisible(ShouldRenderAsNormalTab());
This was already done in Layout().

[miu, 2015-09-18 01:13:37]

drive-by comment:

https://codereview.chromium.org/1354823002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/1354823002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab.h:284: void SetCloseButtonNormalImage();
naming: When I was originally reading this, I was thinking, "Where's the arg to
this method?"  Suggestions:

  ResetCloseButtonAfterThemeChange();
  DoCloseButtonLayout();
  ...

And wording in the comment should be clarified similarly.

[Peter Kasting, 2015-09-18 01:17:13]

https://codereview.chromium.org/1354823002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/1354823002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab.h:284: void SetCloseButtonNormalImage();
On 2015/09/18 01:13:37, miu wrote:
> naming: When I was originally reading this, I was thinking, "Where's the arg
to
> this method?"  Suggestions:
> 
>   ResetCloseButtonAfterThemeChange();
>   DoCloseButtonLayout();
>   ...
> 
> And wording in the comment should be clarified similarly.

Neither of those names would be accurate, though.  This is not called only after
a theme change, and it does not do any close button layout.  It only sets the
close button's normal image.  I can't think of a more accurate name than the
current one, and it seems like the comment makes clear why there doesn't need to
be an arg.

[sky, 2015-09-18 16:01:52]

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (left):

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1060:
title_->SetVisible(ShouldRenderAsNormalTab());
How come you removed this line?

[Evan Stade, 2015-09-18 17:30:33]

title nit: "Render the /tab/ close button..." as other places like FIP aren't
updated.

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:467: // The hovered andpressed images won't
be changing later, so we can set them
nit: "and pressed"

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:767: OnThemeChanged();
why not use OnViewHierarchyChanged? I imagine that gets called before
Layout/GetPreferredSize but even if not, it seems more self-documenting than
having this special close button color value.

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1535: gfx::VectorIconId::TAB_CLOSE_NORMAL,
16, close_button_color_);
nit: lots of 16s hardcoded here, why not put this at the top with the other
consts (for consistency if nothing else)

https://codereview.chromium.org/1354823002/diff/20001/ui/gfx/vector_icons/tab...
File ui/gfx/vector_icons/tab_close_hovered_pressed.icon (right):

https://codereview.chromium.org/1354823002/diff/20001/ui/gfx/vector_icons/tab...
ui/gfx/vector_icons/tab_close_hovered_pressed.icon:1: // The close "X" for a tab
when hovered/pressed.
nit: needs license header

[Peter Kasting, 2015-09-18 19:48:30]

On 2015/09/18 17:30:33, Evan Stade (ooo) wrote:
> title nit: "Render the /tab/ close button..." as other places like FIP aren't
> updated.

Done.

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (left):

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1060:
title_->SetVisible(ShouldRenderAsNormalTab());
On 2015/09/18 16:01:52, sky wrote:
> How come you removed this line?

See comment in patch set 1 -- we do this already in Layout(), so this is
redundant.

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:467: // The hovered andpressed images won't
be changing later, so we can set them
On 2015/09/18 17:30:33, Evan Stade (ooo) wrote:
> nit: "and pressed"

Done.

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:767: OnThemeChanged();
On 2015/09/18 17:30:33, Evan Stade (ooo) wrote:
> why not use OnViewHierarchyChanged? I imagine that gets called before
> Layout/GetPreferredSize but even if not, it seems more self-documenting than
> having this special close button color value.

Done.  I also needed to handle active state changes correctly, so that's now
done too.

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1535: gfx::VectorIconId::TAB_CLOSE_NORMAL,
16, close_button_color_);
On 2015/09/18 17:30:33, Evan Stade (ooo) wrote:
> nit: lots of 16s hardcoded here, why not put this at the top with the other
> consts (for consistency if nothing else)

Done.

https://codereview.chromium.org/1354823002/diff/20001/ui/gfx/vector_icons/tab...
File ui/gfx/vector_icons/tab_close_hovered_pressed.icon (right):

https://codereview.chromium.org/1354823002/diff/20001/ui/gfx/vector_icons/tab...
ui/gfx/vector_icons/tab_close_hovered_pressed.icon:1: // The close "X" for a tab
when hovered/pressed.
On 2015/09/18 17:30:33, Evan Stade (ooo) wrote:
> nit: needs license header

Why?  This isn't source code, it's a resource file.

[sky, 2015-09-18 20:33:38]

LGTM

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (left):

https://codereview.chromium.org/1354823002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1060:
title_->SetVisible(ShouldRenderAsNormalTab());
On 2015/09/18 19:48:30, Peter Kasting wrote:
> On 2015/09/18 16:01:52, sky wrote:
> > How come you removed this line?
> 
> See comment in patch set 1 -- we do this already in Layout(), so this is
> redundant.

I only saw this patchset, so I missed that comment.

[Evan Stade, 2015-09-18 21:51:18]

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:750: SetCloseButtonNormalStateImage();
maybe guard with:

if (details.is_add && details.child == this)

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1533: const SkColor new_close_button_color =
SkColorSetA(theme_provider->GetColor(
why not use COLOR_TAB_ICON?

[Evan Stade, 2015-09-18 22:10:51]

On 2015/09/18 21:51:18, Evan Stade (ooo) wrote:
>
https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/tabs/tab.cc (right):
> 
>
https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/tabs/tab.cc:750: SetCloseButtonNormalStateImage();
> maybe guard with:
> 
> if (details.is_add && details.child == this)
> 
>
https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/tabs/tab.cc:1533: const SkColor new_close_button_color
=
> SkColorSetA(theme_provider->GetColor(
> why not use COLOR_TAB_ICON?

So here's a screenshot of the "beautiful landscape" theme[1]. The tab audio icon
is white because it uses the same algorithm (more or less) as the
back/forward/refresh buttons. The close button is not themed. For a dark or a
light theme, the desired behavior is obvious, but for this intermediate state
it's not clear to me if we want to match tab text color or titlebar button
color. It certainly looks odd to have them mixed right next to each other.

For reference here's a ss that also has the tab waiting indicator[2]

[1] https://screenshot.googleplex.com/A5m43rTHpDW
[2] https://screenshot.googleplex.com/bhGk8AzNNTi

[miu, 2015-09-19 00:19:58]

https://codereview.chromium.org/1354823002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/1354823002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab.h:284: void SetCloseButtonNormalImage();
On 2015/09/18 01:17:13, Peter Kasting wrote:
> Neither of those names would be accurate, though.  This is not called only
after
> a theme change, and it does not do any close button layout.  It only sets the

It's a moot point now (at patch set 3), but back at patch set 1, it was clearly
meant to be called only after a theme change: The one time it wasn't called for
that reason, you had added a giant comment about why you have to call it without
knowing the correct color.  ;-)

Also, in that giant comment, you said "so that the close button gets sized and
laid out correctly."

Just rationalizing my earlier drive-by suggestions...I'll bow-out now.

[Peter Kasting, 2015-09-19 01:20:41]

On 2015/09/19 00:19:58, miu wrote:
>
https://codereview.chromium.org/1354823002/diff/1/chrome/browser/ui/views/tab...
> File chrome/browser/ui/views/tabs/tab.h (right):
> 
>
https://codereview.chromium.org/1354823002/diff/1/chrome/browser/ui/views/tab...
> chrome/browser/ui/views/tabs/tab.h:284: void SetCloseButtonNormalImage();
> On 2015/09/18 01:17:13, Peter Kasting wrote:
> > Neither of those names would be accurate, though.  This is not called only
> after
> > a theme change, and it does not do any close button layout.  It only sets
the
> 
> It's a moot point now (at patch set 3), but back at patch set 1, it was
clearly
> meant to be called only after a theme change: The one time it wasn't called
for
> that reason, you had added a giant comment about why you have to call it
without
> knowing the correct color.  ;-)
> 
> Also, in that giant comment, you said "so that the close button gets sized and
> laid out correctly."

Yeah.  It was confusing, since this function didn't do any layout, but it did
have to be called before we did layout.  So I understood where you were coming
from (and should have said so), it just didn't seem like the right answer. 
Anyway, things seem to be shaping up to make more and more sense as I iterate
the patches here, thankfully.

[Peter Kasting, 2015-09-19 01:20:56]

New snap not yet up.

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:750: SetCloseButtonNormalStateImage();
On 2015/09/18 21:51:18, Evan Stade (ooo) wrote:
> maybe guard with:
> 
> if (details.is_add && details.child == this)

I intentionally didn't do that.  Let's say a parent View creates us and calls
AddChildView() but isn't in a widget hierarchy yet.  Then we'll get called here,
but GetThemeProvider() will still return null, so we can't set the correct
image.  Once the parent gets put in a hierarchy that has a widget, we'll get
another call here, and then we can do the right thing.

If necessary, I could add comments about this.  I could also try to do something
more complex like checking whether we could now get a theme provider and we
hadn't previously set a color.  I didn't bother with this because we already
early-out before calling CreateVectorIcon() when the color is the same as
before.

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1533: const SkColor new_close_button_color =
SkColorSetA(theme_provider->GetColor(
On 2015/09/18 21:51:18, Evan Stade (ooo) wrote:
> why not use COLOR_TAB_ICON?

Thanks for pointing me at that.  Looks like only the media indicator uses this
today.

As you noted, this also causes an issue (even before my change) where the close
button can look like a very different color than the media indicator.  This also
doesn't handle themes correctly which have foreground and background tabs with
wildly different colors from each other, since the button tint was designed for
the toolbar buttons, which always render against the toolbar background, which
may be different than the background tab background.

I think the right behavior is to make the media indicator match the close button
rather than the other way around.  This deals with background tabs better.  And
since the close button is much more common than the media indicator today, this
is less of a behavioral change than the alternative.

Now, how to implement all this.  In email, Sebastien asked me to change the
close button to the equivalent of rendering the chrome grey with no alpha.  That
means that in the default theme, the close button should be exactly the same
color the media indicator already is.  So I think we can just use one piece of
code to compute one color and use it for both icons.  I think the best solution
is to use code like what I'm doing now.

Another option would be to replace this with something like
COLOR_ACTIVE_TAB_ICON and COLOR_INACTIVE_TAB_ICON and then move this logic
there.  The problem then is that the current implementation, which doesn't need
a specific theme to be loaded, is more safely callable from earlier points than
this logic is.  While we could move this logic up, it would be somewhat risky,
because callers who called it "too early" would get a value back but not
necessarily know they needed to call again later.

Since this is all tab-specific anyway, it seems OK to confine the implementation
to this file.  I'll work on this for the next patch iteration.

[Peter Kasting, 2015-09-19 07:02:03]

pkasting@chromium.org changed reviewers:
+ miu@chromium.org

[Peter Kasting, 2015-09-19 07:26:33]

pkasting@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[Peter Kasting, 2015-09-19 07:26:35]

Adding reviewers:

pkotwicz: c/b/themes/ OWNERS
miu: c/b/ui/cocoa/tabs/, c/b/ui/tabs/tab_utils.*,
c/b/ui/views/tabs/media_indicator_button.* OWNERS

See latest comment on bug for new screenshots.

https://codereview.chromium.org/1354823002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/media_indicator_button.cc (right):

https://codereview.chromium.org/1354823002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button.cc:133: (media_state_ ==
TAB_MEDIA_STATE_AUDIO_MUTING))
One could argue that the conditional here is a layering violation, because it's
the code in GetTabMediaIndicatorImage() that knows that only these two states
are affected by color changes, so we should just always call ResetImages().  I
think it's OK for this class to "know that detail" and get an efficiency win.

https://codereview.chromium.org/1354823002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1354823002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:829:
media_indicator_button_->SetVisible(showing_media_indicator_);
Simplified this while I was touching it.

https://codereview.chromium.org/1354823002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.h (left):

https://codereview.chromium.org/1354823002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.h:300: MediaIndicatorButton*
GetMediaIndicatorButton();
Since we called this any time the tab's active state changed, we were basically
always creating it anyway.  The extra code complexity doesn't really buy
anything, so I changed to just initing this in the constructor like the close
button, which means we can access the member directly when needed instead of
having this lazy-getter.

https://codereview.chromium.org/1354823002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/1354823002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.h:74: void
SetAnimationContainer(gfx::AnimationContainer* container);
Changed this to comply with Chrome style while I was touching the code next to
it.

[sky, 2015-09-21 15:02:33]

SLGTM

[Evan Stade, 2015-09-21 18:49:10]

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:750: SetCloseButtonNormalStateImage();
On 2015/09/19 01:20:56, Peter Kasting wrote:
> On 2015/09/18 21:51:18, Evan Stade (ooo) wrote:
> > maybe guard with:
> > 
> > if (details.is_add && details.child == this)
> 
> I intentionally didn't do that.  Let's say a parent View creates us and calls
> AddChildView() but isn't in a widget hierarchy yet.  Then we'll get called
here,
> but GetThemeProvider() will still return null, so we can't set the correct
> image.  Once the parent gets put in a hierarchy that has a widget, we'll get
> another call here, and then we can do the right thing.
> 
> If necessary, I could add comments about this.  I could also try to do
something
> more complex like checking whether we could now get a theme provider and we
> hadn't previously set a color.  I didn't bother with this because we already
> early-out before calling CreateVectorIcon() when the color is the same as
> before.

OK, although it seems like if this is a problem it should also apply in many
other places we use that guard. You can probably still add the is_add check
although I'm not sure of its value.

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1533: const SkColor new_close_button_color =
SkColorSetA(theme_provider->GetColor(
> Now, how to implement all this.  In email, Sebastien asked me to change the
> close button to the equivalent of rendering the chrome grey with no alpha. 
That
> means that in the default theme, the close button should be exactly the same
> color the media indicator already is.  So I think we can just use one piece of
> code to compute one color and use it for both icons.  I think the best
solution
> is to use code like what I'm doing now.

sure.

> 
> Another option would be to replace this with something like
> COLOR_ACTIVE_TAB_ICON and COLOR_INACTIVE_TAB_ICON and then move this logic
> there.  The problem then is that the current implementation, which doesn't
need
> a specific theme to be loaded, is more safely callable from earlier points
than
> this logic is.  While we could move this logic up, it would be somewhat risky,
> because callers who called it "too early" would get a value back but not
> necessarily know they needed to call again later.

Moving logic into ThemeService would be my first choice because that's where
much of the theme-related color generation code lives. You are checking nullness
and making calls to GetThemeProvider() either way in this function, so I'm not
sure I follow your point about the safety of calling.

[Peter Kasting, 2015-09-23 00:12:34]

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:750: SetCloseButtonNormalStateImage();
On 2015/09/21 18:49:10, Evan Stade wrote:
> On 2015/09/19 01:20:56, Peter Kasting wrote:
> > On 2015/09/18 21:51:18, Evan Stade (ooo) wrote:
> > > maybe guard with:
> > > 
> > > if (details.is_add && details.child == this)
> > 
> > I intentionally didn't do that.  Let's say a parent View creates us and
calls
> > AddChildView() but isn't in a widget hierarchy yet.  Then we'll get called
> here,
> > but GetThemeProvider() will still return null, so we can't set the correct
> > image.  Once the parent gets put in a hierarchy that has a widget, we'll get
> > another call here, and then we can do the right thing.
> > 
> > If necessary, I could add comments about this.  I could also try to do
> something
> > more complex like checking whether we could now get a theme provider and we
> > hadn't previously set a color.  I didn't bother with this because we already
> > early-out before calling CreateVectorIcon() when the color is the same as
> > before.
> 
> OK, although it seems like if this is a problem it should also apply in many
> other places we use that guard.

Most places we guard with this we're not so much worried about waiting for a
default theme provider as we're just waiting to create objects until we're part
of a hierarchy.

And, in fact, most of _those_ places are wrong.  I'm the source of much of this:
for years I mistakenly believed that if a view was destroyed while not in a view
hierarchy its children weren't deleted, so I wrote code to not add children
until the parent was put in a view hierarchy, and directed others to do the
same.  Then sky corrected my mistake.  What would be best would be to take all
those places and just init stuff in the constructor or similar, but since
there's no real "bug" this causes and the simplification is minimal, it hasn't
seemed worth it.

So anyway, no one who's doing something like that needs to loosen their guard,
only if there are other places wanting to wait for a theme provider.  There
might be a few of those but I'm not sure where offhand.

> You can probably still add the is_add check
> although I'm not sure of its value.

Seems like a good idea.  Added.

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1533: const SkColor new_close_button_color =
SkColorSetA(theme_provider->GetColor(
On 2015/09/21 18:49:10, Evan Stade wrote:
> > Now, how to implement all this.  In email, Sebastien asked me to change the
> > close button to the equivalent of rendering the chrome grey with no alpha. 
> That
> > means that in the default theme, the close button should be exactly the same
> > color the media indicator already is.  So I think we can just use one piece
of
> > code to compute one color and use it for both icons.  I think the best
> solution
> > is to use code like what I'm doing now.
> 
> sure.
> 
> > 
> > Another option would be to replace this with something like
> > COLOR_ACTIVE_TAB_ICON and COLOR_INACTIVE_TAB_ICON and then move this logic
> > there.  The problem then is that the current implementation, which doesn't
> need
> > a specific theme to be loaded, is more safely callable from earlier points
> than
> > this logic is.  While we could move this logic up, it would be somewhat
risky,
> > because callers who called it "too early" would get a value back but not
> > necessarily know they needed to call again later.
> 
> Moving logic into ThemeService would be my first choice because that's where
> much of the theme-related color generation code lives. You are checking
nullness
> and making calls to GetThemeProvider() either way in this function, so I'm not
> sure I follow your point about the safety of calling.

I was worrying about cases like we had in the omnibox code, where we got the
default theme provider, but it was the wrong theme provider, because we weren't
in the right widget, and once we were in the right widget we had a different
theme provider.  That's part of why having layout_constants.h is better than
getting those off the theme provider.

However, I think the code I'm doing here would address this problem anyway,
because we'd re-get the right color after setting a new theme provider due to
the ViewHierarchyChanged() override.  So this probably isn't a problem.

So let's ignore that and just back up and look at the latest implementation to
see what's changed and decide what makes the most sense.

First, I moved setting the title color into this function as well.  I made a
slight behavioral change: non-active multi-selected tabs will now use the
background title color instead of the foreground title color.  I think this is
more correct given how we draw the tab backgrounds.

Second, basically the difference now comes down to whether I want to replace one
line in tab.cc:

  const SkColor new_button_color = SkColorSetA(title_color, 0xA0);

And the existence of the button_color() getter, which is called once in the
media indicator code, with a call like:

  const SkColor new_button_color = theme_provider->GetColor(IsActive() ?
      ThemeProperties::COLOR_TAB_ICON :
      ThemeProperties::COLOR_BACKGROUND_TAB_ICON);

...in both places, add these two constants to the theme properties, and add
implementations to the theme provider to construct them by returning
COLOR_TAB_TEXT with a different alpha (or the like).

I don't think this is really a win.  It's longer and spreads this knowledge
across more files.  It makes it less clear to the reader that we want the tab
icon colors to be shades of the title color.

It's not an enormous difference, and the code would work either way.  It just
seems a bit better the way I have it now.

[Evan Stade, 2015-09-23 18:16:35]

lgtm

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1533: const SkColor new_close_button_color =
SkColorSetA(theme_provider->GetColor(
> It's not an enormous difference, and the code would work either way.  It just
> seems a bit better the way I have it now.

fair enough

https://codereview.chromium.org/1354823002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/media_indicator_button.cc (right):

https://codereview.chromium.org/1354823002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/media_indicator_button.cc:132: if ((media_state_ ==
TAB_MEDIA_STATE_AUDIO_PLAYING) ||
nit: imo there are more parens than necessary here

https://codereview.chromium.org/1354823002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (left):

https://codereview.chromium.org/1354823002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:1602: if (!media_indicator_button_) {
guess I don't really mind either way, but why this change?

[Peter Kasting, 2015-09-23 20:44:13]

https://codereview.chromium.org/1354823002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/media_indicator_button.cc (right):

https://codereview.chromium.org/1354823002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/media_indicator_button.cc:132: if ((media_state_ ==
TAB_MEDIA_STATE_AUDIO_PLAYING) ||
On 2015/09/23 18:16:35, Evan Stade wrote:
> nit: imo there are more parens than necessary here

Yeah, this is an area where Chromium doesn't have a consistent style.  Should
non-unary subexprs be parenthesized always, never, or sometimes?  My personal
style is "always", but the rest of this file seems to go more with "never". 
Changed to match the file style better.

https://codereview.chromium.org/1354823002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (left):

https://codereview.chromium.org/1354823002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:1602: if (!media_indicator_button_) {
On 2015/09/23 18:16:35, Evan Stade wrote:
> guess I don't really mind either way, but why this change?

I wrote a comment on this removal in
https://codereview.chromium.org/1354823002/diff/80001/chrome/browser/ui/views...
; see the comment on the removal of the function from the header there.

[miu, 2015-09-23 21:15:28]

lgtm % minor considerations:

https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/tabs...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.cc:167: ui::ResourceBundle* rb =
&ui::ResourceBundle::GetSharedInstance();
OOC, why change this?  I realize our style guide forbids passing args to
functions by non-const ref, but isn't it reasonable to use them as locals?  In
this case, the old code was literally declaring that the result from
ui::ResourceBundle::GetSharedInstance() must refer to an object (i.e., not null)
and, as a reference type, |rb| could never be changed to refer to something else
(implicit "pointer constness").

Is there a style guide discussion on using references locally?

https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.h (left):

https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.h:300: MediaIndicatorButton*
GetMediaIndicatorButton();
From about 1-2 years back, IIRC, a code reviewer asked me to make the
MediaIndicatorButton created on-demand.  The rationale was that it was a waste
to have an instance of this widget for every, of possibly dozens, of Tab
instances.  Especially if it would ever only be shown for one or two tabs during
typical browser use.

FWICT, the memory usage isn't increased significantly by having an instance for
every Tab, so I don't disagree with your change here.  However, as a Chrome UI
eng expert, what's your guidance on these kinds of situations?

[Peter Kasting, 2015-09-23 21:31:04]

https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/tabs...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.cc:167: ui::ResourceBundle* rb =
&ui::ResourceBundle::GetSharedInstance();
On 2015/09/23 21:15:28, miu wrote:
> OOC, why change this?  I realize our style guide forbids passing args to
> functions by non-const ref, but isn't it reasonable to use them as locals?

The Google style guiding wording is a bit unclear, but I think the common
reading of it is that it also forbids returning non-const refs.  Accordingly, we
have a bug on fixing GetSharedInstance() to do this:

https://code.google.com/p/chromium/issues/detail?id=289640

In comment 0 of that bug, it notes that a first step here is rewriting callers
as I'm doing here.

In the more general case, while the style guide doesn't specifically speak about
using non-const refs as locals, the main argument it gives for not using
non-const ref arguments (that code reads as if this is by-value but is
modifiable as if it's by-pointer) seems to apply to locals as well, and was said
to explicitly outweigh the benefit of assuring the reader that the object
pointed to is non-null.

For this reason, I generally suggest to people that they not use non-const refs
anywhere.

> Is there a style guide discussion on using references locally?

We may have had some, but none comes to mind instantly.

https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.h (left):

https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.h:300: MediaIndicatorButton*
GetMediaIndicatorButton();
On 2015/09/23 21:15:28, miu wrote:
> From about 1-2 years back, IIRC, a code reviewer asked me to make the
> MediaIndicatorButton created on-demand.  The rationale was that it was a waste
> to have an instance of this widget for every, of possibly dozens, of Tab
> instances.  Especially if it would ever only be shown for one or two tabs
during
> typical browser use.

My main counterargument to this is that, because we created this every time the
tab's activity state changed, the current code didn't really save anything
anyway.  If we really wanted laziness we'd need a more nuanced use of this stuff
rather than just always calling GetMediaIndicatorButton() everywhere.  See also
my comment about why I removed this, in patch set 5.

The question at that point is whether the benefits would justify modifying the
code to actually achieve "true laziness" (heh) in this respect.

> FWICT, the memory usage isn't increased significantly by having an instance
for
> every Tab, so I don't disagree with your change here.  However, as a Chrome UI
> eng expert, what's your guidance on these kinds of situations?

When laziness was suggested here I probably would have asked immediately what
the actual memory and perf costs of creating this all the time are.

AFAICT, we'll have on the order of hundreds of these buttons in the worst case,
and maybe a couple in the average case (based on people's tab use in the wild). 
I suspect the buttons are reasonably cheap in terms of memory footprint and the
cost to create them.  So the cost of always having them seems low in both those
respects.

The next question is what the benefit of non-laziness is.  In this case, it's
shorter and more direct code (though not by a large amount), and, more
significantly, a simplified mental model of this class (we always have these
children) which lines up better with how we use views elsewhere.

So to me the benefits look like they outweigh the costs.

[miu, 2015-09-23 21:37:15]

still lgtm

On 2015/09/23 21:31:04, Peter Kasting wrote:
>
https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/tabs...
> File chrome/browser/ui/tabs/tab_utils.cc (right):
> 
>
https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/tabs...
> chrome/browser/ui/tabs/tab_utils.cc:167: ui::ResourceBundle* rb =
> &ui::ResourceBundle::GetSharedInstance();
> 
> ...
>
> For this reason, I generally suggest to people that they not use non-const
refs
> anywhere.

Sounds reasonable.  Okay, I'll change my habits for future code change (not that
I tend to use local ref's a whole lot in the first place).

>
https://codereview.chromium.org/1354823002/diff/120001/chrome/browser/ui/view...
> File chrome/browser/ui/views/tabs/tab.h (left):
>
> My main counterargument to this is that, because we created this every time
the
> tab's activity state changed, the current code didn't really save anything
> anyway.  If we really wanted laziness we'd need a more nuanced use of this
stuff

Oh!  Yeah, that'd totally defeat the purpose of having the lazy-instantiation.

> The next question is what the benefit of non-laziness is.  In this case, it's
> shorter and more direct code (though not by a large amount), and, more
> significantly, a simplified mental model of this class (we always have these
> children) which lines up better with how we use views elsewhere.

Yep.  Totally agree.  This did make a lot of code cleaner in this change.

[pkotwicz, 2015-09-24 01:21:58]

lgtm

[Peter Kasting, 2015-09-24 04:04:27]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2015-09-24 04:04:28]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
estade@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1354823002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-24 04:05:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354823002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354823002/140001

[commit-bot: I haz the power, 2015-09-24 04:56:10]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-24 04:58:18]

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