Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
235953010:
Resolve the issue of unexpected currentTime values when setting currentTime from seeking event (Closed)
|
Created:
6 years, 8 months ago by Srirama Modified:
6 years, 8 months ago Reviewers:
acolwell GONE FROM CHROMIUM CC:
blink-reviews, nessy, philipj_slow, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, adamk+blink_chromium.org, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@mediaposter Visibility:
Public. |
DescriptionResolve the issue of unexpected currentTime values when setting currentTime from seeking event
BUG=104536
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172133
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fixed review comments #Patch Set 3 : Fixed layout test failure #Patch Set 4 : Added new line in the expected output to match the test case #
Messages
Total messages: 29 (0 generated)
As mentioned in the issue, double seek to the same position causes WebKit to prematurely fire the seeking & seeked events. It does so without taking into account that the media engine is still processing the initial seek. So if there is a seek in progress handle the state properly and avoid unnecessary seeking and seeked events. PTAL
looks pretty good. Just a few comments. https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... File LayoutTests/media/video-double-seek-currentTime-expected.txt (right): https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... LayoutTests/media/video-double-seek-currentTime-expected.txt:1: EXPECTED (video.currentTime == '1.5') OK These expectations don't look right. I'd expect to see the seeked & seeking messages here. https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... File LayoutTests/media/video-double-seek-currentTime.html (right): https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... LayoutTests/media/video-double-seek-currentTime.html:1: <video controls></video> nit: Please add <!DOCTYPE html> and place this stuff inside <html><head> like a normal web page would be. Also move the <video> into a <body> tag and provide a test description in the <body> like other tests do. https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... LayoutTests/media/video-double-seek-currentTime.html:8: function log(str) { Remove this function and replace all calls with consoleWrite(). That is what the other tests use. https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... LayoutTests/media/video-double-seek-currentTime.html:16: function seeking(e) { nit: { for functions should be on the next line just like in Blink C++ source files. Fix here and everywhere below. https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... LayoutTests/media/video-double-seek-currentTime.html:23: log("seeked " + e.target.currentTime); nit: The logged value should be fixed as well to avoid platform differences. https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... LayoutTests/media/video-double-seek-currentTime.html:28: log('Expected ' + expectedSeek + ' got ' + now); nit: use failTest() here and early return. https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... LayoutTests/media/video-double-seek-currentTime.html:38: switch (seekCount) { nit: fix indent. https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... LayoutTests/media/video-double-seek-currentTime.html:51: log('doNextSeek() seeking to ' + newSeekPoint); nit: use toFixed() here. https://codereview.chromium.org/235953010/diff/1/LayoutTests/media/video-doub... LayoutTests/media/video-double-seek-currentTime.html:64: //var video = document.getElementById('video'); nit: Please remove if this isn't needed. https://codereview.chromium.org/235953010/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/235953010/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:1721: bool seekPending = false; nit: Just assign m_seeking to seekPending here and drop the conditional below. Also a name like previousSeekStillPending might eliminate the need for the comment where this variable is used below.
Modified as per the review comments. PTAL.
lgtm
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/235953010/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/235953010/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg tryserver.blink on win_blink_rel
PTAL. There is an extra space in the test case which was removed now.
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/235953010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
Added newline in the expected output to match the test case.
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/235953010/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/235953010/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/235953010/80001
Message was sent while issue was closed.
Change committed as 172133 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
