Introduce new interface for MediaInternals updates.

Introduce new interface for MediaInternals updates.

New and shiny!
- Interface lives in media.
- Adds support for tracking AudioOutputStreams instead of just
just AudioOutputControllers.
- Adds support for tracking AudioInputControllers (which are 1:1
with AudioInputStreams currently).
- Changes the API to be threadsafe from all threads instead of
just the IO thread.
- Removes the nebulous OnSetAudioStreamStatus().
- Removes OnMediaEvents() from the public media API, will still be
callable from the content implementation.
- "Improves" the style of media-internals.
- Fixes JavaScript errors from firing WebUI code before the WebUI
finishes loading.
- Fixes style inconsistencies around table headers.
- Fixes stuck "copy to clipboard" window.

Screenshot of the new UI: http://i.imgur.com/lzQds3e.png

Still to be done:
- Actually log AudioOutputStreams and AudioInputStreams.

BUG=260005
TEST=New unittest! Interface works.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237447

[DaleCurtis, 2013-11-16 00:43:14]

WDYT?

I realized as I uploaded this that it has become entirely an AudioInternals
interface... I could instead have content::MediaInternals subclass
media::AudioInternals which can be passed into all the relevant media/
components.

[DaleCurtis, 2013-11-20 20:03:46]

Since scherkus@ seems overloaded and I don't send near enough reviews to
acolwell@, over to acolwell@ :)

[acolwell GONE FROM CHROMIUM, 2013-11-20 20:14:42]

It is hard to evaluate this w/o seeing the code that will use this interface.
I'm not really a fan of component, owner_id, and component_id being passed to
every method. It seems like these values should be passed in some sort of
constructor since it doesn't appear that they would change.

[DaleCurtis, 2013-11-20 21:18:24]

The interface is modeled after the existing MediaInternals class:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

You can see how it's currently used in AudioRendererHost:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

I'm proposing to extend its usage to AudioInputRendererHost and
AudioOutputDispatcherImpl.  You can see a hacky version of this implementation
using the existing interface here:

https://codereview.chromium.org/69953026/

We could probably have a wrapper class which stores owner_id and component.
Something like MedialInternals::CreateLogger(component) which would contain the
methods listed here w/o the duplication of parameters. Since owner_id is just a
ptr each wrapper class could simply become the owning pointer or use some other
id mechanism.

[DaleCurtis, 2013-11-20 21:38:36]

WDYT about an interface like:

AudioLogManager {
  scoped_ptr<AudioLog> CreateAudioLog(component) = 0;
}

Where AudioLog is:

AudioLog {
   OnAudioComponentCreated(component_id, ...);
   OnAudioComponentStarted(component_id);
   OnAudioComponentStopped(component_id);
   OnAudioComponentDeleted(component_id);
   OnAudioComponentSetVolume(component_id, ...);
}

The current browser side media-internals will extend the
AudioLogManagerInterface and contain an AudioLogImpl which forwards to the
appropriate methods behind the scenes.

[DaleCurtis, 2013-11-20 23:56:58]

PTAL, renamed to AudioLog.  Here's a prototype implementation (not ready for
review; which ends up being a substantial cleanup to MediaInternals to boot):

https://codereview.chromium.org/79923002/

[acolwell GONE FROM CHROMIUM, 2013-11-21 00:53:57]

https://codereview.chromium.org/68173025/diff/70001/media/base/audio_logging.h
File media/base/audio_logging.h (right):

https://codereview.chromium.org/68173025/diff/70001/media/base/audio_logging....
media/base/audio_logging.h:5: #ifndef MEDIA_BASE_AUDIO_LOGGING_H_
It seems like media/audio would be a better home for this. Is there any reason
not to put it in there?

https://codereview.chromium.org/68173025/diff/70001/media/base/audio_logging....
media/base/audio_logging.h:13: class AudioParameters;
nit: move this down into the media namespace below.

https://codereview.chromium.org/68173025/diff/70001/media/base/audio_logging....
media/base/audio_logging.h:19: // takes a |component_id|, a monotonically
increasing integer, along with method
nit: Why is the monotonically increasing characteristic of component_id
discussed here? It seems like it is up to the AudioLog user to determine how it
wants to assign ID's to components. Are component_id's expected to be globally
unique?

