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

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

Patch by scottfr@chromium.org.

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

[scherkus (not reviewing), 2011-09-23 06:51:56]

Moved from http://codereview.chromium.org/7653001/ since my intern (scottfr)
couldn't land this patch before leaving.

The first patch set is the copy arv@ had comments on. The second patch set are
my edits fixing comments.

Thanks for reviewing and apologies for any confusion!

[arv (Not doing code reviews), 2011-09-23 15:59:01]

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/cache_entry.js:29: summary.className =
'ellipsis';
class name should not describe the rendering ut the intention. 

I'm not even sure you need a className here thou. It seems like

.chache-entry > summary {

is sufficient to select this in CSS

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/media_internals.css (right):

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.css:18: #media-players
td, #media-players th {
#media-players td,
#media-players th {

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

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:49: };
no semicolon after function declarations

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:144: * @param {int}
renderer The id of the render process.
@param {number}

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:150: delete
mediaPlayers[key];
Maybe break out of loop here?

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:241: event.time *=
1000;
It is kind of ugly to change the event object here.

I don't see where time is used. Maybe it is cleaner to only do the
multiplication there in case someone else references this object.

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:278: window.onload =
function() {
Do you really have to wait for window.onload?

Can you at least change this to listen to DOMContentLoaded?

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

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:58: this[metric] = {'count':
0, 'total': 0, 'start': null,
this[metric] = {
  count: 0,
  total: 0,
  start: null,
  last: last,
  avg: avg
};

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

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/util.js:74: * @param {int} digits The
number of digits to leave after the decimal.
{number}

[scherkus (not reviewing), 2011-09-23 17:43:23]

thanks for the review!!

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/cache_entry.js:29: summary.className =
'ellipsis';
On 2011/09/23 15:59:01, arv wrote:
> class name should not describe the rendering ut the intention. 
> 
> I'm not even sure you need a className here thou. It seems like
> 
> .chache-entry > summary {
> 
> is sufficient to select this in CSS

I ditched the entire ellipsis thing instead showing the full URL of the media
resource (allows for copy+pasting for debugging)

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
File chrome/browser/resources/media_internals/media_internals.css (right):

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.css:18: #media-players
td, #media-players th {
On 2011/09/23 15:59:01, arv wrote:
> #media-players td,
> #media-players th {

Done.

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

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:49: };
On 2011/09/23 15:59:01, arv wrote:
> no semicolon after function declarations

Done.

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:144: * @param {int}
renderer The id of the render process.
On 2011/09/23 15:59:01, arv wrote:
> @param {number}

Done.

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:150: delete
mediaPlayers[key];
On 2011/09/23 15:59:01, arv wrote:
> Maybe break out of loop here?

Good catch - done.

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:241: event.time *=
1000;
On 2011/09/23 15:59:01, arv wrote:
> It is kind of ugly to change the event object here.
> 
> I don't see where time is used. Maybe it is cleaner to only do the
> multiplication there in case someone else references this object.

Hmm... it looks like the event trickles through to these places:

media_player.js: addEvent() (time not used)
metrics.js: addEvent()
event_list.js: addEvent() (actually modifies time!)

I've made the conversion to milliseconds local to metrics/event_list.js

Nice catch!

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:278: window.onload =
function() {
On 2011/09/23 15:59:01, arv wrote:
> Do you really have to wait for window.onload?
> 
> Can you at least change this to listen to DOMContentLoaded?

Done.

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

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:58: this[metric] = {'count':
0, 'total': 0, 'start': null,
On 2011/09/23 15:59:01, arv wrote:
> this[metric] = {
>   count: 0,
>   total: 0,
>   start: null,
>   last: last,
>   avg: avg
> };

Done.

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

http://codereview.chromium.org/7972028/diff/2001/chrome/browser/resources/med...
chrome/browser/resources/media_internals/util.js:74: * @param {int} digits The
number of digits to leave after the decimal.
On 2011/09/23 15:59:01, arv wrote:
> {number}

Done.

[scherkus (not reviewing), 2011-10-04 17:39:31]

ping on arv

[arv (Not doing code reviews), 2011-10-18 20:50:07]

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/event_list.js:44: var time_in_ms =
event.time * 1000;  // Work with milliseconds.
No underscores in js => timeInMs

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/event_list.js:46: time_in_ms =
time_in_ms - this.startTime_;
-=

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/event_list.js:52: for (var p in
event.params) {
Don't use for in loops for arrays

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:276:
window.addEventListener("DOMContentLoaded", function() {
Use single quotes

Use document since that is the object dispatching DOMContentLoaded.

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_player.js:92:
context.fillText('Unknown file size.', canvas.width / 2,
To me it always suspicious when fillText is used. Can this be done in some other
way so it can be selected, copied, read etc?

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:81: var time_in_ms =
event.time * 1000;  // Work with milliseconds.
no underscores

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/util.js:55: function
printDictionaryToTable(dict, table) {
Please rename. print is not the right word here

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/util.js:73: function round(n, digits) {
Can you use toFixed instead?

[scherkus (not reviewing), 2011-10-19 00:19:43]

thanks for the review!  apologies for doing some meganoob JS stuff D:

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/event_list.js:44: var time_in_ms =
event.time * 1000;  // Work with milliseconds.
On 2011/10/18 20:50:07, arv wrote:
> No underscores in js => timeInMs

Done.

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/event_list.js:46: time_in_ms =
time_in_ms - this.startTime_;
On 2011/10/18 20:50:07, arv wrote:
> -=

Done.

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/event_list.js:52: for (var p in
event.params) {
On 2011/10/18 20:50:07, arv wrote:
> Don't use for in loops for arrays

Done.

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_internals.js:276:
window.addEventListener("DOMContentLoaded", function() {
On 2011/10/18 20:50:07, arv wrote:
> Use single quotes
> 
> Use document since that is the object dispatching DOMContentLoaded.

Done.

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_player.js:92:
context.fillText('Unknown file size.', canvas.width / 2,
On 2011/10/18 20:50:07, arv wrote:
> To me it always suspicious when fillText is used. Can this be done in some
other
> way so it can be selected, copied, read etc?

such as a <div> of text w/ z-index:1 over top of the <canvas>?

on further inspection it looks like fillText() is only used in error conditions
in which case we might not even require a <canvas> in those cases and could
append a <div> of text instead.

was that what you were thinking of?

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/metrics.js:81: var time_in_ms =
event.time * 1000;  // Work with milliseconds.
On 2011/10/18 20:50:07, arv wrote:
> no underscores

Done.

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/util.js:55: function
printDictionaryToTable(dict, table) {
On 2011/10/18 20:50:07, arv wrote:
> Please rename. print is not the right word here

s/print/append

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/util.js:73: function round(n, digits) {
On 2011/10/18 20:50:07, arv wrote:
> Can you use toFixed instead?

Done.

[arv (Not doing code reviews), 2011-10-19 20:30:21]

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

http://codereview.chromium.org/7972028/diff/2003/chrome/browser/resources/med...
chrome/browser/resources/media_internals/media_player.js:92:
context.fillText('Unknown file size.', canvas.width / 2,
On 2011/10/19 00:19:43, scherkus wrote:
> On 2011/10/18 20:50:07, arv wrote:
> > To me it always suspicious when fillText is used. Can this be done in some
> other
> > way so it can be selected, copied, read etc?
> 
> such as a <div> of text w/ z-index:1 over top of the <canvas>?
> 
> on further inspection it looks like fillText() is only used in error
conditions
> in which case we might not even require a <canvas> in those cases and could
> append a <div> of text instead.
> 
> was that what you were thinking of?

Yeah, a <p> or something like that would make more sense if the text does not
need be used as part of an image.

[scherkus (not reviewing), 2011-10-19 23:58:27]

PTAL especially for diffs against PS#4

cache_entry.js:generateCanvas()
 - refactored to accept a canvas object (easier to reuse)
 - split into drawCacheWritesToCanvas and drawCacheReadsToCanvas

media_player.js:redraw()
 - fillText has been replaced w/ table layout and text labels
 - now uses drawCacheWrites/ReadsToCanvas instead of copying pixels

+ some misc style stuff

[arv (Not doing code reviews), 2011-10-20 20:29:36]

http://codereview.chromium.org/7972028/diff/31002/chrome/browser/resources/me...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7972028/diff/31002/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:140: var drawRange =
function(start, end) {
function drawRange(start, end) {
   ...
}

http://codereview.chromium.org/7972028/diff/31002/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:160:
drawCacheReadsToCanvas: function(canvas) {
This looks like copy pasted code. Can you refactore to reduce duplication?

[scherkus (not reviewing), 2011-10-24 18:12:28]

http://codereview.chromium.org/7972028/diff/31002/chrome/browser/resources/me...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7972028/diff/31002/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:140: var drawRange =
function(start, end) {
On 2011/10/20 20:29:37, arv wrote:
> function drawRange(start, end) {
>    ...
> }

Done.

http://codereview.chromium.org/7972028/diff/31002/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:160:
drawCacheReadsToCanvas: function(canvas) {
On 2011/10/20 20:29:37, arv wrote:
> This looks like copy pasted code. Can you refactore to reduce duplication?

Busted :(

Refactored but keep in mind I'm not sure what the Javascript-approved way of
doing helper methods.

[arv (Not doing code reviews), 2011-10-24 22:26:33]

lgtm

http://codereview.chromium.org/7972028/diff/38001/chrome/browser/resources/me...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7972028/diff/38001/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:121: * Accepts the
entires to draw, a canvas fill style, and the canvas to
typo

http://codereview.chromium.org/7972028/diff/38001/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:142: };
no semi colon on function declarations

[scherkus (not reviewing), 2011-10-25 17:30:47]

thanks!

http://codereview.chromium.org/7972028/diff/38001/chrome/browser/resources/me...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7972028/diff/38001/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:121: * Accepts the
entires to draw, a canvas fill style, and the canvas to
On 2011/10/24 22:26:33, arv wrote:
> typo

Done.

http://codereview.chromium.org/7972028/diff/38001/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:142: };
On 2011/10/24 22:26:33, arv wrote:
> no semi colon on function declarations

Done.