Propagate audible state from player to the containing tab

Propagate audible state from player to the containing tab.

This is part 1 (chromium),
part 2 (clank) is https://chrome-internal-review.googlesource.com/#/c/195455/

This CL is only about propagation/plumbing.
Neither the reporting of audible state my players nor the consumption
(UI notification) are implemented.

This patch creates a new object AudioMonitorAndroid and
attaches it to MediaWebContentsObserver. The AudioMonitorAndroid
maintains the audible state for the tab. It receives notifications
from MediaPlayerAndroid objects and sends the resulting
notification to the WebContents.

BUG=414810
Committed: https://crrev.com/6ad356d3e845ed9b67c5fb799b2ee8290f317268
Cr-Commit-Position: refs/heads/master@{#318823}

[qinmin, 2015-02-03 20:33:23]

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

[qinmin, 2015-02-03 20:33:25]

looks pretty good, some comments

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
File content/browser/android/audio_monitor_android.cc (right):

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.cc:14: , is_audible_(false) {
"," is normally in the previous line

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.cc:18: }
you can move "}" to previous line

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
File content/browser/android/audio_monitor_android.h (right):

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:15: class AudioMonitorAndroid {
docs

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:20: bool IsAudible() const;
since this is just a simple getter function, just use 
bool is_audible() const { return is_audible_; }

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:21: void
OnAudibleStateChanged(RenderFrameHost* rfh, int player_id,
missing docs for all the member functions

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:29: 
extra blank line

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:34: StatusMap players_;
players_ is not a good variable name here, what about audio_status_map_?

https://codereview.chromium.org/896673003/diff/1/content/browser/media/media_...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/896673003/diff/1/content/browser/media/media_...
content/browser/media/media_web_contents_observer.h:33: void
OnAudibleStateChanged(RenderFrameHost* render_frame_host);
docs

https://codereview.chromium.org/896673003/diff/1/media/base/android/media_pla...
File media/base/android/media_player_android.cc (right):

https://codereview.chromium.org/896673003/diff/1/media/base/android/media_pla...
media/base/android/media_player_android.cc:83:
manager_->OnAudibleStateChanged(player_id(), is_audible);
For each SetAudible() call, the AudioMonitorAndroid needs to loop through all
the players and check if the tab is audible. 
it is very inefficient if we handing off all the calls to it.
We can simply do a check here to see if audible state changes before we pass the
call to the manager.

https://codereview.chromium.org/896673003/diff/1/media/base/android/media_pla...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/896673003/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:375: void
MediaPlayerBridge::SetVolume(double volume) {
should we also call setAudible() here?

[Tima Vaisburd, 2015-02-03 23:53:05]

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
File content/browser/android/audio_monitor_android.cc (right):

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.cc:14: , is_audible_(false) {
On 2015/02/03 20:33:24, qinmin wrote:
> "," is normally in the previous line

Done.

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.cc:18: }
On 2015/02/03 20:33:24, qinmin wrote:
> you can move "}" to previous line

Done.

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
File content/browser/android/audio_monitor_android.h (right):

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:15: class AudioMonitorAndroid {
On 2015/02/03 20:33:24, qinmin wrote:
> docs

Done.

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:20: bool IsAudible() const;
On 2015/02/03 20:33:24, qinmin wrote:
> since this is just a simple getter function, just use 
> bool is_audible() const { return is_audible_; }

Done.

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:21: void
OnAudibleStateChanged(RenderFrameHost* rfh, int player_id,
On 2015/02/03 20:33:24, qinmin wrote:
> missing docs for all the member functions

Done.

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:29: 
On 2015/02/03 20:33:24, qinmin wrote:
> extra blank line

Done.

https://codereview.chromium.org/896673003/diff/1/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:34: StatusMap players_;
On 2015/02/03 20:33:24, qinmin wrote:
> players_ is not a good variable name here, what about audio_status_map_?

Done.

https://codereview.chromium.org/896673003/diff/1/content/browser/media/media_...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/896673003/diff/1/content/browser/media/media_...
content/browser/media/media_web_contents_observer.h:33: void
OnAudibleStateChanged(RenderFrameHost* render_frame_host);
On 2015/02/03 20:33:24, qinmin wrote:
> docs

Done. OnAudibleStateChanged() is removed from here.

https://codereview.chromium.org/896673003/diff/1/media/base/android/media_pla...
File media/base/android/media_player_android.cc (right):

https://codereview.chromium.org/896673003/diff/1/media/base/android/media_pla...
media/base/android/media_player_android.cc:83:
manager_->OnAudibleStateChanged(player_id(), is_audible);
On 2015/02/03 20:33:24, qinmin wrote:
> For each SetAudible() call, the AudioMonitorAndroid needs to loop through all
> the players and check if the tab is audible. 
> it is very inefficient if we handing off all the calls to it.
> We can simply do a check here to see if audible state changes before we pass
the
> call to the manager.

Added another |bool is_audible_;| to MediaPlayerAndroid.
Maybe it would be also OK to modify 
AudioMonitorAndroid::OnAudibleStateChanged() like

{
  bool old_state = audio_status_map_[Key(rfh, player_id)];
  if (is_audible != old_state) { ... }
}
to keep the logic in one place?

https://codereview.chromium.org/896673003/diff/1/media/base/android/media_pla...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/896673003/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:375: void
MediaPlayerBridge::SetVolume(double volume) {
On 2015/02/03 20:33:24, qinmin wrote:
> should we also call setAudible() here?

Yes, I just want to see it called, I haven't seen this yet.

[Tima Vaisburd, 2015-02-04 03:04:48]

https://codereview.chromium.org/896673003/diff/1/media/base/android/media_pla...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/896673003/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:375: void
MediaPlayerBridge::SetVolume(double volume) {
On 2015/02/03 23:53:05, Tima Vaisburd wrote:
> On 2015/02/03 20:33:24, qinmin wrote:
> > should we also call setAudible() here?
> 
> Yes, I just want to see it called, I haven't seen this yet. 

Done.

[qinmin, 2015-02-04 03:27:54]

https://codereview.chromium.org/896673003/diff/2/content/browser/android/audi...
File content/browser/android/audio_monitor_android.cc (right):

https://codereview.chromium.org/896673003/diff/2/content/browser/android/audi...
content/browser/android/audio_monitor_android.cc:14: is_audible_(false) {
nit: align this with web_contents_ above

https://codereview.chromium.org/896673003/diff/2/content/browser/android/audi...
content/browser/android/audio_monitor_android.cc:39: int is_audible = false;
bool?

https://codereview.chromium.org/896673003/diff/2/content/browser/android/audi...
File content/browser/android/audio_monitor_android.h (right):

https://codereview.chromium.org/896673003/diff/2/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:50: bool is_audible_;
DISALLOW_COPY_AND_ASSIGN(AudioMonitorAndroid);

https://codereview.chromium.org/896673003/diff/2/media/base/android/media_pla...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/896673003/diff/2/media/base/android/media_pla...
media/base/android/media_player_bridge.h:179: double initial_volume_;
why we need 2 volumes? I think java and native should see the same volume, if
java object is not created, we can check j_media_player_bridge_.obj()

[Tima Vaisburd, 2015-02-04 17:39:01]

https://codereview.chromium.org/896673003/diff/2/media/base/android/media_pla...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/896673003/diff/2/media/base/android/media_pla...
media/base/android/media_player_bridge.h:179: double initial_volume_;
On 2015/02/04 03:27:54, qinmin wrote:
> why we need 2 volumes? I think java and native should see the same volume, if
> java object is not created, we can check j_media_player_bridge_.obj()

I noticed that original code did not maintain the |volume_| if
j_media_player_bridge_ exists
(https://cs.corp.google.com/#clankium/src/media/base/android/media_player_brid...).
Is this on purpose?

[qinmin, 2015-02-04 19:16:17]

https://codereview.chromium.org/896673003/diff/2/media/base/android/media_pla...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/896673003/diff/2/media/base/android/media_pla...
media/base/android/media_player_bridge.h:179: double initial_volume_;
On 2015/02/04 17:39:01, Tima Vaisburd wrote:
> On 2015/02/04 03:27:54, qinmin wrote:
> > why we need 2 volumes? I think java and native should see the same volume,
if
> > java object is not created, we can check j_media_player_bridge_.obj()
> 
> I noticed that original code did not maintain the |volume_| if
> j_media_player_bridge_ exists
>
(https://cs.corp.google.com/#clankium/src/media/base/android/media_player_brid...).
> Is this on purpose?

That looks like a bug to me. Although the first setVolume() is called before
MediaPlayer gets constructed, we might still lose the volume if another
setVolume() is called while the player is playing and then it gets released.
MediaSourcePlayer has the correct implementation.

[Tima Vaisburd, 2015-02-05 23:17:26]

Patchset #4 (id:50001) has been deleted

[Tima Vaisburd, 2015-02-05 23:22:27]

https://codereview.chromium.org/896673003/diff/2/content/browser/android/audi...
File content/browser/android/audio_monitor_android.cc (right):

https://codereview.chromium.org/896673003/diff/2/content/browser/android/audi...
content/browser/android/audio_monitor_android.cc:14: is_audible_(false) {
On 2015/02/04 03:27:54, qinmin wrote:
> nit: align this with web_contents_ above

Done.

https://codereview.chromium.org/896673003/diff/2/content/browser/android/audi...
content/browser/android/audio_monitor_android.cc:39: int is_audible = false;
On 2015/02/04 03:27:54, qinmin wrote:
> bool?

Done.

https://codereview.chromium.org/896673003/diff/2/content/browser/android/audi...
File content/browser/android/audio_monitor_android.h (right):

https://codereview.chromium.org/896673003/diff/2/content/browser/android/audi...
content/browser/android/audio_monitor_android.h:50: bool is_audible_;
On 2015/02/04 03:27:54, qinmin wrote:
> DISALLOW_COPY_AND_ASSIGN(AudioMonitorAndroid);

Done.

https://codereview.chromium.org/896673003/diff/2/media/base/android/media_pla...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/896673003/diff/2/media/base/android/media_pla...
media/base/android/media_player_bridge.h:179: double initial_volume_;
On 2015/02/04 19:16:16, qinmin wrote:
> On 2015/02/04 17:39:01, Tima Vaisburd wrote:
> > On 2015/02/04 03:27:54, qinmin wrote:
> > > why we need 2 volumes? I think java and native should see the same volume,
> if
> > > java object is not created, we can check j_media_player_bridge_.obj()
> > 
> > I noticed that original code did not maintain the |volume_| if
> > j_media_player_bridge_ exists
> >
>
(https://cs.corp.google.com/#clankium/src/media/base/android/media_player_brid...).
> > Is this on purpose?
> 
> That looks like a bug to me. Although the first setVolume() is called before
> MediaPlayer gets constructed, we might still lose the volume if another
> setVolume() is called while the player is playing and then it gets released.
> MediaSourcePlayer has the correct implementation.

Simplified SetVolume() the same way AudioDecoderJob works.

[qinmin, 2015-02-06 00:10:00]

lgtm

[Tima Vaisburd, 2015-02-06 00:54:02]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2015-02-06 00:54:54]

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

[commit-bot: I haz the power, 2015-02-06 00:57:41]

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

[commit-bot: I haz the power, 2015-02-06 00:57:43]

Try jobs failed on following builders:
  ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Tima Vaisburd, 2015-02-06 01:14:02]

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

[Tima Vaisburd, 2015-02-06 01:14:44]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2015-02-06 01:16:06]

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

[commit-bot: I haz the power, 2015-02-06 01:22:52]

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

[commit-bot: I haz the power, 2015-02-06 01:22:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Tima Vaisburd, 2015-02-06 01:52:40]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2015-02-06 01:53:51]

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

[commit-bot: I haz the power, 2015-02-06 01:59:57]

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

[commit-bot: I haz the power, 2015-02-06 01:59:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Tima Vaisburd, 2015-02-06 02:19:32]

timav@chromium.org changed reviewers:
+ sievers@chromium.org

[Tima Vaisburd, 2015-02-06 02:19:32]

Dan, could you, please, review this CL? You are the owner of some parts it
touches. Thank you, --Tima

[Tima Vaisburd, 2015-02-06 23:18:35]

timav@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Ted C, 2015-02-07 00:07:15]

overall seems reasonable to me...I'm sad that we can't reuse the
audio_stream_monitor_.WasRecentlyAudible(); from
WebContentsImpl::WasRecentlyAudible, but I'll defer to you and Min about that
since I don't know the media logic.

https://codereview.chromium.org/896673003/diff/110001/content/browser/android...
File content/browser/android/audio_monitor_android.cc (right):

https://codereview.chromium.org/896673003/diff/110001/content/browser/android...
content/browser/android/audio_monitor_android.cc:49:
web_contents_->NotifyNavigationStateChanged(INVALIDATE_TYPE_TAB);
Is this the navigation state that chrome uses to up date it's speaker icon from
the tab strip (i.e. from TabStripModel::UpdateWebContentsStateAt ->
BrowserTabStripController::TabChangedAt)?

https://codereview.chromium.org/896673003/diff/110001/content/browser/android...
File content/browser/android/audio_monitor_android.h (right):

https://codereview.chromium.org/896673003/diff/110001/content/browser/android...
content/browser/android/audio_monitor_android.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
no (c) anymore in the copyright (applies to all files)

https://codereview.chromium.org/896673003/diff/110001/content/browser/android...
content/browser/android/audio_monitor_android.h:17: // together. That state is
accociated with one Chrome tab that might have
The concept of "tab" is a chrome/ level awareness.  I think you'll just want to
stick with WebContents and drop the references to tab.

https://codereview.chromium.org/896673003/diff/110001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/896673003/diff/110001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:27: audio_monitor_(new
AudioMonitorAndroid(web_contents)) {
needs to be ifdef'd to android

https://codereview.chromium.org/896673003/diff/110001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:53: bool
MediaWebContentsObserver::IsAudible() const {
why not make this method only applicable on android?

https://codereview.chromium.org/896673003/diff/110001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/896673003/diff/110001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:16: class
AudioMonitorAndroid;
should we if def this to android as well?

https://codereview.chromium.org/896673003/diff/110001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/896673003/diff/110001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2533: #if
defined(ENABLE_BROWSER_CDMS)
or if we don't want to make the method ifdef'd to android in the other file,
should we ifdef it to this instead?

https://codereview.chromium.org/896673003/diff/110001/media/base/android/medi...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/896673003/diff/110001/media/base/android/medi...
media/base/android/media_player_bridge.cc:388: SetAudible(volume_ > 0);
do we really care about volume?  It might be nice to get these notifications
that some background tab is playing audio before you turn up the volume and hear
it.

[Tima Vaisburd, 2015-02-07 03:00:23]

> I'm sad that we can't reuse the
> audio_stream_monitor_.WasRecentlyAudible() [...]
> but I'll defer to you and Min

Yes, this was my first question to Min, whether the whole approach is
reasonable. As far as I understand for the Android MediaPlayer path monitoring
the level could be problematic.

https://codereview.chromium.org/896673003/diff/110001/content/browser/android...
File content/browser/android/audio_monitor_android.cc (right):

https://codereview.chromium.org/896673003/diff/110001/content/browser/android...
content/browser/android/audio_monitor_android.cc:49:
web_contents_->NotifyNavigationStateChanged(INVALIDATE_TYPE_TAB);
On 2015/02/07 00:07:15, Ted C wrote:
> Is this the navigation state that chrome uses to up date it's speaker icon
from
> the tab strip (i.e. from TabStripModel::UpdateWebContentsStateAt ->
> BrowserTabStripController::TabChangedAt)?

Yes, on desktop the AudioStreamMonitor makes the same call, see
https://cs.corp.google.com/#clankium/src/content/browser/media/audio_stream_m...

https://codereview.chromium.org/896673003/diff/110001/content/browser/android...
File content/browser/android/audio_monitor_android.h (right):

https://codereview.chromium.org/896673003/diff/110001/content/browser/android...
content/browser/android/audio_monitor_android.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/02/07 00:07:15, Ted C wrote:
> no (c) anymore in the copyright (applies to all files)

Done.

https://codereview.chromium.org/896673003/diff/110001/content/browser/android...
content/browser/android/audio_monitor_android.h:17: // together. That state is
accociated with one Chrome tab that might have
On 2015/02/07 00:07:15, Ted C wrote:
> The concept of "tab" is a chrome/ level awareness.  I think you'll just want
to
> stick with WebContents and drop the references to tab.

Done.

https://codereview.chromium.org/896673003/diff/110001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/896673003/diff/110001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:27: audio_monitor_(new
AudioMonitorAndroid(web_contents)) {
On 2015/02/07 00:07:15, Ted C wrote:
> needs to be ifdef'd to android

Done.

https://codereview.chromium.org/896673003/diff/110001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:53: bool
MediaWebContentsObserver::IsAudible() const {
On 2015/02/07 00:07:15, Ted C wrote:
> why not make this method only applicable on android?

Done.

https://codereview.chromium.org/896673003/diff/110001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/896673003/diff/110001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:16: class
AudioMonitorAndroid;
On 2015/02/07 00:07:15, Ted C wrote:
> should we if def this to android as well?

Yes, done.

https://codereview.chromium.org/896673003/diff/110001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/896673003/diff/110001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2533: #if
defined(ENABLE_BROWSER_CDMS)
On 2015/02/07 00:07:15, Ted C wrote:
> or if we don't want to make the method ifdef'd to android in the other file,
> should we ifdef it to this instead?

Yes, I put it under
   #if defined(ENABLE_BROWSER_CDMS) && defined(OS_ANDROID)
I'm not sure whether OS_ANDROID implies ENABLE_BROWSER_CDMS

https://codereview.chromium.org/896673003/diff/110001/media/base/android/medi...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/896673003/diff/110001/media/base/android/medi...
media/base/android/media_player_bridge.cc:388: SetAudible(volume_ > 0);
On 2015/02/07 00:07:15, Ted C wrote:
> do we really care about volume?  It might be nice to get these notifications
> that some background tab is playing audio before you turn up the volume and
hear
> it.

I think we do. This volume is unrelated to rocker buttons on device, it comes
from HTMLMediaElement's |volume| and |muted| properties and must be set
programmatically, usually in javascript on the web page.

[Ted C, 2015-02-09 23:26:23]

lgtm

[no sievers, 2015-02-10 01:33:03]

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
File content/browser/android/audio_monitor_android.cc (right):

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
content/browser/android/audio_monitor_android.cc:40: for (const auto&
player_status : audio_status_map_) {
Does this really need a separate class for the implementation?
It seems like we are really only tracking one bit of state here (per player),
but are adding a lot of plumbing and complexity that could cause bugs (think
objects pointing to each other and their lifetime) and makes the code harder to
follow.

If you kept the interface, can MediaWebContentsObserver just implement this?
Although I think only 'IsAudible()' might remain, in which case it could be a
callback.

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
File content/browser/android/audio_monitor_android.h (right):

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
content/browser/android/audio_monitor_android.h:16: // This class maintains the
audible state of many players
You should say that this class is associated with a WebContents, and maintains
the state regarding all the players in there.

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
content/browser/android/audio_monitor_android.h:24: // Constructor, web_contents
represents the tab.
Let's avoid talking about 'tabs' here and below which is not a concept in
content/. This is associated with a WebContents.

https://codereview.chromium.org/896673003/diff/150001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/896673003/diff/150001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:67:
scoped_ptr<AudioMonitorAndroid> audio_monitor_;
Can you put this before |media_player_managers_|, since |media_player_managers_|
has a pointer to |audio_monitor_| which would otherwise be invalid during
destruction of |media_player_managers_|?

https://codereview.chromium.org/896673003/diff/150001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/896673003/diff/150001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2496: #if
defined(ENABLE_BROWSER_CDMS) && defined(OS_ANDROID)
Why is there an #ifdef here?
Can you put a comment explaining that? Including what is different between the
two implementations and why this is handled differently on Android.

[Tima Vaisburd, 2015-02-11 03:33:44]

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

[Tima Vaisburd, 2015-02-11 03:34:05]

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
File content/browser/android/audio_monitor_android.cc (right):

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
content/browser/android/audio_monitor_android.cc:40: for (const auto&
player_status : audio_status_map_) {
On 2015/02/10 01:33:03, sievers wrote:
> Does this really need a separate class for the implementation?
> It seems like we are really only tracking one bit of state here (per player),
> but are adding a lot of plumbing and complexity that could cause bugs (think
> objects pointing to each other and their lifetime) and makes the code harder
to
> follow.
> 
> If you kept the interface, can MediaWebContentsObserver just implement this?
> Although I think only 'IsAudible()' might remain, in which case it could be a
> callback.

It does not require a separate class, strictly speaking, but I think in this
situation
it would make sense to have an AndroidAudioMonitor that is in a sense parallel
to AudioStreamMonitor
of the desktop.

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
File content/browser/android/audio_monitor_android.h (right):

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
content/browser/android/audio_monitor_android.h:16: // This class maintains the
audible state of many players
On 2015/02/10 01:33:03, sievers wrote:
> You should say that this class is associated with a WebContents, and maintains
> the state regarding all the players in there.

Changed the wording, please verify.

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
content/browser/android/audio_monitor_android.h:24: // Constructor, web_contents
represents the tab.
On 2015/02/10 01:33:03, sievers wrote:
> Let's avoid talking about 'tabs' here and below which is not a concept in
> content/. This is associated with a WebContents.

Done.

https://codereview.chromium.org/896673003/diff/150001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/896673003/diff/150001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:67:
scoped_ptr<AudioMonitorAndroid> audio_monitor_;
On 2015/02/10 01:33:03, sievers wrote:
> Can you put this before |media_player_managers_|, since
|media_player_managers_|
> has a pointer to |audio_monitor_| which would otherwise be invalid during
> destruction of |media_player_managers_|?

Done.

https://codereview.chromium.org/896673003/diff/150001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/896673003/diff/150001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2496: #if
defined(ENABLE_BROWSER_CDMS) && defined(OS_ANDROID)
On 2015/02/10 01:33:03, sievers wrote:
> Why is there an #ifdef here?
> Can you put a comment explaining that? Including what is different between the
> two implementations and why this is handled differently on Android.

Added a comment, please take a look.

[no sievers, 2015-02-12 19:01:11]

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
File content/browser/android/audio_monitor_android.cc (right):

https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
content/browser/android/audio_monitor_android.cc:40: for (const auto&
player_status : audio_status_map_) {
On 2015/02/11 03:34:05, Tima Vaisburd wrote:
> On 2015/02/10 01:33:03, sievers wrote:
> > Does this really need a separate class for the implementation?
> > It seems like we are really only tracking one bit of state here (per
player),
> > but are adding a lot of plumbing and complexity that could cause bugs (think
> > objects pointing to each other and their lifetime) and makes the code harder
> to
> > follow.
> > 
> > If you kept the interface, can MediaWebContentsObserver just implement this?
> > Although I think only 'IsAudible()' might remain, in which case it could be
a
> > callback.
> 
> It does not require a separate class, strictly speaking, but I think in this
> situation
> it would make sense to have an AndroidAudioMonitor that is in a sense parallel
> to AudioStreamMonitor
> of the desktop.

If you are arguing that way: Maybe we should make AudioStreamMonitor the base
interface?

[Tima Vaisburd, 2015-02-24 03:03:00]

On 2015/02/12 19:01:11, sievers wrote:
>
https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
> File content/browser/android/audio_monitor_android.cc (right):
> 
>
https://codereview.chromium.org/896673003/diff/150001/content/browser/android...
> content/browser/android/audio_monitor_android.cc:40: for (const auto&
> player_status : audio_status_map_) {
> On 2015/02/11 03:34:05, Tima Vaisburd wrote:
> > On 2015/02/10 01:33:03, sievers wrote:
> > > Does this really need a separate class for the implementation?
> > > It seems like we are really only tracking one bit of state here (per
> player),
> > > but are adding a lot of plumbing and complexity that could cause bugs
(think
> > > objects pointing to each other and their lifetime) and makes the code
harder
> > to
> > > follow.
> > > 
> > > If you kept the interface, can MediaWebContentsObserver just implement
this?
> > > Although I think only 'IsAudible()' might remain, in which case it could
be
> a
> > > callback.
> > 
> > It does not require a separate class, strictly speaking, but I think in this
> > situation
> > it would make sense to have an AndroidAudioMonitor that is in a sense
parallel
> > to AudioStreamMonitor
> > of the desktop.
> 
> If you are arguing that way: Maybe we should make AudioStreamMonitor the base
> interface?

Sorry for the delay, I renamed AudioMonitorAndroid into MediaPlayersObserver
and extracted the common part between it and AudioStreamMonitor into the base
class
AudioStateProvider. The change went somewhat big, can you, please, take another
look?

[Tima Vaisburd, 2015-02-24 03:03:19]

timav@chromium.org changed reviewers:
+ aurimas@chromium.org

[Tima Vaisburd, 2015-02-24 03:06:50]

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
File content/browser/media/audio_state_provider.h (right):

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_state_provider.h:25: static bool
audio_state_available();
Does it have to be static?

[qinmin, 2015-02-24 18:02:19]

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
File content/browser/media/audio_state_provider.h (right):

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_state_provider.h:25: static bool
audio_state_available();
On 2015/02/24 03:06:50, Tima Vaisburd wrote:
> Does it have to be static?

I think static is fine, but would prefer this to be renamed to
IsAudioStateAvailable() as this is not inline.

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_state_provider.h:27: // If this provider use
monitoring (i.e. measure the signal),
nit: s/use/uses/

[no sievers, 2015-02-25 21:22:04]

https://codereview.chromium.org/896673003/diff/190001/content/browser/android...
File content/browser/android/media_players_observer.cc (right):

https://codereview.chromium.org/896673003/diff/190001/content/browser/android...
content/browser/android/media_players_observer.cc:7: #include <climits>
nit: space after line 7

https://codereview.chromium.org/896673003/diff/190001/content/browser/android...
content/browser/android/media_players_observer.cc:32:
audio_status_map_.erase(Key(rfh, player_id));
Should this DCHECK() that the return value is one, i.e. one player was found and
got removed?

https://codereview.chromium.org/896673003/diff/190001/content/browser/android...
File content/browser/android/media_players_observer.h (right):

https://codereview.chromium.org/896673003/diff/190001/content/browser/android...
content/browser/android/media_players_observer.h:8: #include <map>
nit: space after line 8

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
File content/browser/media/audio_state_provider.h (right):

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_state_provider.h:25: static bool
audio_state_available();
On 2015/02/24 03:06:50, Tima Vaisburd wrote:
> Does it have to be static?

'virtual' would maybe be a tiny bit nicer since it avoids the #ifdef in the
implementation, and moves that to the platform-specific impl file instead. And
also since you do the same thing for audio_stream_monitor().

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_state_provider.h:29: virtual AudioStreamMonitor*
audio_stream_monitor() = 0;
Only used for testing, i.e. GetAudioStreamMonitorForTesting()?

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_state_provider.h:49: bool  was_recently_audible_;
nit: extra space

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
File content/browser/media/audio_stream_monitor_unittest.cc (right):

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_stream_monitor_unittest.cc:60:
ASSERT_TRUE(monitor_);
Doesn't this fail on Android?

If we don't feel like this class will ever be useful, then you can probably just
ifdef-out the whole file on Android.

[Tima Vaisburd, 2015-02-28 03:16:06]

https://codereview.chromium.org/896673003/diff/190001/content/browser/android...
File content/browser/android/media_players_observer.cc (right):

https://codereview.chromium.org/896673003/diff/190001/content/browser/android...
content/browser/android/media_players_observer.cc:7: #include <climits>
On 2015/02/25 21:22:03, sievers wrote:
> nit: space after line 7

Done.

https://codereview.chromium.org/896673003/diff/190001/content/browser/android...
content/browser/android/media_players_observer.cc:32:
audio_status_map_.erase(Key(rfh, player_id));
On 2015/02/25 21:22:03, sievers wrote:
> Should this DCHECK() that the return value is one, i.e. one player was found
and
> got removed?

Done.

https://codereview.chromium.org/896673003/diff/190001/content/browser/android...
File content/browser/android/media_players_observer.h (right):

https://codereview.chromium.org/896673003/diff/190001/content/browser/android...
content/browser/android/media_players_observer.h:8: #include <map>
On 2015/02/25 21:22:04, sievers wrote:
> nit: space after line 8

Done.

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
File content/browser/media/audio_state_provider.h (right):

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_state_provider.h:25: static bool
audio_state_available();
On 2015/02/25 21:22:04, sievers wrote:
> On 2015/02/24 03:06:50, Tima Vaisburd wrote:
> > Does it have to be static?
> 
> 'virtual' would maybe be a tiny bit nicer since it avoids the #ifdef in the
> implementation, and moves that to the platform-specific impl file instead. And
> also since you do the same thing for audio_stream_monitor().

Made pure virtual method IsAudioStateAvailable(). However, it is used on
WebContentsImpl destructor via MaybeReleasePowerSaveBlockers(), I moved the
member up, please advise.

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_state_provider.h:27: // If this provider use
monitoring (i.e. measure the signal),
On 2015/02/24 18:02:19, qinmin wrote:
> nit: s/use/uses/

Done.

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_state_provider.h:29: virtual AudioStreamMonitor*
audio_stream_monitor() = 0;
On 2015/02/25 21:22:04, sievers wrote:
> Only used for testing, i.e. GetAudioStreamMonitorForTesting()?

It is also used in AudioStreamMonitorFromRenderFrame() in
audio_stream_monitor.cc, I kept the old name.

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_state_provider.h:49: bool  was_recently_audible_;
On 2015/02/25 21:22:04, sievers wrote:
> nit: extra space

Done.

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
File content/browser/media/audio_stream_monitor_unittest.cc (right):

https://codereview.chromium.org/896673003/diff/190001/content/browser/media/a...
content/browser/media/audio_stream_monitor_unittest.cc:60:
ASSERT_TRUE(monitor_);
On 2015/02/25 21:22:04, sievers wrote:
> Doesn't this fail on Android?
> 
> If we don't feel like this class will ever be useful, then you can probably
just
> ifdef-out the whole file on Android.

Yes, I did not realize this test even runs on Android. We can exclude it, but I
tried to make it work since it worked before. Please take a look at the hoops.

[Tima Vaisburd, 2015-03-02 18:37:30]

https://codereview.chromium.org/896673003/diff/210001/content/browser/media/a...
File content/browser/media/audio_stream_monitor_unittest.cc (right):

https://codereview.chromium.org/896673003/diff/210001/content/browser/media/a...
content/browser/media/audio_stream_monitor_unittest.cc:56:
web_contents->SetAudioStateProviderForTesting(
Is it better to keep this test on Android (and add stuff like this) or simply
exclude it on OS_ANDROID? If we want to exclude, should it be done by #ifdef of
in .gyp?

https://codereview.chromium.org/896673003/diff/210001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/896673003/diff/210001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:45:
audio_observer->RenderFrameDeleted(render_frame_host);
Can we rely upon MediaPlayersObserver::BrowserMediaPlayerManager::RemovePlayer
being called instead?

[no sievers, 2015-03-02 20:21:24]

https://codereview.chromium.org/896673003/diff/230001/content/browser/android...
File content/browser/android/media_players_observer.cc (right):

https://codereview.chromium.org/896673003/diff/230001/content/browser/android...
content/browser/android/media_players_observer.cc:38: DCHECK(num_erased_entries
== 1);
nit: DCHECK_EQ(1u, num_erased_entries);

https://codereview.chromium.org/896673003/diff/230001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/896673003/diff/230001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:42: if
(!static_cast<WebContentsImpl*>(web_contents())->IsBeingDestroyed()) {
Why do you have to check this?

https://codereview.chromium.org/896673003/diff/230001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/896673003/diff/230001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:146: void
SetAudioStateProviderForTesting(AudioStateProvider* provider);
If you pass a scoped_ptr<> you don't need to comment on the ownership.

Overall it's a bit weird that (like your comment says) we don't use this code in
production on Android, so we have to add a testing hack here to test unused
code. Should we just disable the tests on Android? It would be way more useful
to add some Android-specific unittests that use our monitor implementation.

https://codereview.chromium.org/896673003/diff/230001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/896673003/diff/230001/content/content_browser...
content/content_browser.gypi:308: 'browser/android/media_players_observer.h',
nit: wrong order

[Tima Vaisburd, 2015-03-02 21:03:03]

https://codereview.chromium.org/896673003/diff/230001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/896673003/diff/230001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:42: if
(!static_cast<WebContentsImpl*>(web_contents())->IsBeingDestroyed()) {
On 2015/03/02 20:21:24, sievers wrote:
> Why do you have to check this?

Because otherwise it crashed the test, but the test is going to be removed, so
this piece will be removed too.

https://codereview.chromium.org/896673003/diff/230001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/896673003/diff/230001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:146: void
SetAudioStateProviderForTesting(AudioStateProvider* provider);
On 2015/03/02 20:21:24, sievers wrote:
> If you pass a scoped_ptr<> you don't need to comment on the ownership.
> 
> Overall it's a bit weird that (like your comment says) we don't use this code
in
> production on Android, so we have to add a testing hack here to test unused
> code. Should we just disable the tests on Android? It would be way more useful
> to add some Android-specific unittests that use our monitor implementation.

Yes, I will happily disable AudioStreamMonitor test on Android and get rid of 
this method. Could you, please advise whether it's better to do it with #ifdef
or in .gyp?

[no sievers, 2015-03-02 22:11:13]

On 2015/03/02 21:03:03, Tima Vaisburd wrote:
>
https://codereview.chromium.org/896673003/diff/230001/content/browser/media/m...
> File content/browser/media/media_web_contents_observer.cc (right):
> 
>
https://codereview.chromium.org/896673003/diff/230001/content/browser/media/m...
> content/browser/media/media_web_contents_observer.cc:42: if
> (!static_cast<WebContentsImpl*>(web_contents())->IsBeingDestroyed()) {
> On 2015/03/02 20:21:24, sievers wrote:
> > Why do you have to check this?
> 
> Because otherwise it crashed the test, but the test is going to be removed, so
> this piece will be removed too.
> 
>
https://codereview.chromium.org/896673003/diff/230001/content/browser/web_con...
> File content/browser/web_contents/web_contents_impl.h (right):
> 
>
https://codereview.chromium.org/896673003/diff/230001/content/browser/web_con...
> content/browser/web_contents/web_contents_impl.h:146: void
> SetAudioStateProviderForTesting(AudioStateProvider* provider);
> On 2015/03/02 20:21:24, sievers wrote:
> > If you pass a scoped_ptr<> you don't need to comment on the ownership.
> > 
> > Overall it's a bit weird that (like your comment says) we don't use this
code
> in
> > production on Android, so we have to add a testing hack here to test unused
> > code. Should we just disable the tests on Android? It would be way more
useful
> > to add some Android-specific unittests that use our monitor implementation.
> 
> Yes, I will happily disable AudioStreamMonitor test on Android and get rid of 
> this method. Could you, please advise whether it's better to do it with #ifdef
> or in .gyp?

I would do #ifdef.

For disabling single tests, we usually use this pattern:
#if defined(OS_ANDROID)
// comment why
#define MAYBE_SomethingTest DISABLED_SomethingTest
#else
#define MAYBE_SomethingTest SomethingTest
#endif

TEST_F(SomeTest, MAYBE_SomethingTest)

If most or all of the tests won't work, I'd #ifdef out the whole file.

[Tima Vaisburd, 2015-03-02 22:16:10]

https://codereview.chromium.org/896673003/diff/230001/content/browser/android...
File content/browser/android/media_players_observer.cc (right):

https://codereview.chromium.org/896673003/diff/230001/content/browser/android...
content/browser/android/media_players_observer.cc:38: DCHECK(num_erased_entries
== 1);
On 2015/03/02 20:21:24, sievers wrote:
> nit: DCHECK_EQ(1u, num_erased_entries);

Done.

https://codereview.chromium.org/896673003/diff/230001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/896673003/diff/230001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:42: if
(!static_cast<WebContentsImpl*>(web_contents())->IsBeingDestroyed()) {
On 2015/03/02 21:03:03, Tima Vaisburd wrote:
> On 2015/03/02 20:21:24, sievers wrote:
> > Why do you have to check this?
> 
> Because otherwise it crashed the test, but the test is going to be removed, so
> this piece will be removed too. 

This check is removed now.

https://codereview.chromium.org/896673003/diff/230001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/896673003/diff/230001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:146: void
SetAudioStateProviderForTesting(AudioStateProvider* provider);
On 2015/03/02 21:03:03, Tima Vaisburd wrote:
> On 2015/03/02 20:21:24, sievers wrote:
> > If you pass a scoped_ptr<> you don't need to comment on the ownership.
> > 
> > Overall it's a bit weird that (like your comment says) we don't use this
code
> in
> > production on Android, so we have to add a testing hack here to test unused
> > code. Should we just disable the tests on Android? It would be way more
useful
> > to add some Android-specific unittests that use our monitor implementation.
> 
> Yes, I will happily disable AudioStreamMonitor test on Android and get rid of 
> this method. Could you, please advise whether it's better to do it with #ifdef
> or in .gyp?

Excluded in .gypi

https://codereview.chromium.org/896673003/diff/230001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/896673003/diff/230001/content/content_browser...
content/content_browser.gypi:308: 'browser/android/media_players_observer.h',
On 2015/03/02 20:21:24, sievers wrote:
> nit: wrong order

Done.

[Tima Vaisburd, 2015-03-02 22:18:26]

On 2015/03/02 22:11:13, sievers wrote:
> On 2015/03/02 21:03:03, Tima Vaisburd wrote:
> >
>
https://codereview.chromium.org/896673003/diff/230001/content/browser/media/m...
> > File content/browser/media/media_web_contents_observer.cc (right):
> > 
> >
>
https://codereview.chromium.org/896673003/diff/230001/content/browser/media/m...
> > content/browser/media/media_web_contents_observer.cc:42: if
> > (!static_cast<WebContentsImpl*>(web_contents())->IsBeingDestroyed()) {
> > On 2015/03/02 20:21:24, sievers wrote:
> > > Why do you have to check this?
> > 
> > Because otherwise it crashed the test, but the test is going to be removed,
so
> > this piece will be removed too.
> > 
> >
>
https://codereview.chromium.org/896673003/diff/230001/content/browser/web_con...
> > File content/browser/web_contents/web_contents_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/896673003/diff/230001/content/browser/web_con...
> > content/browser/web_contents/web_contents_impl.h:146: void
> > SetAudioStateProviderForTesting(AudioStateProvider* provider);
> > On 2015/03/02 20:21:24, sievers wrote:
> > > If you pass a scoped_ptr<> you don't need to comment on the ownership.
> > > 
> > > Overall it's a bit weird that (like your comment says) we don't use this
> code
> > in
> > > production on Android, so we have to add a testing hack here to test
unused
> > > code. Should we just disable the tests on Android? It would be way more
> useful
> > > to add some Android-specific unittests that use our monitor
implementation.
> > 
> > Yes, I will happily disable AudioStreamMonitor test on Android and get rid
of 
> > this method. Could you, please advise whether it's better to do it with
#ifdef
> > or in .gyp?
> 
> I would do #ifdef.
> 
> For disabling single tests, we usually use this pattern:
> #if defined(OS_ANDROID)
> // comment why
> #define MAYBE_SomethingTest DISABLED_SomethingTest
> #else
> #define MAYBE_SomethingTest SomethingTest
> #endif
> 
> TEST_F(SomeTest, MAYBE_SomethingTest)
> 
> If most or all of the tests won't work, I'd #ifdef out the whole file.

Sorry, I did not see this in time. I went with gypi.
In this particular case the whole file has to be excluded. If you feel strongly
I'll redo.

[no sievers, 2015-03-02 22:19:41]

lgtm if Min is happy

[qinmin, 2015-03-02 22:55:41]

lgtm % comments

https://codereview.chromium.org/896673003/diff/250001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java
(right):

https://codereview.chromium.org/896673003/diff/250001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java:74:
protected static native boolean nativeIsPlayingAudio(WebContents webContents);
The function name isn't really correct. nativeHasAudibleAudio() feels more
correct.

https://codereview.chromium.org/896673003/diff/250001/content/browser/android...
File content/browser/android/media_players_observer.cc (right):

https://codereview.chromium.org/896673003/diff/250001/content/browser/android...
content/browser/android/media_players_observer.cc:50: bool is_audible = false;
this variable is not really needed.

we can do 
for(...) {
  if (...) {
    Notify(true);
    return;
  }
}
Notify(false);

https://codereview.chromium.org/896673003/diff/250001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/896673003/diff/250001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:57: 
nit: remove extra blank line

https://codereview.chromium.org/896673003/diff/250001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/896673003/diff/250001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:18: #endif //
defined(OS_ANDROID)
nit: 2 spaces before //

[Tima Vaisburd, 2015-03-02 23:30:07]

https://codereview.chromium.org/896673003/diff/250001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java
(right):

https://codereview.chromium.org/896673003/diff/250001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java:74:
protected static native boolean nativeIsPlayingAudio(WebContents webContents);
On 2015/03/02 22:55:41, qinmin wrote:
> The function name isn't really correct. nativeHasAudibleAudio() feels more
> correct.

Renamed this method.

https://codereview.chromium.org/896673003/diff/250001/content/browser/android...
File content/browser/android/media_players_observer.cc (right):

https://codereview.chromium.org/896673003/diff/250001/content/browser/android...
content/browser/android/media_players_observer.cc:50: bool is_audible = false;
On 2015/03/02 22:55:41, qinmin wrote:
> this variable is not really needed.
> 
> we can do 
> for(...) {
>   if (...) {
>     Notify(true);
>     return;
>   }
> }
> Notify(false);

Done.

https://codereview.chromium.org/896673003/diff/250001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/896673003/diff/250001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:57: 
On 2015/03/02 22:55:41, qinmin wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/896673003/diff/250001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/896673003/diff/250001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:18: #endif //
defined(OS_ANDROID)
On 2015/03/02 22:55:41, qinmin wrote:
> nit: 2 spaces before //

Done.

[Tima Vaisburd, 2015-03-03 01:34:46]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-03-03 01:34:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, qinmin@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/896673003/#ps310001
(title: "Added missing copyright notice")

[commit-bot: I haz the power, 2015-03-03 01:35:33]

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

[commit-bot: I haz the power, 2015-03-03 03:07:50]

Committed patchset #16 (id:310001)

[commit-bot: I haz the power, 2015-03-03 03:09:32]

Patchset 16 (id:??) landed as
https://crrev.com/6ad356d3e845ed9b67c5fb799b2ee8290f317268
Cr-Commit-Position: refs/heads/master@{#318823}

[aberent, 2015-03-03 11:21:10]

A revert of this CL (patchset #16 id:310001) has been created in
https://codereview.chromium.org/972973002/ by aberent@chromium.org.

The reason for reverting is: This (Android only) change breaks the downstream
Android test CastSwitchVideoTest#testNewVideoNewPageSameTab.

Reverting as Android Sheriff.

BUG=463445.