Make MediaInternals handle MediaLogEvents.

Make MediaInternals handle MediaLogEvents.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96650

[Scott Franklin, 2011-08-09 17:52:14]

This does not handle deletions, so the size of MediaInternals::data_ grows
indefinitely.

The next patch will handle deletion, as soon as I find the correct place to hook
into to listen for render process destruction.

[scherkus (not reviewing), 2011-08-09 20:41:10]

nit: remove the "depends on ..." from the CL description

typically that stuff goes in the initial CL message because post-commit having
the "depends on ...." message doesn't really help with commit logs

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
File chrome/browser/media/media_internals.cc (right):

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
chrome/browser/media/media_internals.cc:56: std::string source=
base::StringPrintf("media_players.%d.%d",
space before =

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
chrome/browser/media/media_internals.cc:68: // to this.details_ and send it off
to any obsevers.
this.details_ ?

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
chrome/browser/media/media_internals.cc:72:
base::Value::CreateStringValue("creating"));
creating vs created?

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
chrome/browser/media/media_internals.cc:87:
SetItemProperties(kMediaUpdateFunction, source, event.params.DeepCopy());
what's the difference between kMediaEventFunction and kMediaUpdateFunctions?

It looks like we send duplicate information

couldn't this switch code technically get coded in javascript?

[Scott Franklin, 2011-08-09 23:33:38]

I've previously been putting the "depends on" line in the description and
deleting it when the depended-upon patch lands.

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
File chrome/browser/media/media_internals.cc (right):

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
chrome/browser/media/media_internals.cc:56: std::string source=
base::StringPrintf("media_players.%d.%d",
On 2011/08/09 20:41:10, scherkus wrote:
> space before =

Done.

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
chrome/browser/media/media_internals.cc:68: // to this.details_ and send it off
to any obsevers.
On 2011/08/09 20:41:10, scherkus wrote:
> this.details_ ?

Done.

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
chrome/browser/media/media_internals.cc:72:
base::Value::CreateStringValue("creating"));
On 2011/08/09 20:41:10, scherkus wrote:
> creating vs created?

Done.

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
chrome/browser/media/media_internals.cc:87:
SetItemProperties(kMediaUpdateFunction, source, event.params.DeepCopy());
On 2011/08/09 20:41:10, scherkus wrote:
> what's the difference between kMediaEventFunction and kMediaUpdateFunctions?
> 
> It looks like we send duplicate information
> 
> couldn't this switch code technically get coded in javascript?

Yes, it could (and I meant to leave a note to that effect, but I'm not sure what
happened to it). In fact, just about all of this class could get coded in
javascript instead. this.details_ is totally unnecessary, since the data gets
duplicated in javascript anyways.

The problem is, anything not stored here is unavailable to new instances of
chrome://media-internals, so a new page would be populated by updates that it
has no context with which to handle.

This is an awkward compromise so that if you open a video, then open
chrome://media-internals, you'll still get a list of all its properties. What
you won't get is a detailed list of all the events occurring up to that point
because that would be too large to serialize to JSON and send to a renderer
every time anything changes.

I'm still trying to think of a better way for this class to operate. Recording
everything here for use in filling a newly opened tab but only sending a delta
on each update would be ideal, but that will likely result in a lot of
duplicated code.

[scherkus (not reviewing), 2011-08-10 16:00:37]

