Use AudioStreamMonitor to control power save blocking.

Use AudioStreamMonitor to control power save blocking.

Prevents looping, soundless videos from preventing system sleep if they're
in a background tab. We don't want soundless videos in the main tab to
allow sleeping as they'll impact the muted video + captions use case.

Reworks WebContentsImpl to track active media players and whether the
attached WebContents is visible. When visible and there's an active video
player, it will create a power save blocker. One blocker is shared for all
active media players for simplicity.

Reworks AudioStreamMonitor to handle the PowerSaveBlocker for audio
such that one is only created when non-silent audio is present.

To prevent splitting power blocking duties across chrome/ and content/,
AudioStreamMonitor now lives in content/.  WebContents exposes a
WasRecentlyAudible() method for the existing tab audio indicator.

BUG=43667, 367785
TEST=manual.

Committed: https://crrev.com/484d29dcb45ca362fc9049c37dd3a51e11492f14
Cr-Commit-Position: refs/heads/master@{#294303}

[DaleCurtis, 2014-08-21 19:48:35]

Not ready for commit, just looking for discussion on this approach. miu@
discussed this in a previous email as a means to fix the linked bug (which has
been around forever).

[scherkus (not reviewing), 2014-08-21 21:30:36]

I find it mildly amusing that you link to bug 43667 yet your CL description more
or less is bug 367785 haha

questions / thoughts:
1) The "ADA" / I want to watch a movie with captions but muted. This will make
the screen go to sleep, right?
2) The listening to music case seems to now prevent display sleep vs. just
displaying app suspension, right?

[DaleCurtis, 2014-08-21 21:38:33]

On 2014/08/21 21:30:36, scherkus wrote:
> I find it mildly amusing that you link to bug 43667 yet your CL description
more
> or less is bug 367785 haha

Updated to reference both :)

> 
> questions / thoughts:
> 1) The "ADA" / I want to watch a movie with captions but muted. This will make
> the screen go to sleep, right?

Yeah, we'll no longer hold the power save blocker in this case.

> 2) The listening to music case seems to now prevent display sleep vs. just
> displaying app suspension, right?

I don't think this has changed? Per WebContentsImpl, we were holding a power
save
blocker in the audio only case too.

[scherkus (not reviewing), 2014-08-21 21:42:55]

https://codereview.chromium.org/478543003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/478543003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3013:
PowerSaveBlocker::kPowerSaveBlockPreventDisplaySleep, "Playing video");
FYI this is prevent display sleep

https://codereview.chromium.org/478543003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3018:
PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension, "Playing audio");
FYI this is the audio-only branch which only prevents app suspension

I'm not sure what the actual impact is between the two

[DaleCurtis, 2014-08-22 22:14:20]

Ah, I didn't notice the difference in those two fields. Two possible
alternatives:
- We could add a HasPlayingVideo() attribute to the public WebContents interface
which could be queried here.
- We could have WebContentsImpl query
RenderProcessHostImpl::audio_renderer_host()->HasActiveAudio() and only set the
video power save blocker if it's true. 

That fixes the audio only case, but hearing impaired / silent audio stream case
would still block since HasActiveAudio only counts physical output streams, not
whether they have non-zero energy.

[DaleCurtis, 2014-08-22 22:24:16]

To rephrase, the suggested approaches yield:

video only: no longer gets a sleep blocker.
audio only: no change.
video+silent audio: no change.

[DaleCurtis, 2014-08-22 22:44:33]

(notably this wouldn't fix the skybox.com issue since they are playing a 48kHz
audio stream at 0 volume; so I think we need the facilities from
AUdioStreamMonitor).

[miu, 2014-08-25 19:34:53]

https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
File chrome/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
chrome/browser/media/audio_stream_monitor.cc:83: #if !defined(OS_CHROMEOS) &&
!defined(OS_ANDROID)
Hmm...I'm wondering whether this is the right place for creating/owning/etc the
PowerSaveBlocker, since the purpose of AMM is just to monitor things.

However, I'm not sure where would be a better place.  It would be iffy to put
this in the UI code that handles the INVALIDATE_TYPE_TAB events from
WebContents.  Another solution would be to roll your own WebContentsObserver
whose sole purpose is to call
AMM::FromWebContents(contents)->WasRecentlyAudible() each time navigation state
has changed.

https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
chrome/browser/media/audio_stream_monitor.cc:86:
content::PowerSaveBlocker::kPowerSaveBlockPreventDisplaySleep,
You meant "AppSuspension" and not "DisplaySleep" right?

https://codereview.chromium.org/478543003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/478543003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3018:
PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension, "Playing audio");
On 2014/08/21 21:42:55, scherkus wrote:
> FYI this is the audio-only branch which only prevents app suspension
> 
> I'm not sure what the actual impact is between the two

note: It should be perfectly safe to have multiple and different
PowerSaveBlockers around at the same time.

[DaleCurtis, 2014-08-27 21:37:17]

I'm wondering if instead we might want a two part approach:

1. When a tab is not-visible, remove any video power save blockers.
2. Use AudioStreamMonitor to control AppSuspension blockers.

That way soundless video + track on the active tab will still prevent sleep, but
users with background tabs won't be impacted by the failure to sleep. WDYT?

