[VideoPlayer] Browser tests for new separated video player app

[VideoPlayer] Browser tests for new separated video player app

BUG=357621
TEST=manually tested

R=hirono@chromium.org
TBR=arv@chromium.org
# TBRing for adding a resource.

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

[yoshiki, 2014-03-25 07:37:47]

@hirono, PTAL. This patch is depending on
https://codereview.chromium.org/210623003/.

[hirono, 2014-03-25 07:48:16]

https://codereview.chromium.org/209853011/diff/20001/chrome/browser/resources...
File chrome/browser/resources/video_player/js/test_util.js (right):

https://codereview.chromium.org/209853011/diff/20001/chrome/browser/resources...
chrome/browser/resources/video_player/js/test_util.js:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
The Files.app and the video player now share a few script files: util.js,
media/media_controls.js, media/util.js.
Why don't you make the video player share test_util.js?

[yoshiki, 2014-03-25 07:53:33]

https://codereview.chromium.org/209853011/diff/20001/chrome/browser/resources...
File chrome/browser/resources/video_player/js/test_util.js (right):

https://codereview.chromium.org/209853011/diff/20001/chrome/browser/resources...
chrome/browser/resources/video_player/js/test_util.js:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/03/25 07:48:16, hirono wrote:
> The Files.app and the video player now share a few script files: util.js,
> media/media_controls.js, media/util.js.
> Why don't you make the video player share test_util.js?

test.util.registerRemoteTestUtils is a bit different with file_manager's.

file_manager's test_util.js depends on file_manager's background.js.
video_player's test_util.js depends on video_player's background.js.

They are not compatible. I think there are no advantage to share them.

[hirono, 2014-03-25 08:02:19]

https://codereview.chromium.org/209853011/diff/20001/chrome/browser/resources...
File chrome/browser/resources/video_player/js/test_util.js (right):

https://codereview.chromium.org/209853011/diff/20001/chrome/browser/resources...
chrome/browser/resources/video_player/js/test_util.js:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/03/25 07:53:34, yoshiki wrote:
> On 2014/03/25 07:48:16, hirono wrote:
> > The Files.app and the video player now share a few script files: util.js,
> > media/media_controls.js, media/util.js.
> > Why don't you make the video player share test_util.js?
> 
> test.util.registerRemoteTestUtils is a bit different with file_manager's.
> 
> file_manager's test_util.js depends on file_manager's background.js.
> video_player's test_util.js depends on video_player's background.js.
> 
> They are not compatible. I think there are no advantage to share them.

SGTM!

https://codereview.chromium.org/209853011/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/open_video_files.js
(right):

https://codereview.chromium.org/209853011/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/open_video_files.js:38:
return Promise.resolve(false);
Maybe logging the error helps us to find out why the step failed.

https://codereview.chromium.org/209853011/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/open_video_files.js:85:
checkIfNoErrorsOccured(this.next);
Could you also add a error check for the video player?

[yoshiki, 2014-03-25 10:07:27]

@hirono, PTAL. Thanks.

https://codereview.chromium.org/209853011/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/open_video_files.js
(right):

https://codereview.chromium.org/209853011/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/open_video_files.js:38:
return Promise.resolve(false);
On 2014/03/25 08:02:19, hirono wrote:
> Maybe logging the error helps us to find out why the step failed.

Done.

https://codereview.chromium.org/209853011/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/open_video_files.js:85:
checkIfNoErrorsOccured(this.next);
On 2014/03/25 08:02:19, hirono wrote:
> Could you also add a error check for the video player?

Done.