LGTM w/ nits

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
File chrome/browser/media/media_internals.cc (right):

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
chrome/browser/media/media_internals.cc:87:
SetItemProperties(kMediaUpdateFunction, source, event.params.DeepCopy());
On 2011/08/09 23:33:39, Scott Franklin wrote:
> On 2011/08/09 20:41:10, scherkus wrote:
> > what's the difference between kMediaEventFunction and kMediaUpdateFunctions?
> > 
> > It looks like we send duplicate information
> > 
> > couldn't this switch code technically get coded in javascript?
> 
> Yes, it could (and I meant to leave a note to that effect, but I'm not sure
what
> happened to it). In fact, just about all of this class could get coded in
> javascript instead. this.details_ is totally unnecessary, since the data gets
> duplicated in javascript anyways.
> 
> The problem is, anything not stored here is unavailable to new instances of
> chrome://media-internals, so a new page would be populated by updates that it
> has no context with which to handle.
> 
> This is an awkward compromise so that if you open a video, then open
> chrome://media-internals, you'll still get a list of all its properties. What
> you won't get is a detailed list of all the events occurring up to that point
> because that would be too large to serialize to JSON and send to a renderer
> every time anything changes.
> 
> I'm still trying to think of a better way for this class to operate. Recording
> everything here for use in filling a newly opened tab but only sending a delta
> on each update would be ideal, but that will likely result in a lot of
> duplicated code.

Similar to web inspector we can only log+display events only if you open
media-internals first

If it helps clean up a lot of code I'd say it's worth thinking about

http://codereview.chromium.org/7602021/diff/5001/chrome/browser/media/media_i...
chrome/browser/media/media_internals.cc:68: // to this.details_ and send it off
to any obsevers.
ack should have clarified my comment

I don't see what this.details_ refers to

Is it javascript code? or is details_ some C++ member variable?

[Scott Franklin, 2011-08-10 22:26:48]

So, without the event handling logic, this turns out to be a really, really
small CL.
Is this even worth committing? Or perhaps just roll it into a javascript-related
one?

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
File chrome/browser/media/media_internals.cc (right):

http://codereview.chromium.org/7602021/diff/1/chrome/browser/media/media_inte...
chrome/browser/media/media_internals.cc:87:
SetItemProperties(kMediaUpdateFunction, source, event.params.DeepCopy());
On 2011/08/10 16:00:37, scherkus wrote:
> On 2011/08/09 23:33:39, Scott Franklin wrote:
> > On 2011/08/09 20:41:10, scherkus wrote:
> > > what's the difference between kMediaEventFunction and
kMediaUpdateFunctions?
> > > 
> > > It looks like we send duplicate information
> > > 
> > > couldn't this switch code technically get coded in javascript?
> > 
> > Yes, it could (and I meant to leave a note to that effect, but I'm not sure
> what
> > happened to it). In fact, just about all of this class could get coded in
> > javascript instead. this.details_ is totally unnecessary, since the data
gets
> > duplicated in javascript anyways.
> > 
> > The problem is, anything not stored here is unavailable to new instances of
> > chrome://media-internals, so a new page would be populated by updates that
it
> > has no context with which to handle.
> > 
> > This is an awkward compromise so that if you open a video, then open
> > chrome://media-internals, you'll still get a list of all its properties.
What
> > you won't get is a detailed list of all the events occurring up to that
point
> > because that would be too large to serialize to JSON and send to a renderer
> > every time anything changes.
> > 
> > I'm still trying to think of a better way for this class to operate.
Recording
> > everything here for use in filling a newly opened tab but only sending a
delta
> > on each update would be ideal, but that will likely result in a lot of
> > duplicated code.
> 
> Similar to web inspector we can only log+display events only if you open
> media-internals first
> 
> If it helps clean up a lot of code I'd say it's worth thinking about

Huh? web inspector shows everything.

I think JavaScript should actually be fine.

http://codereview.chromium.org/7602021/diff/5001/chrome/browser/media/media_i...
chrome/browser/media/media_internals.cc:68: // to this.details_ and send it off
to any obsevers.
On 2011/08/10 16:00:37, scherkus wrote:
> ack should have clarified my comment
> 
> I don't see what this.details_ refers to
> 
> Is it javascript code? or is details_ some C++ member variable?

oh wow, s/details_/data_/. I imagine it would be hard to find what it refers to
when it doesn't exist.

[scherkus (not reviewing), 2011-08-10 22:47:52]

your call!

if it makes more sense w/ the other changes, then lets merge

[commit-bot: I haz the power, 2011-08-12 22:59:24]

Change committed as 96650