Adds cache and buffer graphs to the properties pane.

Adds cache and buffer graphs to the properties pane.

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

[Ty Overby, 2013-09-04 20:04:15]

Ok, this one should be ready for review.  

All this CL does is add the buffer and cache graphs to the properties pane.

[scherkus (not reviewing), 2013-09-04 23:20:53]

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
File content/browser/resources/media/new/client_renderer.js (right):

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:80: this.drawGraphs_();
I noticed you don't call anything on ClientRenderer when there are network
updates

is playerUpdated() called frequently enough to draw the graph?

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:163: },
nit: add blank line after this

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:165:
removeChildren(this.graphElement);
think we can get away with recreating the graphs DOM every time?

perhaps move this call to when we need to do it, then have drawGraphs_() lazily
construct the DOM whenever the parent element of a canvas is non-null?

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:174: if
(this.selectedPlayer && this.selectedPlayer.properties.url) {
it looks like this.selectedPlayer should always be non-null when drawGraphs_()
is called

can we remove the check?

also if properties.url isn't present it'd be preferable to early-return so we
don't have deeply-nested ifs

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:200: },
add blank line after

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:203: // and close over it
with a module.
any reason not to use a member variable canvas versus a closure?

right now the code is hard to piece together and the calling code looks funny
because drawBufferGraph_() not only draws, but returns the canvas to add as well

it'd be preferable to have drawBufferGraph_() stick to drawing, then make the
call to addToGraphs() separately similar to how cache is handled

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
File content/browser/resources/media/new/main.js (right):

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/main.js:12: var eventPhases_ = {};
can we get some short comments describing what each group of these are for and
the types of objects they contain?

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/main.js:61: case
'PHASE_BEGIN.DISK_CACHE_ENTRY_IMPL':
case statements should be indented by 2 more spaces along w/ the code

[Ty Overby, 2013-09-05 21:01:01]

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
File content/browser/resources/media/new/client_renderer.js (right):

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:80: this.drawGraphs_();
On 2013/09/04 23:20:53, scherkus wrote:
> I noticed you don't call anything on ClientRenderer when there are network
> updates
> 
> is playerUpdated() called frequently enough to draw the graph?

Yeah, buffer_start, buffer_end, buffer_current get spammed *a lot*.

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:163: },
On 2013/09/04 23:20:53, scherkus wrote:
> nit: add blank line after this

Done.

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:165:
removeChildren(this.graphElement);
On 2013/09/04 23:20:53, scherkus wrote:
> think we can get away with recreating the graphs DOM every time?
> 
> perhaps move this call to when we need to do it, then have drawGraphs_()
lazily
> construct the DOM whenever the parent element of a canvas is non-null?

I did it a tad differently, but to the same effect.

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:174: if
(this.selectedPlayer && this.selectedPlayer.properties.url) {
On 2013/09/04 23:20:53, scherkus wrote:
> it looks like this.selectedPlayer should always be non-null when drawGraphs_()
> is called
> 
> can we remove the check?
> 
> also if properties.url isn't present it'd be preferable to early-return so we
> don't have deeply-nested ifs

Yep, check can be removed.

Done.

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:200: },
On 2013/09/04 23:20:53, scherkus wrote:
> add blank line after

Done.

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/client_renderer.js:203: // and close over it
with a module.
On 2013/09/04 23:20:53, scherkus wrote:
> any reason not to use a member variable canvas versus a closure?
> 
> right now the code is hard to piece together and the calling code looks funny
> because drawBufferGraph_() not only draws, but returns the canvas to add as
well
> 
> it'd be preferable to have drawBufferGraph_() stick to drawing, then make the
> call to addToGraphs() separately similar to how cache is handled

Yeah, I agree, I thought it was going to be cleaner this way, but it ended up
quite confusing. 

Done.

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
File content/browser/resources/media/new/main.js (right):

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/main.js:12: var eventPhases_ = {};
On 2013/09/04 23:20:53, scherkus wrote:
> can we get some short comments describing what each group of these are for and
> the types of objects they contain?

Done.