[DaleCurtis, 2014-08-28 00:55:19]

PTAL. I've switched to the combo approach outlined since I think that most
fairly resolves all problematic cases while leaving the muted video + track in
the visible tab case working properly.

I'm torn about this code living in AudioStreamMonitor though. Suggestions for
solutions welcome. I think it'd be unfortunate to reimplement the monitor, but
maybe that's what we have to do.

https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
File chrome/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
chrome/browser/media/audio_stream_monitor.cc:83: #if !defined(OS_CHROMEOS) &&
!defined(OS_ANDROID)
On 2014/08/25 19:34:52, miu wrote:
> Hmm...I'm wondering whether this is the right place for creating/owning/etc
the
> PowerSaveBlocker, since the purpose of AMM is just to monitor things.
> 
> However, I'm not sure where would be a better place.  It would be iffy to put
> this in the UI code that handles the INVALIDATE_TYPE_TAB events from
> WebContents.  Another solution would be to roll your own WebContentsObserver
> whose sole purpose is to call
> AMM::FromWebContents(contents)->WasRecentlyAudible() each time navigation
state
> has changed.

I also wonder about this, as now the code lives across chrome and content; only
by their powers combined does it do the right thing. On the other hand it seems
unfortunate to reimplement this code. Could AudioStreamMonitor be moved to
content? Should this be integrated with WebContentsImpl some how?

https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
chrome/browser/media/audio_stream_monitor.cc:86:
content::PowerSaveBlocker::kPowerSaveBlockPreventDisplaySleep,
On 2014/08/25 19:34:52, miu wrote:
> You meant "AppSuspension" and not "DisplaySleep" right?

Acknowledged.

[miu, 2014-08-28 04:11:58]

https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
File chrome/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
chrome/browser/media/audio_stream_monitor.cc:83: #if !defined(OS_CHROMEOS) &&
!defined(OS_ANDROID)
On 2014/08/28 00:55:19, DaleCurtis wrote:
> On 2014/08/25 19:34:52, miu wrote:
> > Hmm...I'm wondering whether this is the right place for creating/owning/etc
> the
> > PowerSaveBlocker, since the purpose of ASM is just to monitor things.
> 
> I also wonder about this, as now the code lives across chrome and content;
only
> by their powers combined does it do the right thing. On the other hand it
seems
> unfortunate to reimplement this code. Could AudioStreamMonitor be moved to
> content? Should this be integrated with WebContentsImpl some how?

My opinion: Either ASM needs to move into content, or the video side of
PowerSaveBlocking needs to move into chrome.  Do we consider power save blocking
a browser-level feature, or one necessary for rendering/managing web content?

(Side note: One argument in favor of moving ASM into content is that other
browsers that use libcontent, such as Opera, would benefit from the intelligent
audio power save blocking.)

Obviously ASM's in chrome right now because it's only used for browser-level
features: the tab UI indicator and OOM killer.  Moving it into content would
require introducing a public content API.

OTOH, the steps for moving ASM into content wouldn't be too bad, but not exactly
trivial either:

1. Move the audio_stream_monitor.h/.cc files to content/browser/media, and place
the ASM class in the content namespace.  ASM has no dependencies that would make
this difficult.

2. Introduce a public content API that allows chrome code (e.g. tab_utils.cc) to
access ASM functionality.

3. Unlgue ASM from chrome/browser/media/media_capture_devices_dispatcher.cc and
just have AudioRendererHost start/stop it directly instead.  Currently, ARH
invokes MediaCaptureDevicesDispatcher through the MediaObserver interface.

[DaleCurtis, 2014-08-28 18:51:43]

dalecurtis@chromium.org changed reviewers:
+ jam@chromium.org

[DaleCurtis, 2014-08-28 18:51:43]

sky, jam: Are you guys okay with moving AudioStreamMonitor into content/ and
exposing a new content-API for it?  Doing so would:
- Prevent more accurate power blocking from being split across content/chrome.
- Allow dependent browsers (opera, CEF, etc) access to the magic behind the tab
audio indicator.

https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
File chrome/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
chrome/browser/media/audio_stream_monitor.cc:83: #if !defined(OS_CHROMEOS) &&
!defined(OS_ANDROID)
On 2014/08/28 04:11:58, miu wrote:
> On 2014/08/28 00:55:19, DaleCurtis wrote:
> > On 2014/08/25 19:34:52, miu wrote:
> > > Hmm...I'm wondering whether this is the right place for
creating/owning/etc
> > the
> > > PowerSaveBlocker, since the purpose of ASM is just to monitor things.
> > 
> > I also wonder about this, as now the code lives across chrome and content;
> only
> > by their powers combined does it do the right thing. On the other hand it
> seems
> > unfortunate to reimplement this code. Could AudioStreamMonitor be moved to
> > content? Should this be integrated with WebContentsImpl some how?
> 
> My opinion: Either ASM needs to move into content, or the video side of
> PowerSaveBlocking needs to move into chrome.  Do we consider power save
blocking
> a browser-level feature, or one necessary for rendering/managing web content?

I think we should, the ASM stuff is pretty awesome and letting dependent
browsers get access to it would be great.

