Display active media players on chrome://media-internals.

Display active media players on chrome://media-internals.

BUG=
TEST=

[Scott Franklin, 2011-08-15 22:08:56]

It's quite a bit of js, but I attempted to keep everything organized and broken
off into standalone chunks.

[Evan Stade, 2011-08-16 19:12:59]

+arv

[scherkus (not reviewing), 2011-08-16 19:18:29]

I completely suck at reviewing JS sorry :(

I think we'll need estade/arv to help with this one.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/cache_entry.js:223: CacheEntry:
CacheEntry,
aren't trailing , bad in  JS?

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/event_list.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/event_list.js:47: },
trailing , ?

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/event_list.js:51: EventList: EventList,
trailing , ?

[arv (Not doing code reviews), 2011-08-16 19:29:03]

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/cache_entry.js:42: clearControl.href =
'#';
this change is not for the better. Now we get a history entry

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/cache_entry.js:223: CacheEntry:
CacheEntry,
undo change

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/event_list.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/event_list.js:20: var hrow =
document.createElement('tr');
hRow

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/event_list.js:42: params = [];
missing var

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/media_internals.css (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_internals.css:19: padding: 0px
10px;
s/0px/0/

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/media_internals.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_internals.js:86:
printMediaPlayers = function() {
missing var here and in other places

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_internals.js:88: for (id in
mediaPlayers)
missing var

Maybe you should swith to strict mode to catch theses errors?

cr.define('media', function() {
  'use strict';

  ...
});

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_internals.js:140: case
'audio_streams':
indent

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_internals.js:260: redrawTimer =
setTimeout("printMediaPlayers()", 50);
Don't use string based setTimout since it is a global eval.

redrawTimer = setTimeout(printMediaPlayers, 50);

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/media_player.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_player.js:39: for (key in
event.params)
missing var. Use strict mode?

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_player.js:39: for (key in
event.params)
Missing {

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_player.js:49: * Render this
MediaPlayer as a <li>.
Why don't you just make MediaPlayer a sub class of HTMLLIElement then?

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/metrics.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/metrics.js:9: metricProperties = {
missing vars again

[Scott Franklin, 2011-08-16 23:33:52]

That's a neat trick to inherit from HTMLElements. I went ahead and did the same
for Metrics and EventList, which are also displayed as <li>s.

CacheEntry works the same way as well, but a fix for that can go in a separate
patch.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/cache_entry.js:42: clearControl.href =
'#';
On 2011/08/16 19:29:03, arv wrote:
> this change is not for the better. Now we get a history entry

It did work fine before, but Content Security Policy throws an error with
javascript: urls. Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/cache_entry.js:223: CacheEntry:
CacheEntry,
On 2011/08/16 19:29:03, arv wrote:
> undo change

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/event_list.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/event_list.js:20: var hrow =
document.createElement('tr');
On 2011/08/16 19:29:03, arv wrote:
> hRow

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/event_list.js:42: params = [];
On 2011/08/16 19:29:03, arv wrote:
> missing var

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/event_list.js:47: },
On 2011/08/16 19:18:29, scherkus wrote:
> trailing , ?

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/event_list.js:51: EventList: EventList,
On 2011/08/16 19:18:29, scherkus wrote:
> trailing , ?

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/media_internals.css (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_internals.css:19: padding: 0px
10px;
On 2011/08/16 19:29:03, arv wrote:
> s/0px/0/

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/media_internals.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_internals.js:86:
printMediaPlayers = function() {
On 2011/08/16 19:29:03, arv wrote:
> missing var here and in other places

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_internals.js:88: for (id in
mediaPlayers)
On 2011/08/16 19:29:03, arv wrote:
> missing var
> 
> Maybe you should swith to strict mode to catch theses errors?
> 
> cr.define('media', function() {
>   'use strict';
> 
>   ...
> });

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_internals.js:140: case
'audio_streams':
On 2011/08/16 19:29:03, arv wrote:
> indent

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_internals.js:260: redrawTimer =
setTimeout("printMediaPlayers()", 50);
On 2011/08/16 19:29:03, arv wrote:
> Don't use string based setTimout since it is a global eval.
> 
> redrawTimer = setTimeout(printMediaPlayers, 50);

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/media_player.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_player.js:39: for (key in
event.params)
On 2011/08/16 19:29:03, arv wrote:
> missing var. Use strict mode?

Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_player.js:39: for (key in
event.params)
On 2011/08/16 19:29:03, arv wrote:
> Missing {

Didn't realize those were necessary one single-line loops. Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/media_player.js:49: * Render this
MediaPlayer as a <li>.
On 2011/08/16 19:29:03, arv wrote:
> Why don't you just make MediaPlayer a sub class of HTMLLIElement then?

Because I didn't realize such a thing was possible :)
Done.

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
File chrome/browser/resources/media_internals/metrics.js (right):

http://codereview.chromium.org/7653001/diff/1/chrome/browser/resources/media_...
chrome/browser/resources/media_internals/metrics.js:9: metricProperties = {
On 2011/08/16 19:29:03, arv wrote:
> missing vars again

Done.

[Scott Franklin, 2011-08-17 22:08:26]

arv@, any more thoughts on this?

[scherkus (not reviewing), 2011-08-29 22:15:23]

ping on behalf of scottfr

[scherkus (not reviewing), 2011-09-21 16:50:55]

ping?

[arv (Not doing code reviews), 2011-09-22 19:26:03]

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/event_list.js (right):

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/event_list.js:29: this.startTime_ =
null;
This is better done on the prototype

startTime_: null,

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/media_internals.js (right):

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:43: var initialize =
function() {
Please use function statement whenever valid (top level and in function scope)

function initialize() {

}

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:245: new
media.MediaPlayer({'id': source, 'renderer': event.renderer});
{id: ..., event: ...}

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/media_player.js (right):

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_player.js:23: this.id = null;
renderer and id can go on the prototype

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/metrics.js (right):

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:11: 'pipeline_state': true,
no underscores in JS?

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:11: 'pipeline_state': true,
Don't quote property names unless you have to

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:17: 'seek': {'start':
'SEEK',
The indentation of this is strange. Please line break after { and indent 2
spaces

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:82: m.avg.textContent =
round(m.total / m.count);
Math.round?

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/util.js (right):

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/util.js:41: * Truncates a URL to
MAX_URL_LENGTH. If the URL was too long, the last
Use CSS instead.

overflow: hidden;
text-overflow: ellipsis;

[scherkus (not reviewing), 2011-09-23 06:52:41]

*** PATCH IS MOVING TO http://codereview.chromium.org/7972028/ ***

My intern owns this issue so I can't upload any more patches to it. I'll close
this issue.

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/event_list.js (right):

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/event_list.js:29: this.startTime_ =
null;
On 2011/09/22 19:26:04, arv wrote:
> This is better done on the prototype
> 
> startTime_: null,

Done.

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/media_internals.js (right):

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:43: var initialize =
function() {
On 2011/09/22 19:26:04, arv wrote:
> Please use function statement whenever valid (top level and in function scope)
> 
> function initialize() {
> 
> }

Done.

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:245: new
media.MediaPlayer({'id': source, 'renderer': event.renderer});
On 2011/09/22 19:26:04, arv wrote:
> {id: ..., event: ...}

Done.

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/media_player.js (right):

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_player.js:23: this.id = null;
On 2011/09/22 19:26:04, arv wrote:
> renderer and id can go on the prototype

Done.

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/metrics.js (right):

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:11: 'pipeline_state': true,
On 2011/09/22 19:26:04, arv wrote:
> Don't quote property names unless you have to

I believe this is a string identifier for an event name as defined in the C++
part of this code

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:17: 'seek': {'start':
'SEEK',
On 2011/09/22 19:26:04, arv wrote:
> The indentation of this is strange. Please line break after { and indent 2
> spaces

Done.

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:82: m.avg.textContent =
round(m.total / m.count);
On 2011/09/22 19:26:04, arv wrote:
> Math.round?

I wonder whether this was supposed to be media.round (from util.js), which does
rounding to # of digits

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/util.js (right):

http://codereview.chromium.org/7653001/diff/8001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/util.js:41: * Truncates a URL to
MAX_URL_LENGTH. If the URL was too long, the last
On 2011/09/22 19:26:04, arv wrote:
> Use CSS instead.
> 
> overflow: hidden;
> text-overflow: ellipsis;

Done (I hope!)