Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1782363003:
Add subtitles track if there is a subtitles file with the same name. (Closed)
|
Created:
4 years, 9 months ago by ryoh Modified:
4 years, 9 months ago Reviewers:
yoshiki CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, fukino, hirono Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd subtitles track if there is a subtitles file with the same name.
BUG=585723
TEST=manually
Committed: https://crrev.com/9b39fa15a01e930a8ff2a5a53a2fd13c73482bb9
Cr-Commit-Position: refs/heads/master@{#381395}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : simplify #
Total comments: 11
Patch Set 4 : updated #Patch Set 5 : update test #Patch Set 6 : fix test #
Messages
Total messages: 25 (13 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Add subtitles track if there is a subtitles file with the same name. BUG=585723 ========== to ========== Add subtitles track if there is a subtitles file with the same name. BUG=585723 TEST=manually ==========
ryoh@chromium.org changed reviewers: + yoshiki@chromium.org
Hi, I added the support for WebVVT, a subtitle format for HTML5, to our VideoPlayer. Could you take a look? https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... File ui/file_manager/video_player/js/video_player.js (left): https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:417: this.videoElement_.load(); Why do we need this line? It seems 'loadedmetadata' event will be fired whether we call this function or not. If you call this function here, we can't see subtitles. (I think it might be a bug of blink)
https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... File ui/file_manager/video_player/js/video_player.js (left): https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:417: this.videoElement_.load(); On 2016/03/11 15:11:58, ryoh wrote: > Why do we need this line? > It seems 'loadedmetadata' event will be fired whether we call this function or > not. > If you call this function here, we can't see subtitles. > (I think it might be a bug of blink) According to the spec, we need to call load() unless 'src' attribute on 'video'-tag element is modified. https://dev.w3.org/html5/spec-author-view/video.html So, I think we should do this. However, even if the video player works correctly without this, you may remove it but please leave the comment. And if you think it's a bug, please file a bug to http://crbug.com/new. Thanks. https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:403: return Promise.resolve(); nit: returning resolve is unnecessary. please remove. https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:446: VideoPlayer.prototype.loadSubtitles_ = function(url) { nit: Please write a JSdoc (short description of the method and parameter). https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:448: var resolveLocalFileSystem = function(extension) { "resolveLocalFileSystem" looks confusing with resolveLocalFileSystemURL(). "resolveLocalFileSystemWIthExtension" might be better? https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:451: } nit: semicolon
Thank you for your review! I updated the patch. Could you take a look? https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... File ui/file_manager/video_player/js/video_player.js (left): https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:417: this.videoElement_.load(); On 2016/03/14 04:10:52, yoshiki wrote: > On 2016/03/11 15:11:58, ryoh wrote: > > Why do we need this line? > > It seems 'loadedmetadata' event will be fired whether we call this function or > > not. > > If you call this function here, we can't see subtitles. > > (I think it might be a bug of blink) > > According to the spec, we need to call load() unless 'src' attribute on > 'video'-tag element is modified. > https://dev.w3.org/html5/spec-author-view/video.html > So, I think we should do this. > > However, even if the video player works correctly without this, you may remove > it but please leave the comment. > > And if you think it's a bug, please file a bug to http://crbug.com/new. Thanks. Yeah, if we **modified** src attribute, you have to call load(), but we always create new 'video' element (IIRC). Maybe we don't have to call load(). OK, I reported this as a bug: https://bugs.chromium.org/p/chromium/issues/detail?id=594537 https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... File ui/file_manager/video_player/js/video_player.js (right): https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:403: return Promise.resolve(); On 2016/03/14 04:10:52, yoshiki wrote: > nit: returning resolve is unnecessary. please remove. Done. https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:446: VideoPlayer.prototype.loadSubtitles_ = function(url) { On 2016/03/14 04:10:52, yoshiki wrote: > nit: Please write a JSdoc (short description of the method and parameter). Done. https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:448: var resolveLocalFileSystem = function(extension) { On 2016/03/14 04:10:52, yoshiki wrote: > "resolveLocalFileSystem" looks confusing with resolveLocalFileSystemURL(). > "resolveLocalFileSystemWIthExtension" might be better? Sounds nice! Thank you. https://codereview.chromium.org/1782363003/diff/60001/ui/file_manager/video_p... ui/file_manager/video_player/js/video_player.js:451: } On 2016/03/14 04:10:52, yoshiki wrote: > nit: semicolon Done.
Thanks for addressing the comments. lgtm.
The CQ bit was checked by ryoh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1782363003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1782363003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ryoh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoshiki@chromium.org Link to the patchset: https://codereview.chromium.org/1782363003/#ps100001 (title: "update test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1782363003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1782363003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by ryoh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1782363003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1782363003/100001
The CQ bit was checked by ryoh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoshiki@chromium.org Link to the patchset: https://codereview.chromium.org/1782363003/#ps120001 (title: "fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1782363003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1782363003/120001
Message was sent while issue was closed.
Description was changed from ========== Add subtitles track if there is a subtitles file with the same name. BUG=585723 TEST=manually ========== to ========== Add subtitles track if there is a subtitles file with the same name. BUG=585723 TEST=manually ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add subtitles track if there is a subtitles file with the same name. BUG=585723 TEST=manually ========== to ========== Add subtitles track if there is a subtitles file with the same name. BUG=585723 TEST=manually Committed: https://crrev.com/9b39fa15a01e930a8ff2a5a53a2fd13c73482bb9 Cr-Commit-Position: refs/heads/master@{#381395} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9b39fa15a01e930a8ff2a5a53a2fd13c73482bb9 Cr-Commit-Position: refs/heads/master@{#381395} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