https://codereview.chromium.org/23536020/diff/4001/content/browser/resources/...
content/browser/resources/media/new/main.js:61: case
'PHASE_BEGIN.DISK_CACHE_ENTRY_IMPL':
On 2013/09/04 23:20:53, scherkus wrote:
> case statements should be indented by 2 more spaces along w/ the code

Done.

[scherkus (not reviewing), 2013-09-06 18:16:47]

fewwwwwwww tiny nits

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
File content/browser/resources/media/new/client_renderer.js (right):

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:170:
removeChildren(this.graphElement);
won't this cause line 209 to always evaluate to true?

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:172: var addToGraphs =
(function(name, graph) {
considering you're already passing in this. member variables, how about dropping
bind() in favour of something more straightforward:

function addGraph(graphElement, name, graph) {
  // ...
  graphElement.appendChild(li);
}

addGraph(this.graphElement, 'buffered', this.bufferedCanvas);

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:190: bufferExists =
false;
might as well use separate var statement here

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:204: cache.size =
cache.size || +player.properties['total_bytes'];
can you rewrite this to avoid type trickery

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
File content/browser/resources/media/new/main.js (right):

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
content/browser/resources/media/new/main.js:22: var cacheEntries_ = {};
nit: add blink line after this line (you've got things spaced out already!)

[Ty Overby, 2013-09-06 19:56:37]

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
File content/browser/resources/media/new/client_renderer.js (right):

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:170:
removeChildren(this.graphElement);
On 2013/09/06 18:16:47, scherkus wrote:
> won't this cause line 209 to always evaluate to true?

Done.

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:172: var addToGraphs =
(function(name, graph) {
On 2013/09/06 18:16:47, scherkus wrote:
> considering you're already passing in this. member variables, how about
dropping
> bind() in favour of something more straightforward:
> 
> function addGraph(graphElement, name, graph) {
>   // ...
>   graphElement.appendChild(li);
> }
> 
> addGraph(this.graphElement, 'buffered', this.bufferedCanvas);

Done.

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:190: bufferExists =
false;
On 2013/09/06 18:16:47, scherkus wrote:
> might as well use separate var statement here

Done.

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:204: cache.size =
cache.size || +player.properties['total_bytes'];
On 2013/09/06 18:16:47, scherkus wrote:
> can you rewrite this to avoid type trickery

Done.

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
File content/browser/resources/media/new/main.js (right):

https://codereview.chromium.org/23536020/diff/10001/content/browser/resources...
content/browser/resources/media/new/main.js:22: var cacheEntries_ = {};
On 2013/09/06 18:16:47, scherkus wrote:
> nit: add blink line after this line (you've got things spaced out already!)

Done.

[scherkus (not reviewing), 2013-09-06 20:08:15]

lgtm w/ nits

https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
File content/browser/resources/media/new/client_renderer.js (right):

https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:170: var addToGraphs =
function(name, graph, graphElement) {
function addToGraphs(...) {
  ...
}

https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:174:
graphElement.appendChild(li);
de-indent this block of code by 2 spaces

https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:202: cache.size =
cache.size || Number(player.properties['total_bytes']);
what if player.properties['total_bytes'] is undefined?

[Ty Overby, 2013-09-06 20:57:33]

https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
File content/browser/resources/media/new/client_renderer.js (right):

https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:170: var addToGraphs =
function(name, graph, graphElement) {
On 2013/09/06 20:08:15, scherkus wrote:
> function addToGraphs(...) {
>   ...
> }

Done.

https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:174:
graphElement.appendChild(li);
On 2013/09/06 20:08:15, scherkus wrote:
> de-indent this block of code by 2 spaces

Done.

https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:202: cache.size =
cache.size || Number(player.properties['total_bytes']);
On 2013/09/06 20:08:15, scherkus wrote:
> what if player.properties['total_bytes'] is undefined?

That's handled by the cache.  Undefined is falsey so the cache will refuse to
draw it.

[scherkus (not reviewing), 2013-09-06 20:59:04]

https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
File content/browser/resources/media/new/client_renderer.js (right):

https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:202: cache.size =
cache.size || Number(player.properties['total_bytes']);
On 2013/09/06 20:57:33, Ty Overby wrote:
> On 2013/09/06 20:08:15, scherkus wrote:
> > what if player.properties['total_bytes'] is undefined?
> 
> That's handled by the cache.  Undefined is falsey so the cache will refuse to
> draw it.

right ... but if we have Number(undefined) it evaluates to NaN.

is that what we want?

[Ty Overby, 2013-09-06 21:05:59]

On 2013/09/06 20:59:04, scherkus wrote:
>
https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
> File content/browser/resources/media/new/client_renderer.js (right):
> 
>
https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
> content/browser/resources/media/new/client_renderer.js:202: cache.size =
> cache.size || Number(player.properties['total_bytes']);
> On 2013/09/06 20:57:33, Ty Overby wrote:
> > On 2013/09/06 20:08:15, scherkus wrote:
> > > what if player.properties['total_bytes'] is undefined?
> > 
> > That's handled by the cache.  Undefined is falsey so the cache will refuse
to
> > draw it.
> 
> right ... but if we have Number(undefined) it evaluates to NaN.
> 
> is that what we want?

Oh, that's what I meant, NaN is falsey.  It is kind of a hack... I just didn't
want to add another level of nested ifs.

[scherkus (not reviewing), 2013-09-06 21:09:50]

On 2013/09/06 21:05:59, Ty Overby wrote:
> On 2013/09/06 20:59:04, scherkus wrote:
> >
>
https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
> > File content/browser/resources/media/new/client_renderer.js (right):
> > 
> >
>
https://codereview.chromium.org/23536020/diff/18001/content/browser/resources...
> > content/browser/resources/media/new/client_renderer.js:202: cache.size =
> > cache.size || Number(player.properties['total_bytes']);
> > On 2013/09/06 20:57:33, Ty Overby wrote:
> > > On 2013/09/06 20:08:15, scherkus wrote:
> > > > what if player.properties['total_bytes'] is undefined?
> > > 
> > > That's handled by the cache.  Undefined is falsey so the cache will refuse
> to
> > > draw it.
> > 
> > right ... but if we have Number(undefined) it evaluates to NaN.
> > 
> > is that what we want?
> 
> Oh, that's what I meant, NaN is falsey.  It is kind of a hack... I just didn't
> want to add another level of nested ifs.

Please use ifs. No need to resort to hackery.

[Ty Overby (Google), 2013-09-06 21:14:24]

Done.


On Fri, Sep 6, 2013 at 2:09 PM, <scherkus@chromium.org> wrote:

> On 2013/09/06 21:05:59, Ty Overby wrote:
>
>> On 2013/09/06 20:59:04, scherkus wrote:
>> >
>>
>
> https://codereview.chromium.**org/23536020/diff/18001/**
>
content/browser/resources/**media/new/client_renderer.js<https://codereview.chromium.org/23536020/diff/18001/content/browser/resources/media/new/client_renderer.js>
>
>> > File content/browser/resources/**media/new/client_renderer.js (right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/23536020/diff/18001/**
>
content/browser/resources/**media/new/client_renderer.js#**newcode202<https://codereview.chromium.org/23536020/diff/18001/content/browser/resources/media/new/client_renderer.js#newcode202>
>
>> > content/browser/resources/**media/new/client_renderer.js:**202:
>> cache.size =
>> > cache.size || Number(player.properties['**total_bytes']);
>> > On 2013/09/06 20:57:33, Ty Overby wrote:
>> > > On 2013/09/06 20:08:15, scherkus wrote:
>> > > > what if player.properties['total_**bytes'] is undefined?
>> > >
>> > > That's handled by the cache.  Undefined is falsey so the cache will
>> refuse
>> to
>> > > draw it.
>> >
>> > right ... but if we have Number(undefined) it evaluates to NaN.
>> >
>> > is that what we want?
>>
>
>  Oh, that's what I meant, NaN is falsey.  It is kind of a hack... I just
>> didn't
>> want to add another level of nested ifs.
>>
>
> Please use ifs. No need to resort to hackery.
>
>
https://codereview.chromium.**org/23536020/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ty Overby, 2013-09-07 00:17:21]

scherkus@: you should probably review the files that you can.  I'd like to know
who you would have review the rest.

[Ty Overby, 2013-09-09 17:45:54]

OWNERS reviews: 
  creis@:    content/content_tests.gypi 
             content/test/webui_resource_browsertest.cc

  arv@:      ui/webui/resources/js/cr.js

  scherkus@: Remaining files

https://codereview.chromium.org/23536020/diff/27001/ui/webui/resources/js/cr.js
File ui/webui/resources/js/cr.js (right):

https://codereview.chromium.org/23536020/diff/27001/ui/webui/resources/js/cr....
ui/webui/resources/js/cr.js:354: cr.isGTK = typeof chrome !== 'undefined' &&
The reason for this check is that inside of a webui_resource_browsertest,
'chrome' is not defined.  To let cr.js work in this environment, a check here is
required.

[scherkus (not reviewing), 2013-09-09 17:48:28]

lgtm

https://codereview.chromium.org/23536020/diff/27001/ui/webui/resources/js/cr.js
File ui/webui/resources/js/cr.js (right):

https://codereview.chromium.org/23536020/diff/27001/ui/webui/resources/js/cr....
ui/webui/resources/js/cr.js:354: cr.isGTK = typeof chrome !== 'undefined' &&
On 2013/09/09 17:45:55, Ty Overby wrote:
> The reason for this check is that inside of a webui_resource_browsertest,
> 'chrome' is not defined.  To let cr.js work in this environment, a check here
is
> required. 

To elaborate, content-based browsers (e.g., content_shell) can use
ui/webui/resources/js/* but won't necessarily define "chrome" APIs

[arv (Not doing code reviews), 2013-09-09 18:02:18]

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
File content/browser/resources/media/cache_entry.js (right):

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/cache_entry.js:181:
toReturn.appendChild(document.createTextNode(content));
toReturn.textContent = content;

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
File content/browser/resources/media/new/client_renderer.js (right):

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:175: };
no semicolon after FunctionDeclaration

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
File content/browser/resources/media/new/main.js (right):

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:15: var eventPhases_ = {};
no underscores for variable names

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:190: // |chrome| is not defiend
during tests.
Please define chrome in your test instead.

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:191: if(window.chrome !== undefined)
{
ws after if

https://codereview.chromium.org/23536020/diff/27001/ui/webui/resources/js/cr.js
File ui/webui/resources/js/cr.js (right):

https://codereview.chromium.org/23536020/diff/27001/ui/webui/resources/js/cr....
ui/webui/resources/js/cr.js:354: cr.isGTK = typeof chrome !== 'undefined' &&
On 2013/09/09 17:45:55, Ty Overby wrote:
> The reason for this check is that inside of a webui_resource_browsertest,
> 'chrome' is not defined.  To let cr.js work in this environment, a check here
is
> required. 

Please add chrome to your test instead.

[Charlie Reis, 2013-09-09 18:02:22]

One question on content/content_tests.gypi and
content/test/webui_resource_browsertest.cc, with a few drive-by nits in the
other files.

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
File content/browser/resources/media/new/main.js (right):

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:190: // |chrome| is not defiend
during tests.
nit: defined

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:191: if(window.chrome !== undefined)
{
nit: Space after if

https://codereview.chromium.org/23536020/diff/27001/content/test/webui_resour...
File content/test/webui_resource_browsertest.cc (right):

https://codereview.chromium.org/23536020/diff/27001/content/test/webui_resour...
content/test/webui_resource_browsertest.cc:36: AddLibrary(IDR_WEBUI_JS_CR);
Just curious, but where is this used?  I don't see any other test modifications
in this CL.

https://codereview.chromium.org/23536020/diff/27001/ui/webui/resources/js/cr.js
File ui/webui/resources/js/cr.js (right):

https://codereview.chromium.org/23536020/diff/27001/ui/webui/resources/js/cr....
ui/webui/resources/js/cr.js:354: cr.isGTK = typeof chrome !== 'undefined' &&
On 2013/09/09 17:48:28, scherkus wrote:
> On 2013/09/09 17:45:55, Ty Overby wrote:
> > The reason for this check is that inside of a webui_resource_browsertest,
> > 'chrome' is not defined.  To let cr.js work in this environment, a check
here
> is
> > required. 
> 
> To elaborate, content-based browsers (e.g., content_shell) can use
> ui/webui/resources/js/* but won't necessarily define "chrome" APIs

Can these thoughts be added as a comment in the code?

[Ty Overby, 2013-09-09 18:28:12]

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
File content/browser/resources/media/cache_entry.js (right):

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/cache_entry.js:181:
toReturn.appendChild(document.createTextNode(content));
On 2013/09/09 18:02:19, arv wrote:
> toReturn.textContent = content;

Done.

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
File content/browser/resources/media/new/client_renderer.js (right):

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/client_renderer.js:175: };
On 2013/09/09 18:02:19, arv wrote:
> no semicolon after FunctionDeclaration

Done.

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
File content/browser/resources/media/new/main.js (right):

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:190: // |chrome| is not defiend
during tests.
On 2013/09/09 18:02:22, creis wrote:
> nit: defined

Done.

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:190: // |chrome| is not defiend
during tests.
On 2013/09/09 18:02:19, arv wrote:
> Please define chrome in your test instead.

This file is loaded after cr.js but before any test code can be run.  
See:
https://codereview.chromium.org/23536020/diff/27001/content/test/webui_resour...

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:191: if(window.chrome !== undefined)
{
On 2013/09/09 18:02:19, arv wrote:
> ws after if

Done.

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:191: if(window.chrome !== undefined)
{
On 2013/09/09 18:02:22, creis wrote:
> nit: Space after if

Done.

https://codereview.chromium.org/23536020/diff/27001/content/test/webui_resour...
File content/test/webui_resource_browsertest.cc (right):

https://codereview.chromium.org/23536020/diff/27001/content/test/webui_resour...
content/test/webui_resource_browsertest.cc:36: AddLibrary(IDR_WEBUI_JS_CR);
On 2013/09/09 18:02:22, creis wrote:
> Just curious, but where is this used?  I don't see any other test
modifications
> in this CL.

content/browser/resources/media/cache_entry.js

https://codereview.chromium.org/23536020/diff/27001/ui/webui/resources/js/cr.js
File ui/webui/resources/js/cr.js (right):

https://codereview.chromium.org/23536020/diff/27001/ui/webui/resources/js/cr....
ui/webui/resources/js/cr.js:354: cr.isGTK = typeof chrome !== 'undefined' &&
On 2013/09/09 18:02:19, arv wrote:
> On 2013/09/09 17:45:55, Ty Overby wrote:
> > The reason for this check is that inside of a webui_resource_browsertest,
> > 'chrome' is not defined.  To let cr.js work in this environment, a check
here
> is
> > required. 
> 
> Please add chrome to your test instead.

See my comment in reply to your comment in main.js

[Ty Overby, 2013-09-09 18:28:35]

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
File content/browser/resources/media/new/main.js (right):

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:15: var eventPhases_ = {};
On 2013/09/09 18:02:19, arv wrote:
> no underscores for variable names

Done.

[arv (Not doing code reviews), 2013-09-09 18:59:57]

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
File content/browser/resources/media/new/main.js (right):

https://codereview.chromium.org/23536020/diff/27001/content/browser/resources...
content/browser/resources/media/new/main.js:190: // |chrome| is not defiend
during tests.
On 2013/09/09 18:28:13, Ty Overby wrote:
> On 2013/09/09 18:02:19, arv wrote:
> > Please define chrome in your test instead.
> 
> This file is loaded after cr.js but before any test code can be run.  
> See:
>
https://codereview.chromium.org/23536020/diff/27001/content/test/webui_resour...

We are controlling that file too. It can add "var chrome = {};" as needed.

We should prefer modifying test files instead of production files where
possible.

[Ty Overby, 2013-09-09 20:57:55]

Turns out it's easier than I expected.  Thanks for the advice arv@, I've moved
the fix into my test files.

[Charlie Reis, 2013-09-09 21:03:06]

content/content_tests.gypi and
content/test/webui_resource_browsertest.cc LGTM.

[arv (Not doing code reviews), 2013-09-09 21:54:45]

LGTM

[commit-bot: I haz the power, 2013-09-09 21:59:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoverby@chromium.org/23536020/36001

[commit-bot: I haz the power, 2013-09-10 08:47:29]

Change committed as 222221