Video Player: Add a test to open a single video file

Video Player: Add a test to open a single video file

BUG=405415
TEST=browser_test passes

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

[yoshiki, 2014-08-20 03:53:44]

@hirono, PTAL. Thanks

[hirono, 2014-08-20 04:14:07]

https://codereview.chromium.org/485013003/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/common/test_util_common.js
(right):

https://codereview.chromium.org/485013003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/common/test_util_common.js:16:
VolumeManager.getInstance(fulfill);
VolumeManager.getInstance returns promise.
resolveDisplayRoot also returns promise now.
So we can do:

VolumeManager.getInstance().then(function(volumeManager) {
  ...
  return volumeInfo.resolveDisplayRoot();
});

https://codereview.chromium.org/485013003/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js
(right):

https://codereview.chromium.org/485013003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js:20:
testVolumeName, volumeType, entries, opt_selected) {
nit: #19 and #20 matches one line?

https://codereview.chromium.org/485013003/diff/1/ui/file_manager/video_player...
File ui/file_manager/video_player/js/background.js (right):

https://codereview.chromium.org/485013003/diff/1/ui/file_manager/video_player...
ui/file_manager/video_player/js/background.js:70: * @param {Array.<Object>}
videos List of videos to play.
nit: @return

https://codereview.chromium.org/485013003/diff/1/ui/file_manager/video_player...
ui/file_manager/video_player/js/background.js:90: console.error('Launch failed',
error.stack || error);
How about doing: return Promise.reject(error) ?
It make the returned promise rejected, and thus the test is failed by the error.

[yoshiki, 2014-08-20 04:26:21]

@hirono, PTAL

https://codereview.chromium.org/485013003/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/common/test_util_common.js
(right):

https://codereview.chromium.org/485013003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/common/test_util_common.js:16:
VolumeManager.getInstance(fulfill);
On 2014/08/20 04:14:06, hirono wrote:
> VolumeManager.getInstance returns promise.
> resolveDisplayRoot also returns promise now.
> So we can do:
> 
> VolumeManager.getInstance().then(function(volumeManager) {
>   ...
>   return volumeInfo.resolveDisplayRoot();
> });

Done.

https://codereview.chromium.org/485013003/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js
(right):

https://codereview.chromium.org/485013003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js:20:
testVolumeName, volumeType, entries, opt_selected) {
On 2014/08/20 04:14:06, hirono wrote:
> nit: #19 and #20 matches one line?

Done.

https://codereview.chromium.org/485013003/diff/1/ui/file_manager/video_player...
File ui/file_manager/video_player/js/background.js (right):

https://codereview.chromium.org/485013003/diff/1/ui/file_manager/video_player...
ui/file_manager/video_player/js/background.js:70: * @param {Array.<Object>}
videos List of videos to play.
On 2014/08/20 04:14:06, hirono wrote:
> nit: @return

Done.

https://codereview.chromium.org/485013003/diff/1/ui/file_manager/video_player...
ui/file_manager/video_player/js/background.js:90: console.error('Launch failed',
error.stack || error);
On 2014/08/20 04:14:06, hirono wrote:
> How about doing: return Promise.reject(error) ?
> It make the returned promise rejected, and thus the test is failed by the
error.

Done.

[hirono, 2014-08-20 04:33:28]

https://codereview.chromium.org/485013003/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/common/test_util_common.js
(right):

https://codereview.chromium.org/485013003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/common/test_util_common.js:19:
volumeInfo.resolveDisplayRoot(fulfill, reject);
return volumeInfo.resolveDisplayRoot() ?

https://codereview.chromium.org/485013003/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/gallery/test_util.js
(right):

https://codereview.chromium.org/485013003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/gallery/test_util.js:21:
return backgroundComponentsPromise;
Probably, return backgroundComponentsPromise; is not needed.

https://codereview.chromium.org/485013003/diff/40001/ui/file_manager/video_pl...
File ui/file_manager/video_player/js/background.js (right):

https://codereview.chromium.org/485013003/diff/40001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/background.js:71: * @param {Primise} Promise to
be fulfilled on success, or rejected on error.
typo: Primise -> Promise

[yoshiki, 2014-08-20 06:18:14]

@hirono, PTAL

https://codereview.chromium.org/485013003/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/common/test_util_common.js
(right):

https://codereview.chromium.org/485013003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/common/test_util_common.js:19:
volumeInfo.resolveDisplayRoot(fulfill, reject);
On 2014/08/20 04:33:27, hirono wrote:
> return volumeInfo.resolveDisplayRoot() ?

Done.

https://codereview.chromium.org/485013003/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/gallery/test_util.js
(right):

https://codereview.chromium.org/485013003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/gallery/test_util.js:21:
return backgroundComponentsPromise;
On 2014/08/20 04:33:28, hirono wrote:
> Probably, return backgroundComponentsPromise; is not needed.

I'm not sure but I follow you because you know much about gallery.

https://codereview.chromium.org/485013003/diff/40001/ui/file_manager/video_pl...
File ui/file_manager/video_player/js/background.js (right):

https://codereview.chromium.org/485013003/diff/40001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/background.js:71: * @param {Primise} Promise to
be fulfilled on success, or rejected on error.
On 2014/08/20 04:33:28, hirono wrote:
> typo: Primise -> Promise

Done.

[hirono, 2014-08-20 06:35:38]

lgtm. Thank you!

[yoshiki, 2014-08-20 06:48:30]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-08-20 06:49:17]

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

[commit-bot: I haz the power, 2014-08-20 08:29:13]

Committed patchset #3 (60001) as 290791