https://codereview.chromium.org/68173025/diff/70001/media/base/audio_logging....
media/base/audio_logging.h:61: class AudioLogDispenser {
nit: s/Dispenser/Factory since this appears to be a factory pattern and
dispenser is not a term used in any object names in the codebase.

[DaleCurtis, 2013-11-21 01:18:29]

https://codereview.chromium.org/68173025/diff/70001/media/base/audio_logging.h
File media/base/audio_logging.h (right):

https://codereview.chromium.org/68173025/diff/70001/media/base/audio_logging....
media/base/audio_logging.h:5: #ifndef MEDIA_BASE_AUDIO_LOGGING_H_
On 2013/11/21 00:53:58, acolwell wrote:
> It seems like media/audio would be a better home for this. Is there any reason
> not to put it in there?

Done.

https://codereview.chromium.org/68173025/diff/70001/media/base/audio_logging....
media/base/audio_logging.h:13: class AudioParameters;
On 2013/11/21 00:53:58, acolwell wrote:
> nit: move this down into the media namespace below.

Done.

https://codereview.chromium.org/68173025/diff/70001/media/base/audio_logging....
media/base/audio_logging.h:19: // takes a |component_id|, a monotonically
increasing integer, along with method
On 2013/11/21 00:53:58, acolwell wrote:
> nit: Why is the monotonically increasing characteristic of component_id
> discussed here? It seems like it is up to the AudioLog user to determine how
it
> wants to assign ID's to components. Are component_id's expected to be globally
> unique?

It'd be nice if they were, but you're right that's up to the client.

https://codereview.chromium.org/68173025/diff/70001/media/base/audio_logging....
media/base/audio_logging.h:61: class AudioLogDispenser {
On 2013/11/21 00:53:58, acolwell wrote:
> nit: s/Dispenser/Factory since this appears to be a factory pattern and
> dispenser is not a term used in any object names in the codebase.

Done.

[DaleCurtis, 2013-11-22 00:59:53]

Please take a high level look.  The CL is still missing the JavaScript
implementation, but the C++ foundation should be complete.

I'm destroying MockMediaInternals in this CL:
https://codereview.chromium.org/81953002/

[acolwell GONE FROM CHROMIUM, 2013-11-22 21:01:21]

This looks reasonable to me.

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
content/browser/media/media_internals.cc:104:
SendSingleStringUpdate(component_id, kAudioLogStatusKey, "closed", true);
Since this is the only instance where purge_cache is true, perhaps we should put
the purge logic here and remove the bool param.

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
content/browser/media/media_internals.h:27: class CONTENT_EXPORT MediaInternals
: public media::AudioLogFactory {
Since this is the only object that derives from AudioLogFactory and the only
call site actually knows it has a MediaInternals object, I wonder of the Factory
interface is really necessary. Are there other cases where the AudioLogFactory
interface would be used?

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:50: const
std::string::size_type first_brace = utf8_update.find('{');
Why do we need this?!? What other data is in this string?

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:126:
ASSERT_TRUE(update_data_.GetString("status", &status));
nit: This ASSERT_TRUE & EXPECT_EQ appears to be repeated several times. How
about an ExpectStatus("stopped") helper?

[DaleCurtis, 2013-11-22 21:16:03]

(just comments)

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
content/browser/media/media_internals.cc:104:
SendSingleStringUpdate(component_id, kAudioLogStatusKey, "closed", true);
On 2013/11/22 21:01:21, acolwell wrote:
> Since this is the only instance where purge_cache is true, perhaps we should
put
> the purge logic here and remove the bool param.

Fair enough, I originally had it that way, but it requires duplicating the
following lines:

  base::DictionaryValue dict;
  StoreComponentMetadata(component_id, &dict);
  dict.SetString(kAudioLogStatusKey, value);

Perhaps that's clearer than a bool you have to look up though.

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
content/browser/media/media_internals.h:27: class CONTENT_EXPORT MediaInternals
: public media::AudioLogFactory {
On 2013/11/22 21:01:21, acolwell wrote:
> Since this is the only object that derives from AudioLogFactory and the only
> call site actually knows it has a MediaInternals object, I wonder of the
Factory
> interface is really necessary. Are there other cases where the AudioLogFactory
> interface would be used? 

Most of the audio code lives in media/ not content/, so in order for
AudioManager and AudioOutputDispatcherImpl, etc to use the code the interface is
necessary.

My current idea is to have BrowserMainLoop pass in an AudioLogFactory to the
AudioManager (or inject one) which all browser side media/ components can access
through a public getter.

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:50: const
std::string::size_type first_brace = utf8_update.find('{');
On 2013/11/22 21:01:21, acolwell wrote:
> Why do we need this?!? What other data is in this string?

string16 content::WebUI::GetJavascriptCall("my_func", base::DictionaryValue)
turns into: "my_func({key:value,key:value,key:value});" which is passed to each
UpdateCallback().

Ultimately all this is passed to
RenderViewHost::ExecuteJavascriptInWebFrame(string16(), <update>); inside of
MediaInternalsMessageHandler.

https://codereview.chromium.org/68173025/diff/210001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:126:
ASSERT_TRUE(update_data_.GetString("status", &status));
On 2013/11/22 21:01:21, acolwell wrote:
> nit: This ASSERT_TRUE & EXPECT_EQ appears to be repeated several times. How
> about an ExpectStatus("stopped") helper?

Seems reasonable. I'll add it.

[DaleCurtis, 2013-11-25 22:57:18]

PTAL.  Implementation of UI and backend is complete minus plumbing for
AudioOutputStreams and AudioInputControllers which will be done in a followup
CL.

[acolwell GONE FROM CHROMIUM, 2013-11-26 01:33:17]

Looks pretty good to me. Just a few comments, but wanted to hold of on the
l-g-t-m until the lock concerns were resolved.

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
content/browser/media/media_internals.cc:217:
SendUpdate(SerializeUpdate(function, value));
This holds the lock while dispatching to arbitrary callbacks. Do you really want
to do that? If so, then you should probably add a AutoLock to OnMediaEvents()
and add a lock_.AssertAcquired() to SendUpdate() so it is always called in the
same locking context.

If not, then the autolocking should be moved to only protect cache_data_ &
owner_ids_ access.

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
content/browser/media/media_internals.h:75: base::Lock cache_lock_;
nit: s/cache_// since this lock appears to be protecting owner_ids_ as well? Add
a comment indicating that everything below here is protected by the lock.

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:102:
EXPECT_EQ(test_params_.input_channels(), input_channels);
nit: Helper method for ints and generalize  ExpectStatus() for strings?

https://codereview.chromium.org/68173025/diff/350001/content/browser/resource...
File content/browser/resources/media/client_renderer.js (right):

https://codereview.chromium.org/68173025/diff/350001/content/browser/resource...
content/browser/resources/media/client_renderer.js:127: for (id in components) {
If this can get large, you might consider using a DocumentFragment here to avoid
triggering relayout on each element.appendChild().

var fragment = document.createDocumentFragment();
for (id in components) {
  ....
  fragment.appendChild(li)
}
removeChildren(element);
element.appendChild(fragment);

https://codereview.chromium.org/68173025/diff/350001/content/browser/resource...
File content/browser/resources/media/manager.js (right):

https://codereview.chromium.org/68173025/diff/350001/content/browser/resource...
content/browser/resources/media/manager.js:36:
this.audioComponents_[componentType][componentId][key] =
We want to create a union of all key/values here? Why?

https://codereview.chromium.org/68173025/diff/350001/content/browser/resource...
content/browser/resources/media/manager.js:50: !(componentId in
this.audioComponents_[componentType])) {
How can this happen? Isn't it a bug if it does?

[DaleCurtis, 2013-11-26 03:02:20]

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
content/browser/media/media_internals.cc:217:
SendUpdate(SerializeUpdate(function, value));
On 2013/11/26 01:33:17, acolwell wrote:
> This holds the lock while dispatching to arbitrary callbacks. Do you really
want
> to do that? If so, then you should probably add a AutoLock to OnMediaEvents()
> and add a lock_.AssertAcquired() to SendUpdate() so it is always called in the
> same locking context.
> 
> If not, then the autolocking should be moved to only protect cache_data_ &
> owner_ids_ access.

Done.

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
content/browser/media/media_internals.h:75: base::Lock cache_lock_;
On 2013/11/26 01:33:17, acolwell wrote:
> nit: s/cache_// since this lock appears to be protecting owner_ids_ as well?
Add
> a comment indicating that everything below here is protected by the lock.

Done.

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/68173025/diff/350001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:102:
EXPECT_EQ(test_params_.input_channels(), input_channels);
On 2013/11/26 01:33:17, acolwell wrote:
> nit: Helper method for ints and generalize  ExpectStatus() for strings?

Done.

https://codereview.chromium.org/68173025/diff/350001/content/browser/resource...
File content/browser/resources/media/client_renderer.js (right):

https://codereview.chromium.org/68173025/diff/350001/content/browser/resource...
content/browser/resources/media/client_renderer.js:127: for (id in components) {
On 2013/11/26 01:33:17, acolwell wrote:
> If this can get large, you might consider using a DocumentFragment here to
avoid
> triggering relayout on each element.appendChild().
> 
> var fragment = document.createDocumentFragment();
> for (id in components) {
>   ....
>   fragment.appendChild(li)
> }
> removeChildren(element);
> element.appendChild(fragment);

Done.

https://codereview.chromium.org/68173025/diff/350001/content/browser/resource...
File content/browser/resources/media/manager.js (right):

https://codereview.chromium.org/68173025/diff/350001/content/browser/resource...
content/browser/resources/media/manager.js:36:
this.audioComponents_[componentType][componentId][key] =
On 2013/11/26 01:33:17, acolwell wrote:
> We want to create a union of all key/values here? Why?

Because each update doesn't necessary contain all the key value pairs, that's
only sent during SendEverything().  Partial updates are sent after that.

https://codereview.chromium.org/68173025/diff/350001/content/browser/resource...
content/browser/resources/media/manager.js:50: !(componentId in
this.audioComponents_[componentType])) {
On 2013/11/26 01:33:17, acolwell wrote:
> How can this happen? Isn't it a bug if it does?

It's possible the first update sent to JavaScript is "closed"

[acolwell GONE FROM CHROMIUM, 2013-11-26 17:35:07]

lgtm

[commit-bot: I haz the power, 2013-11-26 18:36:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/68173025/370001

[commit-bot: I haz the power, 2013-11-26 18:58:39]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-26 20:52:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/68173025/390001

[commit-bot: I haz the power, 2013-11-26 21:30:44]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-26 22:15:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/68173025/410001

[commit-bot: I haz the power, 2013-11-27 00:11:46]

Change committed as 237447