Make tab audible and muted states and cause available for an extension API

Make tab audible and muted states available for an extension API

* Added 'cause' parameter to mute method to indicate if muted state was changed by a user or by an extension.
* Added support for audible and muted event listeners.

A future patch that relies on these changes will update an API to make the audible and muted states available to extensions.

BUG=438903
Committed: https://crrev.com/dccb8dd3c2453f93475d1958c48f2489937be0dc
Cr-Commit-Position: refs/heads/master@{#319574}

[miu, 2014-12-05 23:07:02]

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

[miu, 2014-12-05 23:46:48]

Looks good.  Just some minor tweaks, and then we can loop in an owner for the
WebContents/WebContentsImpl changes.

https://codereview.chromium.org/757033005/diff/1/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/757033005/diff/1/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:834:
chrome::SetTabAudioMuted(contents, !chrome::IsTabAudioMuted(contents), "user");
Instead of passing "user" as a cause, it's best to have this string constant
declared/defined in one place and then used by all the call points.

Suggestion: Add a static constant to tab_utils.cc:

  const char kUserMuteToggleCause[] = "user";

https://codereview.chromium.org/757033005/diff/1/chrome/browser/ui/tabs/tab_u...
File chrome/browser/ui/tabs/tab_utils.h (right):

https://codereview.chromium.org/757033005/diff/1/chrome/browser/ui/tabs/tab_u...
chrome/browser/ui/tabs/tab_utils.h:102: void
SetTabAudioMuted(content::WebContents* contents, bool muted, const std::string&
cause);
Chromium style: Make sure to stay under 80 chars per line.  Running `git cl
presubmit --force` will tell you where this needs to be fixed throughout your
change.

https://codereview.chromium.org/757033005/diff/1/content/browser/media/captur...
File content/browser/media/capture/web_contents_audio_input_stream.cc (right):

https://codereview.chromium.org/757033005/diff/1/content/browser/media/captur...
content/browser/media/capture/web_contents_audio_input_stream.cc:254:
contents->SetAudioMuted(false, "user");
You won't be able to reference that string constant in tab_utils.cc from here. 
However, IMO, this should probably be a different string anyway.  How about (at
top of this file):

  static const char kForceUnmutedCause[] = "auto-forced for capture";

https://codereview.chromium.org/757033005/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/757033005/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1017: // TODO: The following
should be eliminated once the browser UI uses the new WebContentsObserver
interface.
Actually, I think I gave you bad advice (earlier) on having this TODO comment
here.  I need to first discuss this issue with the browser UI owners some more,
to decide whether NotifyNavigationStateChanged() is the right or wrong mechanism
to update the tab strip UI.  (Maybe the mechanism is right, and the method
should just be renamed to something better.)

In other words, for now, please replace this TODO comment with:

  // Notify the delegate that a UI update may be needed for the new tab media
state.

https://codereview.chromium.org/757033005/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/757033005/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.h:250: void SetAudioMuted(bool
muted, const std::string &cause) override;
style: Place the ampersand next to the type (here and throughout this change).

[Jared Sohn, 2014-12-07 22:39:45]

Thanks for your feedback; I have made the suggested changes.  I just want your
input on moving the "user" constant into a header file versus declaring it in
both modules that use it (or other alternatives.)

https://codereview.chromium.org/757033005/diff/1/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/757033005/diff/1/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:834:
chrome::SetTabAudioMuted(contents, !chrome::IsTabAudioMuted(contents), "user");
On 2014/12/05 23:46:48, miu wrote:
> Instead of passing "user" as a cause, it's best to have this string constant
> declared/defined in one place and then used by all the call points.
> 
> Suggestion: Add a static constant to tab_utils.cc:
> 
>   const char kUserMuteToggleCause[] = "user";

I also need access to this constant from
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc.  

Would it be better to redundantly declare the same constant in that file or
define it once in a shared header file?  Since both classes make use of
tab_utils.h, I have considered moving the constant to there (and then placing it
within the chrome namespace since that is what the function that uses it uses.)

