Add charts to the resource data section of chrome://media-internals.

Add charts to the resource data section of chrome://media-internals.

BUG=17969
TEST=manually


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

[Scott Franklin, 2011-07-21 21:50:47]

This draws charts representing which sections of each file have been read from /
written to the media cache. It uses the HTML5 <canvas> tag for that purpose and
the as-yet-chrome-only <details> tag to hide the actual numbers until clicked.

It seems to me like this is far too much code for what it accomplishes, but
perhaps this is normal for drawing large chunks of a page in javascript? I can't
really think of any ways to make it smaller.

[scherkus (not reviewing), 2011-07-22 19:26:46]

my not-so-experienced javascript eyes don't see anything too crazy with this
patch

a tentative LGTM but estade@ should give it a look-over as well

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:45: // A detailed writen
description of the above.
s/writen/written

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:129: setSize: function()
{
are we better off prompting or simply documenting that it's a limitation of the
caching system

if it's rarely used... will anyone really know the size of the file?

[Evan Stade, 2011-07-22 19:48:12]

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:8: * The width and
height of a CacheEntry canvas.
in pixels (I presume)

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:137: * Redraw
this.canvas_.
maybe a little context about the desired outcome?

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:196:
this.detail_text_.innerHTML = '<strong>Read from cache:</strong><br />';
I think you should use css instead of strong.

This looks like tabular data, so it's ok to use tables for it. And instead of
writing html in strings, I think the js approach would be clearer here.

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:209:
this.detail_text_.innerHTML += this.size;
I always prefer the ternary operator

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
File chrome/browser/resources/media_internals/disjoint_range_set.js (right):

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/disjoint_range_set.js:129: 
docs

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/disjoint_range_set.js:131: var max =
null;
0?

[Scott Franklin, 2011-07-22 22:27:20]

I'm liking the table. Things look much cleaner laid out this way.

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
File chrome/browser/resources/media_internals/cache_entry.js (right):

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:8: * The width and
height of a CacheEntry canvas.
On 2011/07/22 19:48:12, Evan Stade wrote:
> in pixels (I presume)

Oh right.

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:45: // A detailed writen
description of the above.
On 2011/07/22 19:26:46, scherkus wrote:
> s/writen/written

Done.

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:129: setSize: function()
{
On 2011/07/22 19:26:46, scherkus wrote:
> are we better off prompting or simply documenting that it's a limitation of
the
> caching system
> 
> if it's rarely used... will anyone really know the size of the file?

I used it quite a bit when debugging canvas issues. But to be honest no one else
is likely to do so.
Removed.

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:137: * Redraw
this.canvas_.
On 2011/07/22 19:48:12, Evan Stade wrote:
> maybe a little context about the desired outcome?

Done.

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:196:
this.detail_text_.innerHTML = '<strong>Read from cache:</strong><br />';
On 2011/07/22 19:48:12, Evan Stade wrote:
> I think you should use css instead of strong.
> 
> This looks like tabular data, so it's ok to use tables for it. And instead of
> writing html in strings, I think the js approach would be clearer here.

The tables look good; I like it.

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/cache_entry.js:209:
this.detail_text_.innerHTML += this.size;
On 2011/07/22 19:48:12, Evan Stade wrote:
> I always prefer the ternary operator

And plain old || is even better.

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
File chrome/browser/resources/media_internals/disjoint_range_set.js (right):

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/disjoint_range_set.js:129: 
On 2011/07/22 19:48:12, Evan Stade wrote:
> docs

Done.

http://codereview.chromium.org/7479005/diff/14/chrome/browser/resources/media...
chrome/browser/resources/media_internals/disjoint_range_set.js:131: var max =
null;
On 2011/07/22 19:48:12, Evan Stade wrote:
> 0?

-Infinity, in fact. No reason this shouldn't be able to handle negative numbers.
For some reason I was thinking null < any number; I guess I just assumed
javascript would be weird.

[Scott Franklin, 2011-07-26 00:49:37]

estade@, ping?

[Evan Stade, 2011-07-26 01:16:25]

lgtm

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

http://codereview.chromium.org/7479005/diff/10001/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:62: this.detail_table_ =
document.createElement('table');
camelCase_

http://codereview.chromium.org/7479005/diff/10001/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:178: element.textContent
= content || '';
I don't think the || part is necessary here?

[Scott Franklin, 2011-07-26 20:26:34]

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

http://codereview.chromium.org/7479005/diff/10001/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:62: this.detail_table_ =
document.createElement('table');
On 2011/07/26 01:16:26, Evan Stade wrote:
> camelCase_

Oops, reverted to C++. Done.

http://codereview.chromium.org/7479005/diff/10001/chrome/browser/resources/me...
chrome/browser/resources/media_internals/cache_entry.js:178: element.textContent
= content || '';
On 2011/07/26 01:16:26, Evan Stade wrote:
> I don't think the || part is necessary here?

It's not. It was there in case the second argument wasn't provided (which sets
the textContent to 'null'), but I used createElement for those that needed no
default text (and js probably has a default argument syntax I should've used
instead).
Removed.

[commit-bot: I haz the power, 2011-07-27 00:10:29]

Change committed as 94211