Tab audio mute control (views UI), behind a switch (off by default).

Tab audio mute control (views UI), behind a switch (off by default).

This change consolidates all of the existing media indicator UI into a
new MediaIndicatorButton class.  When the indicator is transitioned to
the audio playing or muting state, the button functionality activates;
and, when clicked, will toggle tab-wide audio muting.  Otherwise, the
view only serves to paint the indicator icon with all mouse/gesture
events handled by Tab (its parent View).

Added UMA's to detect usage of the feature, attempted usage of the
disabled feature, and possible close button mis-clicks.

This new feature is off by default.  The plan is to run a UI experiment
and gather the data needed to drive later launch decisions.

Note: This change depends on: https://codereview.chromium.org/586303004/

BUG=360372, 384179
TEST=With --enable-tab-audio-muting, user can click audio indicator to toggle tab muting, or use the tab context menu to toggle muting of one or more selected tabs.

Committed: https://crrev.com/094342f995ebe8303c942933560eb96c72894596
Cr-Commit-Position: refs/heads/master@{#296869}

[miu, 2014-09-21 20:45:28]

miu@chromium.org changed reviewers:
+ mpearson@chromium.org, oshima@chromium.org, pkasting@chromium.org,
sky@chromium.org

[miu, 2014-09-21 20:45:28]

Change description explains it all.  PTAL, specifically:

mpearson: I added some UMA's.  As I'm new to using these, I'd like your opinions
on the ones I added to chrome/browser/ui/views/tabs/tab.cc.  Also, I need OWNERS
for tools/metrics/actions/actions.xml.

oshima: Need OWNERS for graphics added to chrome/app/theme.  Do I need to run a
PNG optimizer, and how/where is that?

pkasting: You may recall an e-mail discussion with the UX folks a while back
that ended in "let's try a UI experiment."  Well, here it is!  ;-)  I tried to
account for most of the UX concerns noted by everyone, but there were a few
where I made an ROI judgement call to not over-complicate the code at this time.

sky: PTAL at all chrome/browser/ui changes.  In tab.cc, the majority of the
changes were about encapsulating all the existing tab media indicator code into
one place in a new internal MediaIndicatorButton class.

[Peter Kasting, 2014-09-22 06:59:09]

On 2014/09/21 20:45:28, miu wrote:
> oshima: Need OWNERS for graphics added to chrome/app/theme.  Do I need to run
a
> PNG optimizer, and how/where is that?

tools/resources/optimize-png-files.sh -- run with -o 2 and point it specifically
at your png files so it doesn't try to compress the world.  Takes a LONG time to
run.

[oshima, 2014-09-22 16:38:25]

c/a/theme lgtm

optimize-png-files.sh is "nice to run", but not required.
I usually run it before brunch cut.

[Mark P, 2014-09-23 17:30:49]

RecordAction calls in tab_strip_model.cc and tab.cc generally lgtm
actions.xml also generally lgtm

Have you tested this by opening about:user-actions and seeing if actions are
recorded when you'd expect (and not at other related times).

Minor comments below.

--mark

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

https://codereview.chromium.org/591963002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:478:
content::RecordAction(UserMetricsAction("MediaIndicatorButton_Dragged"));
Am I understanding right that this is only emitted once at the end of a drag
event?

(I ask to clarify that we're not recording these constantly as the user jiggles
his mouse.)

https://codereview.chromium.org/591963002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:814:
content::RecordAction(UserMetricsAction("CloseTab_MediaIndicatorShowing"));
Is this useful without another user action that records general tab closes? 
i.e., do we have something to help normalize the count of these actions by?  Or
are you not interested in that?

https://codereview.chromium.org/591963002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/591963002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:10904: User clicked the tab media indicator,
but this will have no effect.
nit:
, but
->
;

[miu, 2014-09-23 18:27:59]

> Have you tested this by opening about:user-actions and seeing if actions are
> recorded when you'd expect (and not at other related times).

Yes (neat tool, BTW).  Tested both with and without the
--enable-tab-audio-muting flag.  And, re-tested the latest UMA additions in PS
3.

Thanks for taking a look.  Comments:

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