> 
> (Side note: One argument in favor of moving ASM into content is that other
> browsers that use libcontent, such as Opera, would benefit from the
intelligent
> audio power save blocking.)
> 
> Obviously ASM's in chrome right now because it's only used for browser-level
> features: the tab UI indicator and OOM killer.  Moving it into content would
> require introducing a public content API.
> 
> OTOH, the steps for moving ASM into content wouldn't be too bad, but not
exactly
> trivial either:
> 
> 1. Move the audio_stream_monitor.h/.cc files to content/browser/media, and
place
> the ASM class in the content namespace.  ASM has no dependencies that would
make
> this difficult.
> 
> 2. Introduce a public content API that allows chrome code (e.g. tab_utils.cc)
to
> access ASM functionality.
> 
> 3. Unlgue ASM from chrome/browser/media/media_capture_devices_dispatcher.cc
and
> just have AudioRendererHost start/stop it directly instead.  Currently, ARH
> invokes MediaCaptureDevicesDispatcher through the MediaObserver interface.

This doesn't seem too bad.  I'll move forward with this assuming sky/jam are
onboard.

[sky, 2014-08-28 23:04:39]

On Thu, Aug 28, 2014 at 11:51 AM,  <dalecurtis@chromium.org> wrote:
> sky, jam: Are you guys okay with moving AudioStreamMonitor into content/ and
> exposing a new content-API for it?

John has a better feel for that than I do.

  -Scott

>   Doing so would:
> - Prevent more accurate power blocking from being split across
> content/chrome.
> - Allow dependent browsers (opera, CEF, etc) access to the magic behind the
> tab
> audio indicator.
>
>
>
>
https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
> File chrome/browser/media/audio_stream_monitor.cc (right):
>
>
https://codereview.chromium.org/478543003/diff/20001/chrome/browser/media/aud...
> chrome/browser/media/audio_stream_monitor.cc:83: #if
> !defined(OS_CHROMEOS) && !defined(OS_ANDROID)
> On 2014/08/28 04:11:58, miu wrote:
>>
>> On 2014/08/28 00:55:19, DaleCurtis wrote:
>> > On 2014/08/25 19:34:52, miu wrote:
>> > > Hmm...I'm wondering whether this is the right place for
>
> creating/owning/etc
>>
>> > the
>> > > PowerSaveBlocker, since the purpose of ASM is just to monitor
>
> things.
>>
>> >
>> > I also wonder about this, as now the code lives across chrome and
>
> content;
>>
>> only
>> > by their powers combined does it do the right thing. On the other
>
> hand it
>>
>> seems
>> > unfortunate to reimplement this code. Could AudioStreamMonitor be
>
> moved to
>>
>> > content? Should this be integrated with WebContentsImpl some how?
>
>
>> My opinion: Either ASM needs to move into content, or the video side
>
> of
>>
>> PowerSaveBlocking needs to move into chrome.  Do we consider power
>
> save blocking
>>
>> a browser-level feature, or one necessary for rendering/managing web
>
> content?
>
> I think we should, the ASM stuff is pretty awesome and letting dependent
> browsers get access to it would be great.
>
>
>
>> (Side note: One argument in favor of moving ASM into content is that
>
> other
>>
>> browsers that use libcontent, such as Opera, would benefit from the
>
> intelligent
>>
>> audio power save blocking.)
>
>
>> Obviously ASM's in chrome right now because it's only used for
>
> browser-level
>>
>> features: the tab UI indicator and OOM killer.  Moving it into content
>
> would
>>
>> require introducing a public content API.
>
>
>> OTOH, the steps for moving ASM into content wouldn't be too bad, but
>
> not exactly
>>
>> trivial either:
>
>
>> 1. Move the audio_stream_monitor.h/.cc files to content/browser/media,
>
> and place
>>
>> the ASM class in the content namespace.  ASM has no dependencies that
>
> would make
>>
>> this difficult.
>
>
>> 2. Introduce a public content API that allows chrome code (e.g.
>
> tab_utils.cc) to
>>
>> access ASM functionality.
>
>
>> 3. Unlgue ASM from
>
> chrome/browser/media/media_capture_devices_dispatcher.cc and
>>
>> just have AudioRendererHost start/stop it directly instead.
>
> Currently, ARH
>>
>> invokes MediaCaptureDevicesDispatcher through the MediaObserver
>
> interface.
>
> This doesn't seem too bad.  I'll move forward with this assuming sky/jam
> are onboard.
>
> https://codereview.chromium.org/478543003/

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

[jam, 2014-08-28 23:45:05]

On 2014/08/28 18:51:43, DaleCurtis wrote:
> sky, jam: Are you guys okay with moving AudioStreamMonitor into content/ and
> exposing a new content-API for it?  Doing so would:
> - Prevent more accurate power blocking from being split across content/chrome.
> - Allow dependent browsers (opera, CEF, etc) access to the magic behind the
tab
> audio indicator.

sure. what kind of API would be exposed?

[DaleCurtis, 2014-08-29 00:49:55]

