Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
412813002:
Video Player: Support casting a video (Closed)
|
Created:
6 years, 5 months ago by yoshiki Modified:
6 years, 5 months ago Reviewers:
hirono CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionVideo Player: Support casting a video
This patch adds the feature to controls a video on Chromecast using CastVideoElement and Cast Extension.
BUG=305511
TEST=List of casts is shown
R=hirono@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285170
Patch Set 1 #
Total comments: 34
Patch Set 2 : addressed the comments #
Total comments: 2
Patch Set 3 : addressed the comments #
Total comments: 2
Patch Set 4 : addressed the comments #
Total comments: 4
Patch Set 5 : addressed the comment #
Messages
Total messages: 15 (0 generated)
@hirono, PTAL. Thanks.
https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/cast/cast_video_element.js (right): https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:7: // Inverval for updating media info (in ms). nit: jsdoc @const, @type https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:15: * @constructor nit: @param https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:44: * The total time of the video. Comment about the unit of time is useful. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:45: * @type {number} nit: @return Same for the followings. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:49: this.lastDuration_ = this.castMedia_.media.duration; Changing a value in a getter is a bit strange. Can we update the lastDuration when the this.castMedia_.media is updated? https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:58: * @type {number} @return {?number} ? https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:64: // TODO(yoshiki): Support seek. How about adding throw new Error('Not implemented') or console.error('Not implemented'); https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:75: return this.castMedia_.playerState == chrome.cast.media.PlayerState.PAUSED; === ? Same for others. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:142: * @return {null} Maybe string is OK because it's nullable. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:221: this.castMedia_ = media; Should we check this.castMedia_ == null since it must be unloaded properly by unloadMedia_? https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:280: videoElementInitializePromise = new Promise(function(fulfill, reject) { To relay rejected states, the videoElementInitializePromise should be composed from the other promises. 1. Assign the returning value of 'then' at 304 to the variable. 2. Wrap chrome.cast.request at #308 with 'new Promise' and return the promise as the result value of the callback function for then at 304. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:290: }.wrap()); We don't need to wrap the function for Promise. Because Promise is rejected automatically if the function throw an exception. Instead, you should call 'catch' function for the eventual composed promise, which is the return value of then at #331 in this case. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:336: console.log('loaded: ', currentPos, this.currentPos_); nit: debug log?
@hirono, PTAL again? https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/cast/cast_video_element.js (right): https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:7: // Inverval for updating media info (in ms). On 2014/07/24 02:12:36, hirono wrote: > nit: jsdoc @const, @type Done. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:15: * @constructor On 2014/07/24 02:12:36, hirono wrote: > nit: @param Done. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:44: * The total time of the video. On 2014/07/24 02:12:36, hirono wrote: > Comment about the unit of time is useful. Done. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:45: * @type {number} On 2014/07/24 02:12:36, hirono wrote: > nit: @return > Same for the followings. I think we don't need it because it's a getter method. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:49: this.lastDuration_ = this.castMedia_.media.duration; On 2014/07/24 02:12:36, hirono wrote: > Changing a value in a getter is a bit strange. > Can we update the lastDuration when the this.castMedia_.media is updated? Done. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:58: * @type {number} On 2014/07/24 02:12:36, hirono wrote: > @return {?number} ? Done. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:64: // TODO(yoshiki): Support seek. On 2014/07/24 02:12:36, hirono wrote: > How about adding throw new Error('Not implemented') or console.error('Not > implemented'); Please leave it as it is, since this setter is called frequently. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:75: return this.castMedia_.playerState == chrome.cast.media.PlayerState.PAUSED; On 2014/07/24 02:12:36, hirono wrote: > === ? Same for others. Done. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:142: * @return {null} On 2014/07/24 02:12:36, hirono wrote: > Maybe string is OK because it's nullable. changed to '?string' https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:221: this.castMedia_ = media; On 2014/07/24 02:12:36, hirono wrote: > Should we check this.castMedia_ == null since it must be unloaded properly by > unloadMedia_? Done. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:280: videoElementInitializePromise = new Promise(function(fulfill, reject) { On 2014/07/24 02:12:37, hirono wrote: > To relay rejected states, the videoElementInitializePromise should be composed > from the other promises. > > 1. Assign the returning value of 'then' at 304 to the variable. > 2. Wrap chrome.cast.request at #308 with 'new Promise' and return the promise as > the result value of the callback function for then at 304. I'm not sure but can't we call the outer reject inside the inner promise directly? https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:290: }.wrap()); On 2014/07/24 02:12:36, hirono wrote: > We don't need to wrap the function for Promise. Because Promise is rejected > automatically if the function throw an exception. > Instead, you should call 'catch' function for the eventual composed promise, > which is the return value of then at #331 in this case. > Done. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:336: console.log('loaded: ', currentPos, this.currentPos_); On 2014/07/24 02:12:37, hirono wrote: > nit: debug log? Done.
https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/cast/cast_video_element.js (right): https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:45: * @type {number} On 2014/07/24 02:57:31, yoshiki wrote: > On 2014/07/24 02:12:36, hirono wrote: > > nit: @return > > Same for the followings. > > I think we don't need it because it's a getter method. nit: So please just make #37 consistent with others. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:64: // TODO(yoshiki): Support seek. On 2014/07/24 02:57:32, yoshiki wrote: > On 2014/07/24 02:12:36, hirono wrote: > > How about adding throw new Error('Not implemented') or console.error('Not > > implemented'); > > Please leave it as it is, since this setter is called frequently. SGTM. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:142: * @return {null} On 2014/07/24 02:57:32, yoshiki wrote: > On 2014/07/24 02:12:36, hirono wrote: > > Maybe string is OK because it's nullable. > > changed to '?string' Yes, I misunderstood. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:280: videoElementInitializePromise = new Promise(function(fulfill, reject) { On 2014/07/24 02:57:32, yoshiki wrote: > On 2014/07/24 02:12:37, hirono wrote: > > To relay rejected states, the videoElementInitializePromise should be composed > > from the other promises. > > > > 1. Assign the returning value of 'then' at 304 to the variable. > > 2. Wrap chrome.cast.request at #308 with 'new Promise' and return the promise > as > > the result value of the callback function for then at 304. > > I'm not sure but can't we call the outer reject inside the inner promise > directly? We can. But currently if downloadUrlPromise or mimePromise is rejected, the outer promise does not fulfilled/rejected because Promise.all([downloadUrlPromise, mimePromise]) is also rejected, and thus the following then's callback is not invoked. Composing promises is a good practice (I think) to ensure to convey rejected states to the last promise. https://codereview.chromium.org/412813002/diff/60001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/60001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:343: console.error('Failed to initialize the video element.'); The argument of the error handler is useful. It may be Error thrown in callback functions, so you can print the stack trace of the exception as following. console.error('Failed to...', error.stack || error);
@hirono, PATL. Thanks. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/cast/cast_video_element.js (right): https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/cast/cast_video_element.js:45: * @type {number} On 2014/07/24 03:42:49, hirono wrote: > On 2014/07/24 02:57:31, yoshiki wrote: > > On 2014/07/24 02:12:36, hirono wrote: > > > nit: @return > > > Same for the followings. > > > > I think we don't need it because it's a getter method. > > nit: So please just make #37 consistent with others. Done. https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:280: videoElementInitializePromise = new Promise(function(fulfill, reject) { On 2014/07/24 03:42:49, hirono wrote: > On 2014/07/24 02:57:32, yoshiki wrote: > > On 2014/07/24 02:12:37, hirono wrote: > > > To relay rejected states, the videoElementInitializePromise should be > composed > > > from the other promises. > > > > > > 1. Assign the returning value of 'then' at 304 to the variable. > > > 2. Wrap chrome.cast.request at #308 with 'new Promise' and return the > promise > > as > > > the result value of the callback function for then at 304. > > > > I'm not sure but can't we call the outer reject inside the inner promise > > directly? > > We can. But currently if downloadUrlPromise or mimePromise is rejected, the > outer promise does not fulfilled/rejected because > Promise.all([downloadUrlPromise, mimePromise]) is also rejected, and thus the > following then's callback is not invoked. > > Composing promises is a good practice (I think) to ensure to convey rejected > states to the last promise. I agree this. How about the current code? https://codereview.chromium.org/412813002/diff/60001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/60001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:343: console.error('Failed to initialize the video element.'); On 2014/07/24 03:42:49, hirono wrote: > The argument of the error handler is useful. > It may be Error thrown in callback functions, so you can print the stack trace > of the exception as following. > > console.error('Failed to...', error.stack || error); Done.
https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:280: videoElementInitializePromise = new Promise(function(fulfill, reject) { On 2014/07/24 05:19:00, yoshiki wrote: > On 2014/07/24 03:42:49, hirono wrote: > > On 2014/07/24 02:57:32, yoshiki wrote: > > > On 2014/07/24 02:12:37, hirono wrote: > > > > To relay rejected states, the videoElementInitializePromise should be > > composed > > > > from the other promises. > > > > > > > > 1. Assign the returning value of 'then' at 304 to the variable. > > > > 2. Wrap chrome.cast.request at #308 with 'new Promise' and return the > > promise > > > as > > > > the result value of the callback function for then at 304. > > > > > > I'm not sure but can't we call the outer reject inside the inner promise > > > directly? > > > > We can. But currently if downloadUrlPromise or mimePromise is rejected, the > > outer promise does not fulfilled/rejected because > > Promise.all([downloadUrlPromise, mimePromise]) is also rejected, and thus the > > following then's callback is not invoked. > > > > Composing promises is a good practice (I think) to ensure to convey rejected > > states to the last promise. > > I agree this. How about the current code? It looks good. But I would rather stop nesting 'new Promise'. It makes the indent shallow and frees us from having both fulfill/reject and innerFulfill/innerReject. https://codereview.chromium.org/412813002/diff/80001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/80001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:298: }.wrap()); The exception thrown in the callback will be reported doubly. How about passing innerFulfill to getDriveEntryProperties directly and map the result value by continuing 'then'.
@hirono, PTAL again? https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/20001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:280: videoElementInitializePromise = new Promise(function(fulfill, reject) { On 2014/07/24 05:46:09, hirono wrote: > On 2014/07/24 05:19:00, yoshiki wrote: > > On 2014/07/24 03:42:49, hirono wrote: > > > On 2014/07/24 02:57:32, yoshiki wrote: > > > > On 2014/07/24 02:12:37, hirono wrote: > > > > > To relay rejected states, the videoElementInitializePromise should be > > > composed > > > > > from the other promises. > > > > > > > > > > 1. Assign the returning value of 'then' at 304 to the variable. > > > > > 2. Wrap chrome.cast.request at #308 with 'new Promise' and return the > > > promise > > > > as > > > > > the result value of the callback function for then at 304. > > > > > > > > I'm not sure but can't we call the outer reject inside the inner promise > > > > directly? > > > > > > We can. But currently if downloadUrlPromise or mimePromise is rejected, the > > > outer promise does not fulfilled/rejected because > > > Promise.all([downloadUrlPromise, mimePromise]) is also rejected, and thus > the > > > following then's callback is not invoked. > > > > > > Composing promises is a good practice (I think) to ensure to convey rejected > > > states to the last promise. > > > > I agree this. How about the current code? > > It looks good. But I would rather stop nesting 'new Promise'. It makes the > indent shallow and frees us from having both fulfill/reject and > innerFulfill/innerReject. Done. https://codereview.chromium.org/412813002/diff/80001/ui/file_manager/video_pl... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/80001/ui/file_manager/video_pl... ui/file_manager/video_player/js/video_player.js:298: }.wrap()); On 2014/07/24 05:46:09, hirono wrote: > The exception thrown in the callback will be reported doubly. > How about passing innerFulfill to getDriveEntryProperties directly and map the > result value by continuing 'then'. Done.
https://codereview.chromium.org/412813002/diff/100001/ui/file_manager/video_p... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/100001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:311: }.wrap(this), reject, undefined, this.currentCast_.label); This is the last. Could you remove the wrap as well? Thank you!
https://codereview.chromium.org/412813002/diff/100001/ui/file_manager/video_p... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/100001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:311: }.wrap(this), reject, undefined, this.currentCast_.label); On 2014/07/24 06:10:38, hirono wrote: > This is the last. Could you remove the wrap as well? Thank you! I think this is necessary. An exception in a callback of chrome APIs doesn't provide detailed information or are sometimes just ignored. To let us know the information of error, this wrap is useful.
https://codereview.chromium.org/412813002/diff/100001/ui/file_manager/video_p... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/100001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:311: }.wrap(this), reject, undefined, this.currentCast_.label); On 2014/07/24 06:16:52, yoshiki wrote: > On 2014/07/24 06:10:38, hirono wrote: > > This is the last. Could you remove the wrap as well? Thank you! > > I think this is necessary. An exception in a callback of chrome APIs doesn't > provide detailed information or are sometimes just ignored. To let us know the > information of error, this wrap is useful. Though I didn't noticed it before, if an exception is thrown in the callback of requestSession, it does not turn the promise rejected because the caller of the callback function is the chrome API, not the constructor of Promise. Maybe we should: return new Promise(function(fulfill, reject) { chrome.cast.requestSession(fulfill, reject, undefined, this.currentCast_.lable); }).then(function(session) { ... }.bind(this));
PTAL https://codereview.chromium.org/412813002/diff/100001/ui/file_manager/video_p... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/412813002/diff/100001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:311: }.wrap(this), reject, undefined, this.currentCast_.label); On 2014/07/24 06:35:55, hirono wrote: > On 2014/07/24 06:16:52, yoshiki wrote: > > On 2014/07/24 06:10:38, hirono wrote: > > > This is the last. Could you remove the wrap as well? Thank you! > > > > I think this is necessary. An exception in a callback of chrome APIs doesn't > > provide detailed information or are sometimes just ignored. To let us know the > > information of error, this wrap is useful. > > Though I didn't noticed it before, if an exception is thrown in the callback of > requestSession, it does not turn the promise rejected because the caller of the > callback function is the chrome API, not the constructor of Promise. > > Maybe we should: > > return new Promise(function(fulfill, reject) { > chrome.cast.requestSession(fulfill, reject, undefined, > this.currentCast_.lable); > }).then(function(session) { > ... > }.bind(this)); I got it. Thanks for clear explanation.
Thanks! 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/412813002/120001
Message was sent while issue was closed.
Committed patchset #5 manually as r285170 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