https://codereview.chromium.org/591963002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:478:
content::RecordAction(UserMetricsAction("MediaIndicatorButton_Dragged"));
On 2014/09/23 17:30:49, Mark P wrote:
> Am I understanding right that this is only emitted once at the end of a drag
> event?
> 
> (I ask to clarify that we're not recording these constantly as the user
jiggles
> his mouse.)

It's emitted once during a drag event.  The only way to trigger this is to press
the left mouse button down within the media indicator and then drag to some
location outside the media indicator.  The RecordAction() is triggered only
once, and not constantly as the mouse moves.  I confirmed this with
chrome://user-actions.

https://codereview.chromium.org/591963002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:814:
content::RecordAction(UserMetricsAction("CloseTab_MediaIndicatorShowing"));
On 2014/09/23 17:30:49, Mark P wrote:
> Is this useful without another user action that records general tab closes? 
> i.e., do we have something to help normalize the count of these actions by? 
Or
> are you not interested in that?

Good point.  I was thinking of using this metric to answer questions about users
that choose to close a "noisy tab."  However, I'm now envisioning a number of
questions related to other intentions, mis-clicks, etc.  I've added a few
actions here to help normalize and differentiate between the situations.

https://codereview.chromium.org/591963002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/591963002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:10904: User clicked the tab media indicator,
but this will have no effect.
On 2014/09/23 17:30:49, Mark P wrote:
> nit:
> , but
> ->
> ;

Done.

[Mark P, 2014-09-23 18:56:15]

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

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:814: if (media_indicator_button_->enabled())
On 2014/09/23 18:27:58, miu wrote:
> On 2014/09/23 17:30:49, Mark P wrote:
> > Is this useful without another user action that records general tab closes? 
> > i.e., do we have something to help normalize the count of these actions by? 
> Or
> > are you not interested in that?
> 
> Good point.  I was thinking of using this metric to answer questions about
users
> that choose to close a "noisy tab."  However, I'm now envisioning a number of
> questions related to other intentions, mis-clicks, etc.  I've added a few
> actions here to help normalize and differentiate between the situations.

Are you planning to do sequence analysis?  If not, this type of structure looks
lot more like a histogram?  Consider recording a histogram with those four
values.

Also, I think we might have a different CloseTab user action that might be hit
on this flow.  Can you check?  i.e., you might need to add
CloseTab_NoMediaIndicator if you have a CloseTab that already is emitted by
something in this function / called by this function.

[miu, 2014-09-23 19:28:27]

On 2014/09/23 18:56:15, Mark P wrote:
> Are you planning to do sequence analysis?  If not, this type of structure
looks
> lot more like a histogram?  Consider recording a histogram with those four
> values.

Yes, there are many questions where order-of-events w.r.t. the other UMAs would
be important.  I can also see some cases where the histogram would be useful,
but that can always be generated by querying the UMAs.

> Also, I think we might have a different CloseTab user action that might be hit
> on this flow.  Can you check?  i.e., you might need to add
> CloseTab_NoMediaIndicator if you have a CloseTab that already is emitted by
> something in this function / called by this function.

Two things that I found:

1. There's a lingering (perhaps long-unused?) CloseTab_Mouse UMA, which is only
recorded from the Mac/Cocoa UI code.

2. The "TouchUMA" at the end of the Tab::ButtonPressed() method invokes some
code in src/ash/, but appears to only be interested in tap gestures.

In all, I've checked via chrome://user-actions and chrome://histograms, and
don't see any duplicate "CloseTab" or similar UMAs (nor histograms) being
generated when closing tabs on my desktop build (using the mouse).

[Mark P, 2014-09-23 19:49:56]

Thanks for looking in the CloseTab situation.  This change lgtm, baring one
comment below.

(remember to revise actions.xml correspondingly)

--mark

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

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:819:
content::RecordAction(UserMetricsAction("CloseTab_CaptureIndicator"));
Can you please give this a better name?