Specifically, I was thinking AudioStreamMonitor would move into
content/browser/media and WebContents would expose a content/public
WasRecentlyAudible() method. The RenderProcessHostImpl would own the
AudioStreamMonitor and pass it into the AudioRendererHost during construction.
RenderProcessHostImpl would expose an accessor for the AudioStreamMonitor just
like it does for the AudioRendererHost today. WebContentsImpl would use this
accessor to field calls to its public WasRecentlyAudible() method.

The AudioRendererHost would now call AudioStreamMonitor::StartMonitoringStream()
directly instead of through the MediaObserver used by chrome/.  We could then
remove the corresponding MediaObserver API if we're so inclined.

The chrome/ MediaObserver (MediaCaptureDevices) would stop doing anything with
the OnAudioStreamPlaying sent today.

Longer term if/when the tab audio indicator starts using the power level and
clipping information we throw away today, we can expose something like a
GetAudioPowerLevel(*power_level, *clipping) method on the WebContents.

[jam, 2014-08-29 17:20:50]

On 2014/08/29 00:49:55, DaleCurtis wrote:
> Specifically, I was thinking AudioStreamMonitor would move into
> content/browser/media and WebContents would expose a content/public
> WasRecentlyAudible() method. The RenderProcessHostImpl would own the
> AudioStreamMonitor and pass it into the AudioRendererHost during construction.
> RenderProcessHostImpl would expose an accessor for the AudioStreamMonitor just
> like it does for the AudioRendererHost today. WebContentsImpl would use this
> accessor to field calls to its public WasRecentlyAudible() method.
> 
> The AudioRendererHost would now call
AudioStreamMonitor::StartMonitoringStream()
> directly instead of through the MediaObserver used by chrome/.  We could then
> remove the corresponding MediaObserver API if we're so inclined.
> 
> The chrome/ MediaObserver (MediaCaptureDevices) would stop doing anything with
> the OnAudioStreamPlaying sent today.
> 
> Longer term if/when the tab audio indicator starts using the power level and
> clipping information we throw away today, we can expose something like a
> GetAudioPowerLevel(*power_level, *clipping) method on the WebContents.

sgtm

[DaleCurtis, 2014-08-29 18:19:41]

Yuri kindly informed me of some misconceptions I had. The WebContents may change
RenderProcessHostImpls over time, so we can't tie the AudioStreamMonitor to
RenderProcessHostImpl without significant changes in how it works.

Instead, I'll amend the above plan to make fewer changes, the AudioStreamMonitor
will continue to be owned by WebContentsImpl via the WebContentsUserData schema.
AudioStreamMonitor will now expose two static methods like so:

static void StartMonitoringStream(process_id, frame_id, host_id, stream_id,
callback);
static void StopMonitoringStream(process_id, frame_id, host_id, stream_id,
callback);

These static methods will handle posting to the UI thread and looking up the
correct WebContents instance just like MediaStreamCaptureDevices does in the
chrome/-world today. The existing non-static StartMonitoringStream and
StopMonitoringStream methods on AudioStreamMonitor will be privatized.
AudioRendererHost will use the static helper methods on AudioStreamMonitor.

The rest of the plan will remain the same modulo WebContentsImpl no longer needs
to talk to the RenderProcessHostImpl.

[DaleCurtis, 2014-08-29 23:56:47]

Okay all moved.  PTAL, but no rush, I won't be back until Wednesday.

https://codereview.chromium.org/478543003/diff/80001/content/browser/media/au...
File content/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/80001/content/browser/media/au...
content/browser/media/audio_stream_monitor.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Sadly codereview isn't detecting the diff here, but the only new code sections
are the five static methods at the top of this file, plus the power save blocker
from the original patch sets.

https://codereview.chromium.org/478543003/diff/80001/content/browser/media/au...
content/browser/media/audio_stream_monitor.cc:36: // TODO(dalecurtis): ????
DEFINE_WEB_CONTENTS_USER_DATA_KEY won't link,
Not sure what to do about this.  Sadly there are no other unittest users of
WebContentsUserData that don't define the data inside the actual unittest.

https://codereview.chromium.org/478543003/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/478543003/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/audio_renderer_host.cc:29: #define
AUDIO_STREAM_MONITORING
It's unfortunate to define this everywhere. I'm open to suggestions such as
stubbing out the ASM methods for Android/IOS so multiple places don't have this
define gunk.

[DaleCurtis, 2014-08-29 23:59:26]

Yuri showed me --similirity=10, so the latest patch set got the move detection
correctly!  Please see the comments on PS#3 though.

[jam, 2014-09-02 19:50:33]