[hirono, 2014-03-26 02:55:04]

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
File chrome/browser/resources/video_player/js/error_util.js (right):

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:19: * Wrap the function
to use it as a callback.
nit: Wraps

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:27:
Function.prototype.wrap = function(thisObject) {
Strictly our design guide forbid us to modify prototypes of builtin objects.
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:37:
console.error('Exception is happen in callback.', liveStack);
The e.stack is also useful information. Could you add it?

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:37:
console.error('Exception is happen in callback.', liveStack);
nit: Exception happens or Exception is thrown?

[yoshiki, 2014-03-26 04:55:51]

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
File chrome/browser/resources/video_player/js/error_util.js (right):

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/03/26 02:55:05, hirono wrote:
> nit: 2014

Done.

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:19: * Wrap the function
to use it as a callback.
On 2014/03/26 02:55:05, hirono wrote:
> nit: Wraps

Done.

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:27:
Function.prototype.wrap = function(thisObject) {
On 2014/03/26 02:55:05, hirono wrote:
> Strictly our design guide forbid us to modify prototypes of builtin objects.
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...

Not "strictly". The guide says "Modifying other builtins like Function.prototype
is less dangerous".

Of cause, we can instead use a normal function like wrap(func, thisObj). But I'm
considering to replace bind() with wrap() in Files.app as well as in video
player. Along the way, the same syntax as bind() is better to replace it, since
the only thing we need to do it replacing of the text.

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:37:
console.error('Exception is happen in callback.', liveStack);
On 2014/03/26 02:55:05, hirono wrote:
> nit: Exception happens or Exception is thrown?

Done.

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:37:
console.error('Exception is happen in callback.', liveStack);
On 2014/03/26 02:55:05, hirono wrote:
> The e.stack is also useful information. Could you add it?

The original exception e will be caught by the handler of the outer function.

[hirono, 2014-03-26 08:18:31]

The newest change is not uploaded?

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
File chrome/browser/resources/video_player/js/error_util.js (right):

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:27:
Function.prototype.wrap = function(thisObject) {
On 2014/03/26 04:55:52, yoshiki wrote:
> On 2014/03/26 02:55:05, hirono wrote:
> > Strictly our design guide forbid us to modify prototypes of builtin objects.
> >
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...
> 
> Not "strictly". The guide says "Modifying other builtins like
Function.prototype
> is less dangerous".
> 
> Of cause, we can instead use a normal function like wrap(func, thisObj). But
I'm
> considering to replace bind() with wrap() in Files.app as well as in video
> player. Along the way, the same syntax as bind() is better to replace it,
since
> the only thing we need to do it replacing of the text.

SGTM.

[yoshiki, 2014-03-26 08:45:46]

Sorry, I forgot to upload it. PATL again?

[hirono, 2014-03-26 08:57:58]

lgtm!

[yoshiki, 2014-03-26 13:29:49]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-03-26 13:29:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/209853011/220001

[yoshiki, 2014-03-26 13:29:54]

The CQ bit was unchecked by yoshiki@chromium.org

[yoshiki, 2014-03-26 13:30:53]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-03-26 13:31:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/209853011/220001

[commit-bot: I haz the power, 2014-03-26 16:28:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 16:28:38]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[yoshiki, 2014-03-27 00:24:03]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-03-27 00:30:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/209853011/220001

[commit-bot: I haz the power, 2014-03-27 02:09:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 02:09:55]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[yoshiki, 2014-03-28 18:13:08]

Committed patchset #6 manually as r260198 (presubmit successful).

[mtomasz, 2014-04-01 19:37:49]

Sorry for late comments. I just came across this CL.

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
File chrome/browser/resources/video_player/js/error_util.js (right):

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:27:
Function.prototype.wrap = function(thisObject) {
On 2014/03/26 08:18:31, hirono wrote:
> On 2014/03/26 04:55:52, yoshiki wrote:
> > On 2014/03/26 02:55:05, hirono wrote:
> > > Strictly our design guide forbid us to modify prototypes of builtin
objects.
> > >
> >
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...
> > 
> > Not "strictly". The guide says "Modifying other builtins like
> Function.prototype
> > is less dangerous".
> > 
> > Of cause, we can instead use a normal function like wrap(func, thisObj). But
> I'm
> > considering to replace bind() with wrap() in Files.app as well as in video
> > player. Along the way, the same syntax as bind() is better to replace it,
> since
> > the only thing we need to do it replacing of the text.
> 
> SGTM. 

Out of curiosity. Why don't we put this code to #16? Is there any advantage of
wrap() over window.onerror?

I'm a little bit concerned about violating the style guide here, which clearly
says No for modifying prototypes of builtin types. Can't we avoid it?

https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
chrome/browser/resources/video_player/js/error_util.js:39:
window.JSErrorCount++;
Aren't we incrementing JSErrorCount twice? #16 and #39?

[yoshiki, 2014-04-02 04:20:10]

On 2014/04/01 19:37:49, mtomasz wrote:
> Sorry for late comments. I just came across this CL.
> 
>
https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
> File chrome/browser/resources/video_player/js/error_util.js (right):
> 
>
https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
> chrome/browser/resources/video_player/js/error_util.js:27:
> Function.prototype.wrap = function(thisObject) {
> On 2014/03/26 08:18:31, hirono wrote:
> > On 2014/03/26 04:55:52, yoshiki wrote:
> > > On 2014/03/26 02:55:05, hirono wrote:
> > > > Strictly our design guide forbid us to modify prototypes of builtin
> objects.
> > > >
> > >
> >
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...
> > > 
> > > Not "strictly". The guide says "Modifying other builtins like
> > Function.prototype
> > > is less dangerous".
> > > 
> > > Of cause, we can instead use a normal function like wrap(func, thisObj).
But
> > I'm
> > > considering to replace bind() with wrap() in Files.app as well as in video
> > > player. Along the way, the same syntax as bind() is better to replace it,
> > since
> > > the only thing we need to do it replacing of the text.
> > 
> > SGTM. 
> 
> Out of curiosity. Why don't we put this code to #16? Is there any advantage of
> wrap() over window.onerror?

There are 2 advantages.

(1) 
window.onerror is DOM error handler so it doesn't catch an exception in the
handler of non-DOM async function.

// Example1, error in handler of DOM async function
var count = 0;
window.onerror = function() { count++; }
setTimeout(function() { throw 'error'; }); // DOM function
setTimeout(function() { assert(count == 1); }, 100);


// Example2, error in handler of non-DOM async function
var count = 0;
window.onerror = function() { count++; }
chrome.local.storage.get(function() { throw 'error'; }); // non-DOM function
setTimeout(function() { assert(count == 0); }, 100);  // The exception is not
caught!

(2) we can know the stack trace of caller in case of error. 


> I'm a little bit concerned about violating the style guide here, which clearly
> says No for modifying prototypes of builtin types. Can't we avoid it?
> 
>
https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
> chrome/browser/resources/video_player/js/error_util.js:39:
> window.JSErrorCount++;
> Aren't we incrementing JSErrorCount twice? #16 and #39?

I think double-counting is not a problem for this count, since what we want to
know is whether any error is happened or not. But I agreed that it looks
confusing. I think it's better to add a comment.

[mtomasz, 2014-04-02 04:42:25]

On 2014/04/02 04:20:10, yoshiki wrote:
> On 2014/04/01 19:37:49, mtomasz wrote:
> > Sorry for late comments. I just came across this CL.
> > 
> >
>
https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
> > File chrome/browser/resources/video_player/js/error_util.js (right):
> > 
> >
>
https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
> > chrome/browser/resources/video_player/js/error_util.js:27:
> > Function.prototype.wrap = function(thisObject) {
> > On 2014/03/26 08:18:31, hirono wrote:
> > > On 2014/03/26 04:55:52, yoshiki wrote:
> > > > On 2014/03/26 02:55:05, hirono wrote:
> > > > > Strictly our design guide forbid us to modify prototypes of builtin
> > objects.
> > > > >
> > > >
> > >
> >
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...
> > > > 
> > > > Not "strictly". The guide says "Modifying other builtins like
> > > Function.prototype
> > > > is less dangerous".
> > > > 
> > > > Of cause, we can instead use a normal function like wrap(func, thisObj).
> But
> > > I'm
> > > > considering to replace bind() with wrap() in Files.app as well as in
video
> > > > player. Along the way, the same syntax as bind() is better to replace
it,
> > > since
> > > > the only thing we need to do it replacing of the text.
> > > 
> > > SGTM. 
> > 
> > Out of curiosity. Why don't we put this code to #16? Is there any advantage
of
> > wrap() over window.onerror?
> 
> There are 2 advantages.
> 
> (1) 
> window.onerror is DOM error handler so it doesn't catch an exception in the
> handler of non-DOM async function.
> 
> // Example1, error in handler of DOM async function
> var count = 0;
> window.onerror = function() { count++; }
> setTimeout(function() { throw 'error'; }); // DOM function
> setTimeout(function() { assert(count == 1); }, 100);
> 
> 
> // Example2, error in handler of non-DOM async function
> var count = 0;
> window.onerror = function() { count++; }
> chrome.local.storage.get(function() { throw 'error'; }); // non-DOM function
> setTimeout(function() { assert(count == 0); }, 100);  // The exception is not
> caught!

Thanks for the explanation. This is very interesting, I didn't know about it.
Looks like a bug in Chrome.

> 
> (2) we can know the stack trace of caller in case of error. 

We can access the stacktrace via window.onerror, using the 5th argument. It
seems to be a very recently added feature.
http://html5.org/tools/web-apps-tracker?from=8085&to=8086

> 
> 
> > I'm a little bit concerned about violating the style guide here, which
clearly
> > says No for modifying prototypes of builtin types. Can't we avoid it?
> > 
> >
>
https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
> > chrome/browser/resources/video_player/js/error_util.js:39:
> > window.JSErrorCount++;
> > Aren't we incrementing JSErrorCount twice? #16 and #39?
> 
> I think double-counting is not a problem for this count, since what we want to
> know is whether any error is happened or not. But I agreed that it looks
> confusing. I think it's better to add a comment.

If so, then how about changing to bool, to avoid confusion?

[yoshiki, 2014-04-02 06:07:31]

On 2014/04/02 04:42:25, mtomasz wrote:
> On 2014/04/02 04:20:10, yoshiki wrote:
> > On 2014/04/01 19:37:49, mtomasz wrote:
> > > Sorry for late comments. I just came across this CL.
> > > 
> > >
> >
>
https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
> > > File chrome/browser/resources/video_player/js/error_util.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
> > > chrome/browser/resources/video_player/js/error_util.js:27:
> > > Function.prototype.wrap = function(thisObject) {
> > > On 2014/03/26 08:18:31, hirono wrote:
> > > > On 2014/03/26 04:55:52, yoshiki wrote:
> > > > > On 2014/03/26 02:55:05, hirono wrote:
> > > > > > Strictly our design guide forbid us to modify prototypes of builtin
> > > objects.
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...
> > > > > 
> > > > > Not "strictly". The guide says "Modifying other builtins like
> > > > Function.prototype
> > > > > is less dangerous".
> > > > > 
> > > > > Of cause, we can instead use a normal function like wrap(func,
thisObj).
> > But
> > > > I'm
> > > > > considering to replace bind() with wrap() in Files.app as well as in
> video
> > > > > player. Along the way, the same syntax as bind() is better to replace
> it,
> > > > since
> > > > > the only thing we need to do it replacing of the text.
> > > > 
> > > > SGTM. 
> > > 
> > > Out of curiosity. Why don't we put this code to #16? Is there any
advantage
> of
> > > wrap() over window.onerror?
> > 
> > There are 2 advantages.
> > 
> > (1) 
> > window.onerror is DOM error handler so it doesn't catch an exception in the
> > handler of non-DOM async function.
> > 
> > // Example1, error in handler of DOM async function
> > var count = 0;
> > window.onerror = function() { count++; }
> > setTimeout(function() { throw 'error'; }); // DOM function
> > setTimeout(function() { assert(count == 1); }, 100);
> > 
> > 
> > // Example2, error in handler of non-DOM async function
> > var count = 0;
> > window.onerror = function() { count++; }
> > chrome.local.storage.get(function() { throw 'error'; }); // non-DOM function
> > setTimeout(function() { assert(count == 0); }, 100);  // The exception is
not
> > caught!
> 
> Thanks for the explanation. This is very interesting, I didn't know about it.
> Looks like a bug in Chrome.

I assume it is by design, not a bug. But I have no clue.

> > (2) we can know the stack trace of caller in case of error. 
> 
> We can access the stacktrace via window.onerror, using the 5th argument. It
> seems to be a very recently added feature.
> http://html5.org/tools/web-apps-tracker?from=8085&to=8086

The 5th argument seems the stacktrace of inside handler. What we want to know
here is the stacktrace of the callee (= async function), in my example, the
stacktrace of setTimeout and chrome.local.storage.get.

In tot, we can see an async stacktrace in inspector:
https://code.google.com/p/chromium/issues/detail?id=272416.
But it doesn't appear on console.

> > 
> > 
> > > I'm a little bit concerned about violating the style guide here, which
> clearly
> > > says No for modifying prototypes of builtin types. Can't we avoid it?
> > > 
> > >
> >
>
https://codereview.chromium.org/209853011/diff/60001/chrome/browser/resources...
> > > chrome/browser/resources/video_player/js/error_util.js:39:
> > > window.JSErrorCount++;
> > > Aren't we incrementing JSErrorCount twice? #16 and #39?
> > 
> > I think double-counting is not a problem for this count, since what we want
to
> > know is whether any error is happened or not. But I agreed that it looks
> > confusing. I think it's better to add a comment.
> 
> If so, then how about changing to bool, to avoid confusion?

I think, number may help debugging in some case even if it is rough. How about
changing the name to 'JSErrorRoughCount'?