[sky, 2014-09-23 22:58:18]

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model.cc:705: void
TabStripModel::SetTabAudioMuted(int index, bool mute) {
Move this and IsTabAudioMuted into tab_util. My rationale here is that
TabStripModel shouldn't contain any of the command execution. That should all be
moved to a separate object and TabStripModel slimmed down.
Also, you should DCHECK the index if valid and not null check GetWebContentsAt.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model.cc:936: if
(!chrome::IsTabAudioMutingFeatureEnabled())
Does this check matter here?

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model.cc:941: if (!tab ||
!chrome::CanToggleAudioMute(tab))
You shouldn't need the !tab here, at which point you can nuke the temporary.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model.cc:1131: bool
TabStripModel::WillContextMenuMute(int index) {
Move this to tab_util and make it take a const std::vector<int>& indices. At
which point a better name is AreAllTabsMuted.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_utils.cc:172: default:
Can you explicitly list the states so that you get a compile error when a new
one is added (like you have on 241)?

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

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:393: class Tab::MediaIndicatorButton :
public views::ImageButton,
Move this into its own .h/.cc.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:396: explicit MediaIndicatorButton(Tab* tab)
You don't seem to use tab at all here.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:486: opaqueness = 1.0 - opaqueness;  //
Fading out, not in.
Can't you make the animation return the right value so you don't have to do
this?

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:504: virtual void NotifyClick(const
ui::Event& event) OVERRIDE {
Is it going to be annoying that the behavior of the button can change while the
mouse is over it?

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:512: if (Tab* const tab =
static_cast<Tab*>(parent()))
Can the parent really be NULL? I would expect this to be a DCHECK.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:517: virtual void AnimationCanceled(const
gfx::Animation* animation) OVERRIDE {
Having ImageButton be the delegate for two animation classes is error prone. I
would rather you move the delegate for media_indicator_animation_ into it's own
class.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:948: button->SetBoundsRect(bounds);
Did you make sure this does the right thing when rtl?

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

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.h:279: MediaIndicatorButton*
LazyGetMediaIndicatorButton();
Nuke the 'Lazy'. That is, make it GetMediaIndicatorButton().

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

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_unittest.cc:192:
CHECK(tab.media_indicator_button_);
You should ADD_FAILURE here and early return.

[miu, 2014-09-24 22:34:16]

Thanks, sky.  Addressed all your comments.  PTAL.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model.cc:705: void
TabStripModel::SetTabAudioMuted(int index, bool mute) {
On 2014/09/23 22:58:17, sky wrote:
> Move this and IsTabAudioMuted into tab_util. My rationale here is that
> TabStripModel shouldn't contain any of the command execution. That should all
be
> moved to a separate object and TabStripModel slimmed down.
> Also, you should DCHECK the index if valid and not null check
GetWebContentsAt.

Done.  Agreed; I wasn't encapsulating this functionality in the right place.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model.cc:936: if
(!chrome::IsTabAudioMutingFeatureEnabled())
On 2014/09/23 22:58:17, sky wrote:
> Does this check matter here?

Done since the CanToggleAudioMute() should return false if the feature is not
enabled.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model.cc:941: if (!tab ||
!chrome::CanToggleAudioMute(tab))
On 2014/09/23 22:58:17, sky wrote:
> You shouldn't need the !tab here, at which point you can nuke the temporary.

Done.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model.cc:1131: bool
TabStripModel::WillContextMenuMute(int index) {
On 2014/09/23 22:58:17, sky wrote:
> Move this to tab_util and make it take a const std::vector<int>& indices. At
> which point a better name is AreAllTabsMuted.

Done.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_utils.cc:172: default:
On 2014/09/23 22:58:17, sky wrote:
> Can you explicitly list the states so that you get a compile error when a new
> one is added (like you have on 241)?

Done.

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

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:393: class Tab::MediaIndicatorButton :
public views::ImageButton,
On 2014/09/23 22:58:18, sky wrote:
> Move this into its own .h/.cc.

Done.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:396: explicit MediaIndicatorButton(Tab* tab)
On 2014/09/23 22:58:18, sky wrote:
> You don't seem to use tab at all here.

Done.  Yep, I can downcast from View::parent() when needed, and added DCHECK to
make sure it's a Tab.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:486: opaqueness = 1.0 - opaqueness;  //
Fading out, not in.
On 2014/09/23 22:58:18, sky wrote:
> Can't you make the animation return the right value so you don't have to do
> this?

I think it goes against the intentions of gfx::Animation, which is always meant
to progress from 0.0 (semantically means start) to 1.0 (finish).  AFAICT, there
is no construct in gfx::Animation (or gfx::Tween) that allows progressing an
animation from 1.0 to 0.0.

Note that this isn't new code here (it is from the deleted
Tab::PaintMediaIndicator() method).  Also, whatever we change in tab_utils would
affect the Cocoa UI code, so if there's any action item here, it should be in a
different change.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:504: virtual void NotifyClick(const
ui::Event& event) OVERRIDE {
On 2014/09/23 22:58:17, sky wrote:
> Is it going to be annoying that the behavior of the button can change while
the
> mouse is over it?

(I assume you're referring to the code/comment at line 453 above.)

Very likely not, and it should be rare that this ever happens at times other
than after a click on the button.  There's also a significant fade-in/out
duration, so the user experience shouldn't be jarring.

w.r.t. just after a click, there's a very good reason to do it: The default
behavior of ImageButton is to return to the "hover" image after a click. 
However, because MediaIndicatorButton doubles as an indicator, this makes it
unclear to the user whether the button they just pressed did something.  With my
logic, the "callback" from WebContents will cause the new muting state to be
shown, and IMO this is the natural behavior the user is expecting to see.

If you still have concerns, please try patching in this change and run with the
--enable-tab-audio-muting flag.  Or, at least be assured that this is a UI
experiment and we can iterate.  ;-)

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:512: if (Tab* const tab =
static_cast<Tab*>(parent()))
On 2014/09/23 22:58:18, sky wrote:
> Can the parent really be NULL? I would expect this to be a DCHECK.

Done.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:517: virtual void AnimationCanceled(const
gfx::Animation* animation) OVERRIDE {
On 2014/09/23 22:58:18, sky wrote:
> Having ImageButton be the delegate for two animation classes is error prone. I
> would rather you move the delegate for media_indicator_animation_ into it's
own
> class.

Done.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:819:
content::RecordAction(UserMetricsAction("CloseTab_CaptureIndicator"));
On 2014/09/23 19:49:56, Mark P wrote:
> Can you please give this a better name?

Done.  Named it RecordingIndicator, per offline IM discussion.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:948: button->SetBoundsRect(bounds);
On 2014/09/23 22:58:18, sky wrote:
> Did you make sure this does the right thing when rtl?

Good thing you had me check.  When I first implemented the indicator last year,
it was working for RTL locales.  However, it looks like it was broken with
r286754.  I've fixed the problem, and now everything works.

I modified the Tab layout unit tests to run in both LTR and RTL mode prevent
future regressions.

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

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.h:279: MediaIndicatorButton*
LazyGetMediaIndicatorButton();
On 2014/09/23 22:58:18, sky wrote:
> Nuke the 'Lazy'. That is, make it GetMediaIndicatorButton().

Done.

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

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_unittest.cc:192:
CHECK(tab.media_indicator_button_);
On 2014/09/23 22:58:18, sky wrote:
> You should ADD_FAILURE here and early return.

Done.

[sky, 2014-09-25 19:25:05]

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model.cc:705: void
TabStripModel::SetTabAudioMuted(int index, bool mute) {
On 2014/09/24 22:34:15, miu wrote:
> On 2014/09/23 22:58:17, sky wrote:
> > Move this and IsTabAudioMuted into tab_util. My rationale here is that
> > TabStripModel shouldn't contain any of the command execution. That should
all
> be
> > moved to a separate object and TabStripModel slimmed down.
> > Also, you should DCHECK the index if valid and not null check
> GetWebContentsAt.
> 
> Done.  Agreed; I wasn't encapsulating this functionality in the right place.

Can you also move the setters? Long term TabStripModel shouldn't have any of the
command stuff, it should be a separate object (or just a bunch of functions like
you have in tab_util).

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

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:486: opaqueness = 1.0 - opaqueness;  //
Fading out, not in.
On 2014/09/24 22:34:16, miu wrote:
> On 2014/09/23 22:58:18, sky wrote:
> > Can't you make the animation return the right value so you don't have to do
> > this?
> 
> I think it goes against the intentions of gfx::Animation, which is always
meant
> to progress from 0.0 (semantically means start) to 1.0 (finish).  AFAICT,
there
> is no construct in gfx::Animation (or gfx::Tween) that allows progressing an
> animation from 1.0 to 0.0.

Well, it depends upon the animation. In theory you could make one that goes from
1 to 0, but you're right, we don't offer many that do this.

> Note that this isn't new code here (it is from the deleted
> Tab::PaintMediaIndicator() method).  Also, whatever we change in tab_utils
would
> affect the Cocoa UI code, so if there's any action item here, it should be in
a
> different change.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:504: virtual void NotifyClick(const
ui::Event& event) OVERRIDE {
On 2014/09/24 22:34:16, miu wrote:
> On 2014/09/23 22:58:17, sky wrote:
> > Is it going to be annoying that the behavior of the button can change while
> the
> > mouse is over it?
> 
> (I assume you're referring to the code/comment at line 453 above.)

Just in general because the state may change at any point.

> Very likely not, and it should be rare that this ever happens at times other
> than after a click on the button.  There's also a significant fade-in/out
> duration, so the user experience shouldn't be jarring.
> 
> w.r.t. just after a click, there's a very good reason to do it: The default
> behavior of ImageButton is to return to the "hover" image after a click. 
> However, because MediaIndicatorButton doubles as an indicator, this makes it
> unclear to the user whether the button they just pressed did something.  With
my
> logic, the "callback" from WebContents will cause the new muting state to be
> shown, and IMO this is the natural behavior the user is expecting to see.
> 
> If you still have concerns, please try patching in this change and run with
the
> --enable-tab-audio-muting flag.  Or, at least be assured that this is a UI
> experiment and we can iterate.  ;-)

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

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:72: if ((media_state_ ==
TAB_MEDIA_STATE_AUDIO_PLAYING &&
It seems like all of this should be moved to non-views specific media ui code.
But if you don't care about mac yet it can wait.

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:105: }
Do you need a SchedulePaint here? It isn't clear to me that all code paths
trigger a schedulepaint.

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:121:
content::RecordAction(UserMetricsAction("MediaIndicatorButton_Dragged"));
Why do you care about this? Also, consistently name the actions. Here you have
MediaIndicatorButton_Dragged below you have MuteTab and UnmuteTab. I like the
prefix, maybe MediaIndicatorButton_Mute?

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:129: opaqueness = 1.0 -
opaqueness;  // Fading out, not in.
Seems like the lookup for opaquness should move to a common place too.

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:150: else
I suspect this can be hit. Specifically if the mouse is pressed, state changes
to disabled and then you release the mouse. I'm ok if you deal with this
separately, just wanted to point out I think the NOTREACHED can be hit.

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

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.h:42: static const char
kViewClassName[];
nit: this should be first in the section (see style guide).

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

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_unittest.cc:95: if (testing_for_rtl_locale()) {
nit: no {}

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_unittest.cc:103: return false;
I don't get what you're trying to do here. If not testing rtl (meaning you want
ltr) you always return false and test early returns.

[miu, 2014-09-25 22:49:36]

Addressed all round 2 comments.  PTAL.

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://codereview.chromium.org/591963002/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model.cc:705: void
TabStripModel::SetTabAudioMuted(int index, bool mute) {
On 2014/09/25 19:25:04, sky wrote:
> On 2014/09/24 22:34:15, miu wrote:
> > On 2014/09/23 22:58:17, sky wrote:
> > > Move this and IsTabAudioMuted into tab_util. My rationale here is that
> > > TabStripModel shouldn't contain any of the command execution. That should
> all
> > be
> > > moved to a separate object and TabStripModel slimmed down.
> > > Also, you should DCHECK the index if valid and not null check
> > GetWebContentsAt.
> > 
> > Done.  Agreed; I wasn't encapsulating this functionality in the right place.
> 
> Can you also move the setters? Long term TabStripModel shouldn't have any of
the
> command stuff, it should be a separate object (or just a bunch of functions
like
> you have in tab_util).

Done.

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

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:72: if ((media_state_ ==
TAB_MEDIA_STATE_AUDIO_PLAYING &&
On 2014/09/25 19:25:04, sky wrote:
> It seems like all of this should be moved to non-views specific media ui code.
> But if you don't care about mac yet it can wait.

Acknowledged.  Yeah, what I needed is to modify the
chrome::CreateTabMediaIndicatorFadeAnimation() function to take both the before
and after state.  I'll do that when I address Mac UI.

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:105: }
On 2014/09/25 19:25:05, sky wrote:
> Do you need a SchedulePaint here? It isn't clear to me that all code paths
> trigger a schedulepaint.

The calls to SetImage(), SetEnabled(), and SetState() above call it when it's
needed.  I've put a comment here to note that assumption.

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:121:
content::RecordAction(UserMetricsAction("MediaIndicatorButton_Dragged"));
On 2014/09/25 19:25:04, sky wrote:
> Why do you care about this?

I want a metric to help determine whether the mute toggle button is interfering
with the user's ability to drag a tab on the tab strip.  Currently in Chrome, a
tab drag can start with the mouse pressed down on the indicator.  In this
change, I've preserved that behavior in all cases except where the indicator
turns into the mute button.

> Also, consistently name the actions. Here you have
> MediaIndicatorButton_Dragged below you have MuteTab and UnmuteTab. I like the
> prefix, maybe MediaIndicatorButton_Mute?

Done.

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:129: opaqueness = 1.0 -
opaqueness;  // Fading out, not in.
On 2014/09/25 19:25:05, sky wrote:
> Seems like the lookup for opaquness should move to a common place too.

Acknowledged.  As above, when I make the changes for Mac, I'll be able to
provide an Animation that can return 1.0 to 0.0.  Then, we won't need the extra
translation anymore.

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:150: else
On 2014/09/25 19:25:04, sky wrote:
> I suspect this can be hit. Specifically if the mouse is pressed, state changes
> to disabled and then you release the mouse. I'm ok if you deal with this
> separately, just wanted to point out I think the NOTREACHED can be hit.

I think we're good here: CustomButton (superclass) is what calls the
NotifyClick() method, and there are no code paths that allow it to do so while
the state is disabled.  There is one for a keyboard accelerator event, but that
shouldn't apply to this button.

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

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.h:42: static const char
kViewClassName[];
On 2014/09/25 19:25:05, sky wrote:
> nit: this should be first in the section (see style guide).

Done.

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

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_unittest.cc:95: if (testing_for_rtl_locale()) {
On 2014/09/25 19:25:05, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_unittest.cc:103: return false;
On 2014/09/25 19:25:05, sky wrote:
> I don't get what you're trying to do here. If not testing rtl (meaning you
want
> ltr) you always return false and test early returns.

I thought of a cleaner way to do this to make it much more readable, and changed
the code.

[sky, 2014-09-25 23:39:47]

LGTM

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

https://codereview.chromium.org/591963002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/media_indicator_button.cc:150: else
On 2014/09/25 22:49:36, miu wrote:
> On 2014/09/25 19:25:04, sky wrote:
> > I suspect this can be hit. Specifically if the mouse is pressed, state
changes
> > to disabled and then you release the mouse. I'm ok if you deal with this
> > separately, just wanted to point out I think the NOTREACHED can be hit.
> 
> I think we're good here: CustomButton (superclass) is what calls the
> NotifyClick() method, and there are no code paths that allow it to do so while
> the state is disabled.  There is one for a keyboard accelerator event, but
that
> shouldn't apply to this button.

You are right, I missed the CustomButton handling. Nice!

[miu, 2014-09-26 01:47:02]

The CQ bit was checked by miu@chromium.org

[miu, 2014-09-26 01:47:34]

Thanks everyone for the quick turnaround on this!  Much appreciated, given perf
season and all.  :-)

[commit-bot: I haz the power, 2014-09-26 01:48:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/591963002/120001

[commit-bot: I haz the power, 2014-09-26 03:09:56]

Committed patchset #7 (id:120001) as ac89c10d721ddf0184e552f1ab6116ec56889cfa

[commit-bot: I haz the power, 2014-09-26 03:10:45]

Patchset 7 (id:??) landed as
https://crrev.com/094342f995ebe8303c942933560eb96c72894596
Cr-Commit-Position: refs/heads/master@{#296869}