Is this okay, or is there a preference for not putting string constants in
header files (perhaps because it will get instantiated by any file that includes
it, even if it doesn't use it?  Although, since it is static it will only get
instantiated once, so its very small negative effect would be allocating space
when modules that don't call the function but use the header file get built.)

[Jared Sohn, 2014-12-07 23:07:52]

I am looking for OWNERS approval from 

sky@chromium.org for chrome/browser/ui/tabs/tab_strip_model.cc

and 

jam@chromium.org for content/public/browser/* and content/browser/web_contents/*
changes.

[miu, 2014-12-09 01:48:12]

miu@chromium.org changed reviewers:
+ jam@chromium.org, sky@chromium.org

[miu, 2014-12-09 01:50:22]

Added sky@ and jam@ as reviewers (not sure if they saw your request on cc'ed
emails).

sky for chrome/browser/ui/tabs/tab_strip_model.cc

jam for content/public/browser/* and content/browser/web_contents/* changes.

[miu, 2014-12-09 01:56:24]

lgtm % the one change:

https://codereview.chromium.org/757033005/diff/1/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/757033005/diff/1/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:834:
chrome::SetTabAudioMuted(contents, !chrome::IsTabAudioMuted(contents), "user");
On 2014/12/07 22:39:45, Jared Sohn wrote:
> On 2014/12/05 23:46:48, miu wrote:
> > Suggestion: Add a static constant to tab_utils.cc:
> > 
> >   const char kUserMuteToggleCause[] = "user";
> 
> I also need access to this constant from
> chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc.  

The way you did it is the preferred way, with one minor thing.  See next comment
for clarification...

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

https://codereview.chromium.org/757033005/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_utils.h:36: const char kUserMuteToggleCause[] =
"user";
You should only declare the constant here:

  const char kUserMuteToggleCause[];

And define it in the tab_utils.cc file:

  const char kUserMuteToggleCause[] = "user";

That'll resolve the concern you had about building the string into every file
that includes tab_utils.h.  ;-)

[sky, 2014-12-09 16:37:45]

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

https://codereview.chromium.org/757033005/diff/40001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_utils.h:36: const char kUserMuteToggleCause[] =
"user";
On 2014/12/09 01:56:24, miu wrote:
> You should only declare the constant here:
> 
>   const char kUserMuteToggleCause[];
> 
> And define it in the tab_utils.cc file:
> 
>   const char kUserMuteToggleCause[] = "user";
> 
> That'll resolve the concern you had about building the string into every file
> that includes tab_utils.h.  ;-)

Is there a reason this is a string and not an enum? If you need a string at a
later date you can always map the enum to a string internally when needed.

[jam, 2014-12-09 17:20:06]

chrome is the one telling content to mute a tab. why should content then notify
chrome that a tab got muted (in the new WCO callbacks)? seems like chrome can do
this work itself

[Jared Sohn, 2014-12-09 17:51:13]

On 2014/12/09 at 16:37:45, sky wrote:
>
https://codereview.chromium.org/757033005/diff/40001/chrome/browser/ui/tabs/t...
> File chrome/browser/ui/tabs/tab_utils.h (right):
> 
>
https://codereview.chromium.org/757033005/diff/40001/chrome/browser/ui/tabs/t...
> chrome/browser/ui/tabs/tab_utils.h:36: const char kUserMuteToggleCause[] =
"user";
> On 2014/12/09 01:56:24, miu wrote:
> > You should only declare the constant here:
> > 
> >   const char kUserMuteToggleCause[];
> > 
> > And define it in the tab_utils.cc file:
> > 
> >   const char kUserMuteToggleCause[] = "user";
> > 
> > That'll resolve the concern you had about building the string into every
file
> > that includes tab_utils.h.  ;-)
> 
> Is there a reason this is a string and not an enum? If you need a string at a
later date you can always map the enum to a string internally when needed.

The reason we pass a string is that a followup patch will have extension-related
code call the same method, passing along an extension id instead.

[Jared Sohn, 2014-12-09 17:58:52]

On 2014/12/09 at 01:56:24, miu wrote:
> lgtm % the one change:
> 
>
https://codereview.chromium.org/757033005/diff/1/chrome/browser/ui/cocoa/tabs...
> File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):
> 
>
https://codereview.chromium.org/757033005/diff/1/chrome/browser/ui/cocoa/tabs...
> chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:834:
chrome::SetTabAudioMuted(contents, !chrome::IsTabAudioMuted(contents), "user");
> On 2014/12/07 22:39:45, Jared Sohn wrote:
> > On 2014/12/05 23:46:48, miu wrote:
> > > Suggestion: Add a static constant to tab_utils.cc:
> > > 
> > >   const char kUserMuteToggleCause[] = "user";
> > 
> > I also need access to this constant from
> > chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc.  
> 
> The way you did it is the preferred way, with one minor thing.  See next
comment for clarification...
> 
>
https://codereview.chromium.org/757033005/diff/40001/chrome/browser/ui/tabs/t...
> File chrome/browser/ui/tabs/tab_utils.h (right):
> 
>
https://codereview.chromium.org/757033005/diff/40001/chrome/browser/ui/tabs/t...
> chrome/browser/ui/tabs/tab_utils.h:36: const char kUserMuteToggleCause[] =
"user";
> You should only declare the constant here:
> 
>   const char kUserMuteToggleCause[];
> 
> And define it in the tab_utils.cc file:
> 
>   const char kUserMuteToggleCause[] = "user";
> 
> That'll resolve the concern you had about building the string into every file
that includes tab_utils.h.  ;-)

I dismissed this approach because I don't think it meets the concern I raised.
(I imagine every module that includes tab_utils.h will also get linked with
tab_utils.cc, so I don't think the change you suggested will improve things in
that way; but if it is preferred style for other reasons I can make the change.)

[sky, 2014-12-09 19:24:20]

Add a comment to make that clear. It also isn't clear why
kUserMuteToggleCause corresponds to.

  -Scott

On Tue, Dec 9, 2014 at 9:51 AM,  <jared.sohn@gmail.com> wrote:
> On 2014/12/09 at 16:37:45, sky wrote:
>
>
https://codereview.chromium.org/757033005/diff/40001/chrome/browser/ui/tabs/t...
>>
>> File chrome/browser/ui/tabs/tab_utils.h (right):
>
>
>
>
https://codereview.chromium.org/757033005/diff/40001/chrome/browser/ui/tabs/t...
>>
>> chrome/browser/ui/tabs/tab_utils.h:36: const char kUserMuteToggleCause[] =
>
> "user";
>>
>> On 2014/12/09 01:56:24, miu wrote:
>> > You should only declare the constant here:
>> >
>> >   const char kUserMuteToggleCause[];
>> >
>> > And define it in the tab_utils.cc file:
>> >
>> >   const char kUserMuteToggleCause[] = "user";
>> >
>> > That'll resolve the concern you had about building the string into every
>
> file
>>
>> > that includes tab_utils.h.  ;-)
>
>
>> Is there a reason this is a string and not an enum? If you need a string
>> at a
>
> later date you can always map the enum to a string internally when needed.
>
> The reason we pass a string is that a followup patch will have
> extension-related
> code call the same method, passing along an extension id instead.
>
> https://codereview.chromium.org/757033005/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[miu, 2014-12-09 21:00:47]

On 2014/12/09 17:20:06, jam wrote:
> chrome is the one telling content to mute a tab. why should content then
notify
> chrome that a tab got muted (in the new WCO callbacks)? seems like chrome can
do
> this work itself

Jared: I got to looking at this again.  I think jam's right and we might not
need the WebContentsObserver callbacks anymore.  For the extension code,
TabsEventRouter can check the audible and mute state (using tab_utils.h
functions) whenever its TabUpdated() method is called, and dispatch the
changed_properties as needed.  I'll follow-up in e-mail later today with
details.

[Jared Sohn, 2014-12-19 06:56:13]

This has been done in the most recent patch.

On 2014/12/09 at 19:24:20, sky wrote:
> Add a comment to make that clear. It also isn't clear why
> kUserMuteToggleCause corresponds to.
> 
>   -Scott
> 
> On Tue, Dec 9, 2014 at 9:51 AM,  <jared.sohn@gmail.com> wrote:
> > On 2014/12/09 at 16:37:45, sky wrote:
> >
> >
https://codereview.chromium.org/757033005/diff/40001/chrome/browser/ui/tabs/t...
> >>
> >> File chrome/browser/ui/tabs/tab_utils.h (right):
> >
> >
> >
> >
https://codereview.chromium.org/757033005/diff/40001/chrome/browser/ui/tabs/t...
> >>
> >> chrome/browser/ui/tabs/tab_utils.h:36: const char kUserMuteToggleCause[] =
> >
> > "user";
> >>
> >> On 2014/12/09 01:56:24, miu wrote:
> >> > You should only declare the constant here:
> >> >
> >> >   const char kUserMuteToggleCause[];
> >> >
> >> > And define it in the tab_utils.cc file:
> >> >
> >> >   const char kUserMuteToggleCause[] = "user";
> >> >
> >> > That'll resolve the concern you had about building the string into every
> >
> > file
> >>
> >> > that includes tab_utils.h.  ;-)
> >
> >
> >> Is there a reason this is a string and not an enum? If you need a string
> >> at a
> >
> > later date you can always map the enum to a string internally when needed.
> >
> > The reason we pass a string is that a followup patch will have
> > extension-related
> > code call the same method, passing along an extension id instead.
> >
> > https://codereview.chromium.org/757033005/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
>

[Jared Sohn, 2014-12-19 07:03:33]

On 2014/12/09 at 17:20:06, jam wrote:
> chrome is the one telling content to mute a tab. why should content then
notify chrome that a tab got muted (in the new WCO callbacks)? seems like chrome
can do this work itself

In the latest patch, the new notifications code was removed (it will be done via
TabsEventRouter::TabUpdated in an upcoming patch) but the code now
stores/provides an accessor for the muted cause field.

[miu, 2014-12-20 03:42:38]

https://codereview.chromium.org/757033005/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/757033005/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:1081: std::string
mutedToggleCause_;
style (underscores, not camel case): This should be muted_toggle_cause_.

[Jared Sohn, 2014-12-20 23:00:53]

On 2014/12/20 at 03:42:38, miu wrote:
>
https://codereview.chromium.org/757033005/diff/60001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_impl.h (right):
> 
>
https://codereview.chromium.org/757033005/diff/60001/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl.h:1081: std::string
mutedToggleCause_;
> style (underscores, not camel case): This should be muted_toggle_cause_.

Thanks.  Should be fixed in latest patch.

[Jared Sohn, 2015-01-09 22:57:50]

On 2014/12/20 at 23:00:53, Jared Sohn wrote:
> On 2014/12/20 at 03:42:38, miu wrote:
> >
https://codereview.chromium.org/757033005/diff/60001/content/browser/web_cont...
> > File content/browser/web_contents/web_contents_impl.h (right):
> > 
> >
https://codereview.chromium.org/757033005/diff/60001/content/browser/web_cont...
> > content/browser/web_contents/web_contents_impl.h:1081: std::string
mutedToggleCause_;
> > style (underscores, not camel case): This should be muted_toggle_cause_.
> 
> Thanks.  Should be fixed in latest patch.

Happy new year.  Can I get some feedback on this patch?

[jam, 2015-01-15 23:22:01]

content shouldn't be changed. audio muted cause is something that's only used by
chrome, so it shouldn't us content to hold it. see
http://www.chromium.org/developers/content-module/content-api

[miu, 2015-01-16 22:25:40]

On 2015/01/09 22:57:50, Jared Sohn wrote:
> Happy new year.  Can I get some feedback on this patch?

Taking a look now!  :)  Sorry for the delay...

[miu, 2015-01-16 23:04:12]

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

https://codereview.chromium.org/757033005/diff/80001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_utils.cc:260: contents->SetAudioMuted(muted, cause);
Following up on what jam said:

> content shouldn't be changed. audio muted cause is something that's only used
by
> chrome, so it shouldn't us content to hold it. see
> http://www.chromium.org/developers/content-module/content-api

I think you could add a WebContentsUserData class here (in tab_utils.cc) to
store the "cause" string, and this would allow you to easily remove it from all
the src/content APIs and code:

  struct LastMuteMetadata : public
content::WebContentsUserData<LastMuteMetadata> {
    string cause;  // Extension ID or constant from header file or empty string
  
   private:
    explicit LastMuteMetadata(content::WebContents* contents) {}
    friend struct content::WebContentsUserData<LastMuteMetadata>;
  };
  
  DEFINE_WEB_CONTENTS_USER_DATA_KEY(LastMuteMetadata);

...then, in SetTabAudioMuted(), just before the content->SetAudioMuted() call:

  LastMuteMetadata::CreateForWebContents(contents);  // Create if not exists.
  LastMuteMetadata::FromWebContents(contents)->cause = cause;

...then, you can also add the GetTabAudioMutedCause() function here in
tab_utils.cc:

  const char kMutedToggleCauseCapture[] = "auto-forced for capture";

  const std::string& GetTabAudioMutedCause(content::WebContents* contents) {
    LastMuteMetadata::CreateForWebContents(contents);  // Create if not exists.
    if (GetTabMediaStateForContents(contents) == TAB_MEDIA_STATE_CAPTURING) {
      // For tab capture, libcontent forces muting off.
      LastMuteMetadata::FromWebContents(contents)->cause =
kMutedToggleCauseCapture;
    }
    return LastMuteMetadata::FromWebContents(contents)->cause;
  }

[Jared Sohn, 2015-01-19 09:48:18]

Thanks.  Code similar to what you described is working for me.  I will post a
new patch here within the next day or so.

A quick question:  The only remaining changes to web_contents_impl involve
changing the "mute" argument into "muted" in a few places.  I would argue that
this is "more right" since it is consistent with other code, but it doesn't
really affect the implementation of the tab audio API.  Shall we keep it as a
part of this patch?

[miu, 2015-01-22 00:55:23]

lgtm, assuming these two comments are addressed:

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

https://codereview.chromium.org/757033005/diff/120001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.cc:21:
content::WebContentsUserData<LastMuteMetadata> {
style: Please run `git cl format` to make indentation in-line with Chromium
style guide.

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

https://codereview.chromium.org/757033005/diff/120001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.h:114: std::string&
GetTabAudioMutedCause(content::WebContents* contents);
Please make the return type const std::string& to avoid external mutation of the
string.

[Jared Sohn, 2015-01-31 00:59:36]

jared.sohn@gmail.com changed reviewers:
- jam@chromium.org

[Jared Sohn, 2015-01-31 01:03:14]

I just removed the code that would change 'mute' to 'muted'.  This along with
earlier changes means that jam is no longer needed as a reviewer and @sky only
needs to LGTM adding cause as a parameter to the call to the SetTabAudioMuted
method in tab_strip_model.cc.

[sky, 2015-02-02 15:43:06]

https://codereview.chromium.org/757033005/diff/160001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_observer.h (right):

https://codereview.chromium.org/757033005/diff/160001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_observer.h:135: virtual void
TabAudibleStateChanged(content::WebContents* contents,
I don't think this makes sense in the TabStripModel. Why put it here?

[miu, 2015-02-02 20:31:17]

On 2015/02/02 15:43:06, sky wrote:
>
https://codereview.chromium.org/757033005/diff/160001/chrome/browser/ui/tabs/...
> chrome/browser/ui/tabs/tab_strip_model_observer.h:135: virtual void
> TabAudibleStateChanged(content::WebContents* contents,
> I don't think this makes sense in the TabStripModel. Why put it here?

I think this got left over from a prior patch set.  Jared: For the extensions
API impl (upcoming, separate change), you were just going to check the tab
audible/mute state in TabsEventRouter::TabUpdated() and add to
|changed_properties| there, right?  If so, there don't need to be any changes to
TabStripModelObserver.

[Jared Sohn, 2015-02-07 22:33:13]

On 2015/02/02 at 20:31:17, miu wrote:
> On 2015/02/02 15:43:06, sky wrote:
> >
https://codereview.chromium.org/757033005/diff/160001/chrome/browser/ui/tabs/...
> > chrome/browser/ui/tabs/tab_strip_model_observer.h:135: virtual void
> > TabAudibleStateChanged(content::WebContents* contents,
> > I don't think this makes sense in the TabStripModel. Why put it here?
> 
> I think this got left over from a prior patch set.  Jared: For the extensions
API impl (upcoming, separate change), you were just going to check the tab
audible/mute state in TabsEventRouter::TabUpdated() and add to
|changed_properties| there, right?  If so, there don't need to be any changes to
TabStripModelObserver.

Correct, I will remove these changes.  At the moment those functions are still
being called to actually add to |changed_properties|, but we can easily move
that code inline or into methods that do not inherit.

[Jared Sohn, 2015-02-11 23:27:18]

On 2015/02/07 at 22:33:13, Jared Sohn wrote:
> On 2015/02/02 at 20:31:17, miu wrote:
> > On 2015/02/02 15:43:06, sky wrote:
> > >
https://codereview.chromium.org/757033005/diff/160001/chrome/browser/ui/tabs/...
> > > chrome/browser/ui/tabs/tab_strip_model_observer.h:135: virtual void
> > > TabAudibleStateChanged(content::WebContents* contents,
> > > I don't think this makes sense in the TabStripModel. Why put it here?
> > 
> > I think this got left over from a prior patch set.  Jared: For the
extensions API impl (upcoming, separate change), you were just going to check
the tab audible/mute state in TabsEventRouter::TabUpdated() and add to
|changed_properties| there, right?  If so, there don't need to be any changes to
TabStripModelObserver.
> 
> Correct, I will remove these changes.  At the moment those functions are still
being called to actually add to |changed_properties|, but we can easily move
that code inline or into methods that do not inherit.

Those changes have been removed.  Can I get another review?

[miu, 2015-02-19 20:02:44]

lgtm % one thing:

https://codereview.chromium.org/757033005/diff/180001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_utils.h (right):

https://codereview.chromium.org/757033005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.h:40: const char kMutedToggleCauseUser[] =
"user";
These should be in tab_utils.cc.  Then, declare them in the header file here as:

  extern const char kMutedToggleCauseUser[];
  extern const char kMutedToggleCauseCapture[];

See the following for examples of how this works:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...

[Jared Sohn, 2015-02-20 23:58:49]

New patchsets have been uploaded after l-g-t-m from miu@chromium.org

[Jared Sohn, 2015-03-05 01:20:29]

@sky,

Can I get a LGTM from you?

[sky, 2015-03-05 16:52:43]

LGTM

https://codereview.chromium.org/757033005/diff/200001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_utils.h (right):

https://codereview.chromium.org/757033005/diff/200001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.h:111: const std::string& cause);
Document what cause is. Especially that it may not be on the constants defined
above.

[miu, 2015-03-07 22:58:11]

The CQ bit was checked by miu@chromium.org

[miu, 2015-03-07 22:58:11]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/757033005/#ps220001
(title: "Document cause within method declaration")

[commit-bot: I haz the power, 2015-03-07 22:58:37]

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

[commit-bot: I haz the power, 2015-03-08 00:59:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-08 00:59:16]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[miu, 2015-03-08 03:11:51]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2015-03-08 03:12:44]

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

[commit-bot: I haz the power, 2015-03-08 03:54:38]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-03-08 03:55:13]

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