Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
307863004:
[Video Player] Re-factoring the code (Closed)
|
Created:
6 years, 6 months ago by yoshiki Modified:
6 years, 6 months ago Reviewers:
mtomasz CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Video Player] Re-factoring the code
Convert the video app into a class. No functionality change.
BUG=171191
TEST=browser test passes.
R=mtomasz@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274182
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressed the comments #
Total comments: 8
Patch Set 3 : Addressed the comments #
Messages
Total messages: 10 (0 generated)
@mtomasz: PTAL. Thanks.
https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:63: * @param {string} message Message id. nit: Can we make it private? https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:73: } nit: ; missing. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:76: * Handles playback (decoder) errors. nit: @private missing. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:111: this.controls = null; nit: Can we make them private? https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:121: * @param {Array.<Object.<string, Object>>} videos List of video to play. nit: ... -> List of videos. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:161: * @param {function(number, number)=} callback Callback nit: callback -> opt_callback https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:161: * @param {function(number, number)=} callback Callback nit: Callback -> Completion callback. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:197: * Unlaods the current video. typo: Unloads. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:212: // TODO: chrome.app.window soon will be able to resize the content area. FYI: From M36, we can finally do that. https://developer.chrome.com/apps/app_window#type-AppWindow -> innerBounds. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:286: var initPhase1 = Promise.all([new Promise(initVideos.wrap(null)), nit: initPhase1 and initPhase2 naming is not helpful. How about either: 1. Chaining it: Promise.all(...).then() 2. Or renaming initPhase1 to initPromise, and removing initPhase2, since it is not used?
@mtomasz: Thanks. PTAL. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:63: * @param {string} message Message id. On 2014/05/29 13:44:12, mtomasz wrote: > nit: Can we make it private? Done. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:73: } On 2014/05/29 13:44:12, mtomasz wrote: > nit: ; missing. Done. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:76: * Handles playback (decoder) errors. On 2014/05/29 13:44:12, mtomasz wrote: > nit: @private missing. Done. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:111: this.controls = null; On 2014/05/29 13:44:12, mtomasz wrote: > nit: Can we make them private? Done. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:121: * @param {Array.<Object.<string, Object>>} videos List of video to play. On 2014/05/29 13:44:12, mtomasz wrote: > nit: ... -> List of videos. Done. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:161: * @param {function(number, number)=} callback Callback On 2014/05/29 13:44:12, mtomasz wrote: > nit: Callback -> Completion callback. Done. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:161: * @param {function(number, number)=} callback Callback On 2014/05/29 13:44:12, mtomasz wrote: > nit: callback -> opt_callback Done. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:197: * Unlaods the current video. On 2014/05/29 13:44:12, mtomasz wrote: > typo: Unloads. Done. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:212: // TODO: chrome.app.window soon will be able to resize the content area. On 2014/05/29 13:44:12, mtomasz wrote: > FYI: From M36, we can finally do that. > > https://developer.chrome.com/apps/app_window#type-AppWindow -> innerBounds. Thanks for guidance. I'll change it in a separated patch. https://codereview.chromium.org/307863004/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:286: var initPhase1 = Promise.all([new Promise(initVideos.wrap(null)), On 2014/05/29 13:44:12, mtomasz wrote: > nit: initPhase1 and initPhase2 naming is not helpful. How about either: > > 1. Chaining it: > Promise.all(...).then() > > 2. Or renaming initPhase1 to initPromise, and removing initPhase2, since it is > not used? Done #2. Thanks.
https://codereview.chromium.org/307863004/diff/60001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/307863004/diff/60001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:117: this.__defineGetter__('controls', function() { I think get {} is preferred over __defineGetter__. Any reason for using __defineGetter__? __defineGetter__ is a non standard feature, while "get" is a part of the JS standard. https://codereview.chromium.org/307863004/diff/60001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:146: _}.wrap(this); nit: _ accidential change? https://codereview.chromium.org/307863004/diff/60001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:206: VideoPlayer.prototype.unloadVideo = function() { nit: unloadVideo -> unloadVideo_ https://codereview.chromium.org/307863004/diff/60001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:300: return new Promise(player.playVideo_.wrap(player)); nit: Accessing a private member. We should make it public.
@mtomasz: PTAL. Thanks. https://codereview.chromium.org/307863004/diff/60001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/307863004/diff/60001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:117: this.__defineGetter__('controls', function() { On 2014/05/29 23:16:33, mtomasz wrote: > I think get {} is preferred over __defineGetter__. Any reason for using > __defineGetter__? > > __defineGetter__ is a non standard feature, while "get" is a part of the JS > standard. Done. https://codereview.chromium.org/307863004/diff/60001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:146: _}.wrap(this); On 2014/05/29 23:16:33, mtomasz wrote: > nit: _ accidential change? Sorry, Removed. https://codereview.chromium.org/307863004/diff/60001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:206: VideoPlayer.prototype.unloadVideo = function() { On 2014/05/29 23:16:33, mtomasz wrote: > nit: unloadVideo -> unloadVideo_ This method is public. Removed @private from JSDOC. https://codereview.chromium.org/307863004/diff/60001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:300: return new Promise(player.playVideo_.wrap(player)); On 2014/05/29 23:16:33, mtomasz wrote: > nit: Accessing a private member. We should make it public. Good catch. DONE.
lgtm
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/307863004/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Committed patchset #3 manually as r274182 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