content lgtm with nits

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:22: content::WebContents* const
web_contents =
nit: remove content:: from this file

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.h (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:33: // content::WebContents
instance (referred to as "the tab" in comments below).
update comment to remove "content::"

https://codereview.chromium.org/478543003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:29: #define
AUDIO_STREAM_MONITORING
ditto

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

https://codereview.chromium.org/478543003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:117: #define
AUDIO_STREAM_MONITORING
nit: use a boolean member in this class that is initialized through
'!defined(OS_ANDROID) && !defined(OS_IOS)', but don't define ifdef's and use it
through the file. that is confusing and not a standard way of enabling stuff per
platform

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

https://codereview.chromium.org/478543003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:984:
scoped_ptr<content::PowerSaveBlocker> power_save_blocker_;
no "content::" in content namespace

[miu, 2014-09-02 21:04:40]

https://codereview.chromium.org/478543003/diff/100001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (left):

https://codereview.chromium.org/478543003/diff/100001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1036:
'browser/media/cast_transport_host_filter_unittest.cc',
I don't think you meant to delete this line.

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:169: blocker_ =
content::PowerSaveBlocker::Create(
Can we move this into WebContentsImpl now?  Meaning, in
WebContentsImpl::NotifyNavigationStateChanged(), add something like:

  if (WasRecentlyAudible()) {
    // Make sure WebContentsImpl::power_save_blocker_ is instantiated.
  } else {
    // Make sure there is no power_save_blocker_ unless there is video playing.
  }

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.h (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:59: int stream_id,
This just occurred to me: Are stream_ids unique over an entire browser, or just
inside one render process?  Maybe the internals of ASM need to key by
render_process_id+stream_id, and not just stream_id by itself?

To be clear, if the change is needed, this would be fixing a pre-existing bug.

https://codereview.chromium.org/478543003/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/478543003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3015: blocker =
PowerSaveBlocker::Create(
I'm not sure the blocker is set up correctly for platforms where
AUDIO_STREAM_MONITORING is not defined.  It seems like if only audio is playing,
we will be creating a "PreventDisplaySleep" blocker.  Also, the !IsHidden()
check (a dozen LOC down) isn't relevant for audio, right?

Given my comments elsewhere (in ASM.cc about moving the PowerSaveBlocker into
WebContentsImpl), perhaps you might retain this code and call this method (with
has_audio=true) from WCImpl::NotifyNavigationStateChanged()?

[DaleCurtis, 2014-09-03 18:48:55]

Just comment updates. Please reply.

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:36: // TODO(dalecurtis): ????
DEFINE_WEB_CONTENTS_USER_DATA_KEY won't link,
No ideas on what's going on here? :)

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:169: blocker_ =
content::PowerSaveBlocker::Create(
On 2014/09/02 21:04:39, miu wrote:
> Can we move this into WebContentsImpl now?  Meaning, in
> WebContentsImpl::NotifyNavigationStateChanged(), add something like:
> 
>   if (WasRecentlyAudible()) {
>     // Make sure WebContentsImpl::power_save_blocker_ is instantiated.
>   } else {
>     // Make sure there is no power_save_blocker_ unless there is video
playing.
>   }
> 

Let me play with it. I think that's a great idea if it works out.

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.h (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:59: int stream_id,
On 2014/09/02 21:04:39, miu wrote:
> This just occurred to me: Are stream_ids unique over an entire browser, or
just
> inside one render process?  Maybe the internals of ASM need to key by
> render_process_id+stream_id, and not just stream_id by itself?
> 
> To be clear, if the change is needed, this would be fixing a pre-existing bug.

Yeah, I noticed this too when refactoring and hadn't had a chance to sleuth it
out yet. The stream ids are created from an IDMap owned by AudioMessageFilter
which is in turn owned by the RenderThreadImpl.

My knowledge is a bit out of date on whether RenderThreadImpl ->
RenderProcessHostImpl is a 1:1 mapping. I'd guess not though... miu@, jam@ can
you confirm? If not I'll fix that in a separate patch before landing this one.

https://codereview.chromium.org/478543003/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/478543003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3015: blocker =
PowerSaveBlocker::Create(
On 2014/09/02 21:04:39, miu wrote:
> I'm not sure the blocker is set up correctly for platforms where
> AUDIO_STREAM_MONITORING is not defined.  It seems like if only audio is
playing,
> we will be creating a "PreventDisplaySleep" blocker.  Also, the !IsHidden()
> check (a dozen LOC down) isn't relevant for audio, right?
> 
> Given my comments elsewhere (in ASM.cc about moving the PowerSaveBlocker into
> WebContentsImpl), perhaps you might retain this code and call this method
(with
> has_audio=true) from WCImpl::NotifyNavigationStateChanged()?

It ends up being correct since Androids PowerSaveBlocker is the same regardless
of type, but I agree that it's hacky. I'll see what I can do about making this
clearer.

[miu, 2014-09-03 22:41:53]

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:36: // TODO(dalecurtis): ????
DEFINE_WEB_CONTENTS_USER_DATA_KEY won't link,
On 2014/09/03 18:48:55, DaleCurtis wrote:
> No ideas on what's going on here? :)

If you were compiling on Windows, I would suggest:

class CONTENT_EXPORT AudioStreamMonitor
    : NON_EXPORTED_BASE(public WebContentsUserData<AudioStreamMonitor>) {
      ^^^^^^^^^^^^^^^^^

But on posix, I'm not sure.  Is it when you link chrome or content_unittests?

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.h (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:59: int stream_id,
On 2014/09/03 18:48:55, DaleCurtis wrote:
> On 2014/09/02 21:04:39, miu wrote:
> > This just occurred to me: Are stream_ids unique over an entire browser, or
> just
> > inside one render process?  Maybe the internals of ASM need to key by
> > render_process_id+stream_id, and not just stream_id by itself?
> > 
> > To be clear, if the change is needed, this would be fixing a pre-existing
bug.
> 
> Yeah, I noticed this too when refactoring and hadn't had a chance to sleuth it
> out yet. The stream ids are created from an IDMap owned by AudioMessageFilter
> which is in turn owned by the RenderThreadImpl.

If the stream IDs are created in the render process, there can't be a guarantee
that they will be unique over the whole browser.  Looks like ID map always vends
out integer IDs starting at 1.  So, yep, I'm pretty sure this is a bug that
needs to be fixed.

[DaleCurtis, 2014-09-04 21:59:06]

PTAL.  All power management is back in WebContentsImpl, so things have changed a
bit since your lgtm jam@, please look over it again.

I've added an extra unittest to WebContentsImpl for the new behavior.

https://codereview.chromium.org/478543003/diff/100001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (left):

https://codereview.chromium.org/478543003/diff/100001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1036:
'browser/media/cast_transport_host_filter_unittest.cc',
On 2014/09/02 21:04:39, miu wrote:
> I don't think you meant to delete this line.

Whoops!

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:22: content::WebContents* const
web_contents =
On 2014/09/02 19:50:33, jam wrote:
> nit: remove content:: from this file

Done.

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:36: // TODO(dalecurtis): ????
DEFINE_WEB_CONTENTS_USER_DATA_KEY won't link,
On 2014/09/03 22:41:53, miu wrote:
> On 2014/09/03 18:48:55, DaleCurtis wrote:
> > No ideas on what's going on here? :)
> 
> If you were compiling on Windows, I would suggest:
> 
> class CONTENT_EXPORT AudioStreamMonitor
>     : NON_EXPORTED_BASE(public WebContentsUserData<AudioStreamMonitor>) {
>       ^^^^^^^^^^^^^^^^^
> 
> But on posix, I'm not sure.  Is it when you link chrome or content_unittests?

Correct. I tried NON_EXPORTED_BASE before creating what you see below, no luck
:|

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:169: blocker_ =
content::PowerSaveBlocker::Create(
On 2014/09/03 18:48:55, DaleCurtis wrote:
> On 2014/09/02 21:04:39, miu wrote:
> > Can we move this into WebContentsImpl now?  Meaning, in
> > WebContentsImpl::NotifyNavigationStateChanged(), add something like:
> > 
> >   if (WasRecentlyAudible()) {
> >     // Make sure WebContentsImpl::power_save_blocker_ is instantiated.
> >   } else {
> >     // Make sure there is no power_save_blocker_ unless there is video
> playing.
> >   }
> > 
> 
> Let me play with it. I think that's a great idea if it works out.

Done.

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.h (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:33: // content::WebContents
instance (referred to as "the tab" in comments below).
On 2014/09/02 19:50:33, jam wrote:
> update comment to remove "content::"

Done.

https://codereview.chromium.org/478543003/diff/100001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:59: int stream_id,
On 2014/09/03 22:41:53, miu wrote:
> On 2014/09/03 18:48:55, DaleCurtis wrote:
> > On 2014/09/02 21:04:39, miu wrote:
> > > This just occurred to me: Are stream_ids unique over an entire browser, or
> > just
> > > inside one render process?  Maybe the internals of ASM need to key by
> > > render_process_id+stream_id, and not just stream_id by itself?
> > > 
> > > To be clear, if the change is needed, this would be fixing a pre-existing
> bug.
> > 
> > Yeah, I noticed this too when refactoring and hadn't had a chance to sleuth
it
> > out yet. The stream ids are created from an IDMap owned by
AudioMessageFilter
> > which is in turn owned by the RenderThreadImpl.
> 
> If the stream IDs are created in the render process, there can't be a
guarantee
> that they will be unique over the whole browser.  Looks like ID map always
vends
> out integer IDs starting at 1.  So, yep, I'm pretty sure this is a bug that
> needs to be fixed.

Fixed, not too hard. Just added a std::pair<int,int> for the map key.

https://codereview.chromium.org/478543003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/478543003/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:29: #define
AUDIO_STREAM_MONITORING
On 2014/09/02 19:50:33, jam wrote:
> ditto

Done.

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

https://codereview.chromium.org/478543003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:117: #define
AUDIO_STREAM_MONITORING
On 2014/09/02 19:50:33, jam wrote:
> nit: use a boolean member in this class that is initialized through
> '!defined(OS_ANDROID) && !defined(OS_IOS)', but don't define ifdef's and use
it
> through the file. that is confusing and not a standard way of enabling stuff
per
> platform

I instead plumbing a bit which is defined where this decision is made, so we
don't have multiple clients reimplementing this.  AudioOutputController now
decides whether or not it will monitor and the audio stream monitor keys off of
that.

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

https://codereview.chromium.org/478543003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:984:
scoped_ptr<content::PowerSaveBlocker> power_save_blocker_;
On 2014/09/02 19:50:33, jam wrote:
> no "content::" in content namespace

Done.

[miu, 2014-09-05 18:20:25]

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/O...
File content/browser/media/OWNERS (right):

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/O...
content/browser/media/OWNERS:20: per-file
media_stream_capture_indicator*=miu@chromium.org
Don't need this 2nd line (no media_stream_capture_indicator.* here).

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.h (right):

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:39: // AudioStreamMonitor will
additionally create a PowerSaveBlocker to prevent app
Not true anymore, since you moved that into WebContentsImpl.

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:46: static bool
tab_audio_indicator_available() {
naming: Let's not refer to chrome-level concepts in content (and there is more
than just this use case).  How about naming this is_available() or
can_monitor()?

Also, the comment here needs similar adjustment (and other comments elsewhere in
this change).

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:138: typedef std::pair<int, int>
StreamID;
Thanks for fixing this!  :)

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

https://codereview.chromium.org/478543003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1087:
video_power_save_blocker_.reset();
Seems like this should go in the block of code a few lines above.  If a tab is
being captured, it's probably right to keep the video power save blocker
instantiated.

https://codereview.chromium.org/478543003/diff/120001/media/audio/audio_outpu...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/478543003/diff/120001/media/audio/audio_outpu...
media/audio/audio_output_controller.h:248: // Scans audio samples from
OnMoreData() as input to compute power levels.
Not sure whether I agree with taking things this far.  Seems like a total waste
to compile this in for the unsupported platforms.  Perhaps jam@ can comment?

[DaleCurtis, 2014-09-05 18:30:15]

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

https://codereview.chromium.org/478543003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1087:
video_power_save_blocker_.reset();
On 2014/09/05 18:20:25, miu wrote:
> Seems like this should go in the block of code a few lines above.  If a tab is
> being captured, it's probably right to keep the video power save blocker
> instantiated.

Fair enough, I thought capturing already had its own power save blockers though?
Which reminds me I need to add tests for WasHidden/WasShown in relation to the
power save blocker. I'll do that in the next PS.

https://codereview.chromium.org/478543003/diff/120001/media/audio/audio_outpu...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/478543003/diff/120001/media/audio/audio_outpu...
media/audio/audio_output_controller.h:248: // Scans audio samples from
OnMoreData() as input to compute power levels.
On 2014/09/05 18:20:25, miu wrote:
> Not sure whether I agree with taking things this far.  Seems like a total
waste
> to compile this in for the unsupported platforms.  Perhaps jam@ can comment?

I was hoping the compiler would be smart enough to realize it's unused since the
static above will compile to false or true.

[miu, 2014-09-05 18:44:31]

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

https://codereview.chromium.org/478543003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1087:
video_power_save_blocker_.reset();
On 2014/09/05 18:30:14, DaleCurtis wrote:
> On 2014/09/05 18:20:25, miu wrote:
> > Seems like this should go in the block of code a few lines above.  If a tab
is
> > being captured, it's probably right to keep the video power save blocker
> > instantiated.
> 
> Fair enough, I thought capturing already had its own power save blockers
though?
> Which reminds me I need to add tests for WasHidden/WasShown in relation to the
> power save blocker. I'll do that in the next PS.

It's a subtle point: It's not *capturing* that has its own blocker, it's
*streaming* (i.e., when remoting content).  So, it's possible to capture a tab,
hide it, and then have its video stream being played back in another tab in the
same browser.  (This is generally only useful for testing/development of tab
capture changes, but useful nonetheless.)

[DaleCurtis, 2014-09-05 22:13:35]

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/O...
File content/browser/media/OWNERS (right):

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/O...
content/browser/media/OWNERS:20: per-file
media_stream_capture_indicator*=miu@chromium.org
On 2014/09/05 18:20:25, miu wrote:
> Don't need this 2nd line (no media_stream_capture_indicator.* here).

Done.

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.h (right):

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:39: // AudioStreamMonitor will
additionally create a PowerSaveBlocker to prevent app
On 2014/09/05 18:20:25, miu wrote:
> Not true anymore, since you moved that into WebContentsImpl.

Done.

https://codereview.chromium.org/478543003/diff/120001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:46: static bool
tab_audio_indicator_available() {
On 2014/09/05 18:20:25, miu wrote:
> naming: Let's not refer to chrome-level concepts in content (and there is more
> than just this use case).  How about naming this is_available() or
> can_monitor()?
> 
> Also, the comment here needs similar adjustment (and other comments elsewhere
in
> this change).

Done.

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

https://codereview.chromium.org/478543003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1087:
video_power_save_blocker_.reset();
On 2014/09/05 18:44:31, miu wrote:
> On 2014/09/05 18:30:14, DaleCurtis wrote:
> > On 2014/09/05 18:20:25, miu wrote:
> > > Seems like this should go in the block of code a few lines above.  If a
tab
> is
> > > being captured, it's probably right to keep the video power save blocker
> > > instantiated.
> > 
> > Fair enough, I thought capturing already had its own power save blockers
> though?
> > Which reminds me I need to add tests for WasHidden/WasShown in relation to
the
> > power save blocker. I'll do that in the next PS.
> 
> It's a subtle point: It's not *capturing* that has its own blocker, it's
> *streaming* (i.e., when remoting content).  So, it's possible to capture a
tab,
> hide it, and then have its video stream being played back in another tab in
the
> same browser.  (This is generally only useful for testing/development of tab
> capture changes, but useful nonetheless.)

Done. Did you want to modify IncrementCapturerCount / DecrementCapturerCount to
handle this correctly too?  It gets a bit messier if so.

[miu, 2014-09-06 02:51:00]

lgtm

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

https://codereview.chromium.org/478543003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1087:
video_power_save_blocker_.reset();
On 2014/09/05 22:13:35, DaleCurtis wrote:
> Done. Did you want to modify IncrementCapturerCount / DecrementCapturerCount
to
> handle this correctly too?  It gets a bit messier if so.

Looks like it already does.  IIUC, DecrementCapturerCount() will call
WasHidden() after the count goes to zero, which should reset
video_power_save_blocker_.

[DaleCurtis, 2014-09-09 00:41:23]

dalecurtis@chromium.org changed reviewers:
- sky@chromium.org

[DaleCurtis, 2014-09-09 00:41:23]

jam, scherkus: Ping?
sky to CC since jam@ is owner for all.

[scherkus (not reviewing), 2014-09-09 21:07:41]

nits and questions

lgtm otherwise

https://codereview.chromium.org/478543003/diff/140001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/140001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:44: CONTENT_EXPORT int
WebContentsUserData<AudioStreamMonitor>::kLocatorKey = 0;
as far as my code searching skills can tell, AudioStreamMonitor is the only
class that both inherits from WebContentsUserData<T> as well as is an exported
class (i.e., has unit tests)

https://codereview.chromium.org/478543003/diff/140001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:60: /* static */
// static

here and below

https://codereview.chromium.org/478543003/diff/140001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.h (right):

https://codereview.chromium.org/478543003/diff/140001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:43: static bool
monitoring_available() {
what's the difference between calling
content::AudioStreamMonitor::monitoring_available() and calling
media::AOC::w_m_a_l() directly?

[DaleCurtis, 2014-09-10 00:51:15]

https://codereview.chromium.org/478543003/diff/140001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.cc (right):

https://codereview.chromium.org/478543003/diff/140001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:44: CONTENT_EXPORT int
WebContentsUserData<AudioStreamMonitor>::kLocatorKey = 0;
On 2014/09/09 21:07:41, scherkus wrote:
> as far as my code searching skills can tell, AudioStreamMonitor is the only
> class that both inherits from WebContentsUserData<T> as well as is an exported
> class (i.e., has unit tests)

Yeah, that's the only reason I can see this hasn't been hit before.

https://codereview.chromium.org/478543003/diff/140001/content/browser/media/a...
content/browser/media/audio_stream_monitor.cc:60: /* static */
On 2014/09/09 21:07:41, scherkus wrote:
> // static
> 
> here and below

Done.

https://codereview.chromium.org/478543003/diff/140001/content/browser/media/a...
File content/browser/media/audio_stream_monitor.h (right):

https://codereview.chromium.org/478543003/diff/140001/content/browser/media/a...
content/browser/media/audio_stream_monitor.h:43: static bool
monitoring_available() {
On 2014/09/09 21:07:41, scherkus wrote:
> what's the difference between calling
> content::AudioStreamMonitor::monitoring_available() and calling
> media::AOC::w_m_a_l() directly?

Just one of semantics, WCI wants to know if stream monitoring is available.
Stream monitoring is only available if audio power level analysis is available.
We could have only one method on AOC, but content/ callers would be inferring
internal details about what ASM is doing with the AOC in that case.

[jam, 2014-09-10 18:29:27]

lgtm

[DaleCurtis, 2014-09-10 22:15:33]

Patchset #8 (id:180001) has been deleted

[DaleCurtis, 2014-09-10 22:20:10]

I ended up having to drop the WebContentsUserData, I couldn't figure out a clean
way to get required template incantations to work with both MSVC++ and gcc/clang
when the class is exported. Instead of adding some nasty specializations and
ifdefs to the header file, instead I now just have the WebContentsImpl directly
own the AudioStreamMonitor.

This is a relatively trivial change, so barring any outcry I'll CQ this shortly.

[DaleCurtis, 2014-09-10 23:09:22]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:18:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/478543003/200001

[DaleCurtis, 2014-09-10 23:54:48]

The CQ bit was unchecked by dalecurtis@chromium.org

[DaleCurtis, 2014-09-11 01:09:45]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-09-11 01:16:44]

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

[commit-bot: I haz the power, 2014-09-11 02:53:09]

Committed patchset #9 (id:220001) as 2a3da1f4c387720dc12c21784ec9af2a1df99100

[commit-bot: I haz the power, 2014-09-11 03:09:28]

Patchset 9 (id:??) landed as
https://crrev.com/484d29dcb45ca362fc9049c37dd3a51e11492f14
Cr-Commit-Position: refs/heads/master@{#294303}

[DaleCurtis, 2014-09-11 18:02:13]

Hmm, this got reverted due to some DCHECK failures here:

https://codereview.chromium.org/562863002/

Not sure why it didn't link back to this CL.  I'll take a look and put up a
follow up patch set.

[DaleCurtis, 2014-09-11 19:38:40]

Turns out the new DCHECKs exposed some bugs, I'll revert the revert after the
fixes land:

https://codereview.chromium.org/564943002/