Description
Piotr Grad
2013-11-06 00:47:17 PST
Created attachment 216147 [details]
Patch & layout test
Created attachment 216149 [details]
Patch & layout test
For which version of the spec is this? http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html Last updated on the 1st of November doesn't have that change. (In reply to comment #3) > For which version of the spec is this? > > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html > Last updated on the 1st of November doesn't have that change. 4.7.10.16 Event summary ... ended Event All the slaved media elements have newly ended playback. and: 4.8.10.8 Playing the media resource ... A media element is said to have ended playback when: ... The current playback position is the earliest possible position, and The direction of playback is backwards. (In reply to comment #0) > Ended event was triggered only when payback was going forward. According to W3C specification ended event should be triggered also when playback rate is negative and reaches the earliest position. This is correct, the latest editor's draft says [1]: A media element is said to have ended playback when: The element's readyState attribute is HAVE_METADATA or greater, and Either: The current playback position is the end of the media resource, and The direction of playback is forwards, and Either the media element does not have a loop attribute specified, or the media element has a current media controller. Or: The current playback position is the earliest possible position, and The direction of playback is backwards. [1] http://www.w3.org/html/wg/drafts/html/CR/embedded-content-0.html#ended-playback Comment on attachment 216149 [details] Patch & layout test View in context: https://bugs.webkit.org/attachment.cgi?id=216149&action=review > LayoutTests/media/video-ended-event-negative-playback.html:17 > + function () > + { > + video.currentTime = 2; > + video.playbackRate = -1; > + video.play(); > + failTestIn(2500); > + }); This test will always take at least 2 seconds to complete.This seems excessive, can you make it something like .5 instead? > Source/WebCore/html/HTMLMediaElement.cpp:3679 > double dur = duration(); Nit: can you please get rid of the "dur" abbreviation while you are here: double duration = this->duration(); Thanks for the link Eric. Created attachment 216268 [details]
Patch & layout test
Comment on attachment 216268 [details] Patch & layout test Attachment 216268 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22598177 New failing tests: media/video-ended-event-negative-playback.html Created attachment 216276 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 216278 [details]
Patch & layout test
loop attribute should be omitted for negative playback rate.
Comment on attachment 216278 [details] Patch & layout test Attachment 216278 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22728179 New failing tests: media/video-ended-event-negative-playback.html Created attachment 216283 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216278 [details] Patch & layout test Attachment 216278 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22608167 New failing tests: media/video-ended-event-negative-playback.html Created attachment 216291 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216278 [details] Patch & layout test Attachment 216278 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22508197 New failing tests: media/video-ended-event-negative-playback.html Created attachment 216296 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
This is failing on the Mac because the results have an extra blank line: --- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/media/video-ended-event-negative-playback-expected.txt +++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/media/video-ended-event-negative-playback-actual.txt @@ -1,3 +1,4 @@ + EVENT(canplaythrough) EVENT(ended) END OF TEST Comment on attachment 216278 [details] Patch & layout test View in context: https://bugs.webkit.org/attachment.cgi?id=216278&action=review > LayoutTests/media/video-ended-event-negative-playback.html:22 > + <video loop/> I wonder if the failure on the Mac is because of this: a video element is not a void element, it must have an end tag > Source/WebCore/html/HTMLMediaElement.cpp:3679 > double dur = duration(); As before, can you please get rid of the "dur" abbreviation while you are here: double duration = this->duration(); Created attachment 216376 [details]
Patch & layout test
(In reply to comment #19) > (From update of attachment 216278 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216278&action=review > > > LayoutTests/media/video-ended-event-negative-playback.html:22 > > + <video loop/> > > I wonder if the failure on the Mac is because of this: a video element is not a void element, it must have an end tag > > > Source/WebCore/html/HTMLMediaElement.cpp:3679 > > double dur = duration(); > > As before, can you please get rid of the "dur" abbreviation while you are here: > > double duration = this->duration(); I cannot change 'dur' to 'duration' because then i would get compiler error. Var name cannot override function name used in same scope. Created attachment 216377 [details]
Pathc & layout test ver.2
Comment on attachment 216376 [details] Patch & layout test Attachment 216376 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/21248580 New failing tests: media/video-ended-event-negative-playback.html Created attachment 216384 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216377 [details] Pathc & layout test ver.2 Attachment 216377 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22508518 New failing tests: media/video-ended-event-negative-playback.html Created attachment 216385 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
(In reply to comment #26) > Created an attachment (id=216385) [details] > Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 > > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. > Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5 This fail is because of regression in mac port. Comment on attachment 216377 [details] Pathc & layout test ver.2 Attachment 216377 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22728471 New failing tests: media/video-ended-event-negative-playback.html Created attachment 216393 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216376 [details] Patch & layout test Attachment 216376 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22588480 New failing tests: media/video-ended-event-negative-playback.html Created attachment 216396 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
(In reply to comment #21) > (In reply to comment #19) > > (From update of attachment 216278 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=216278&action=review > > > > > LayoutTests/media/video-ended-event-negative-playback.html:22 > > > + <video loop/> > > > > I wonder if the failure on the Mac is because of this: a video element is not a void element, it must have an end tag > > > > > Source/WebCore/html/HTMLMediaElement.cpp:3679 > > > double dur = duration(); > > > > As before, can you please get rid of the "dur" abbreviation while you are here: > > > > double duration = this->duration(); > > I cannot change 'dur' to 'duration' because then i would get compiler error. Var name cannot override function name used in same scope. Really? Did you actually try my suggestion? (In reply to comment #27) > (In reply to comment #26) > > Created an attachment (id=216385) [details] [details] > > Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 > > > > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. > > Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5 > > This fail is because of regression in mac port. Which regression is that? When I apply your patch and run on the Mac, the test fails because it has an extra blank line at the top of the results. I assume this is because of bug 123479, which made media elements a replaced element. (In reply to comment #32) > (In reply to comment #21) > > (In reply to comment #19) > > > (From update of attachment 216278 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=216278&action=review > > > > > > > LayoutTests/media/video-ended-event-negative-playback.html:22 > > > > + <video loop/> > > > > > > I wonder if the failure on the Mac is because of this: a video element is not a void element, it must have an end tag > > > > > > > Source/WebCore/html/HTMLMediaElement.cpp:3679 > > > > double dur = duration(); > > > > > > As before, can you please get rid of the "dur" abbreviation while you are here: > > > > > > double duration = this->duration(); > > > > I cannot change 'dur' to 'duration' because then i would get compiler error. Var name cannot override function name used in same scope. > > Really? Did you actually try my suggestion? Yes, i tried to be 100% sure after your second question about it. Created attachment 216436 [details]
Patch & layout test with new line
(In reply to comment #33) > (In reply to comment #27) > > (In reply to comment #26) > > > Created an attachment (id=216385) [details] [details] [details] > > > Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 > > > > > > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. > > > Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5 > > > > This fail is because of regression in mac port. > > Which regression is that? > > When I apply your patch and run on the Mac, the test fails because it has an extra blank line at the top of the results. I assume this is because of bug 123479, which made media elements a replaced element. Yes, i saw it. Wonder why it was passing on other platforms. (In reply to comment #34) > (In reply to comment #32) > > (In reply to comment #21) > > > (In reply to comment #19) > > > > (From update of attachment 216278 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=216278&action=review > > > > > > > > > LayoutTests/media/video-ended-event-negative-playback.html:22 > > > > > + <video loop/> > > > > > > > > I wonder if the failure on the Mac is because of this: a video element is not a void element, it must have an end tag > > > > > > > > > Source/WebCore/html/HTMLMediaElement.cpp:3679 > > > > > double dur = duration(); > > > > > > > > As before, can you please get rid of the "dur" abbreviation while you are here: > > > > > > > > double duration = this->duration(); > > > > > > I cannot change 'dur' to 'duration' because then i would get compiler error. Var name cannot override function name used in same scope. > > > > Really? Did you actually try my suggestion? > Yes, i tried to be 100% sure after your second question about it. Odd, add "this->" to the method call makes it work for me. Comment on attachment 216436 [details] Patch & layout test with new line Clearing flags on attachment: 216436 Committed r158965: <http://trac.webkit.org/changeset/158965> All reviewed patches have been landed. Closing bug. This behavior is incorrect. The latest WHATWG spec still says: "When the current playback position reaches the earliest possible position of the media resource when the direction of playback is backwards, then the user agent must only queue a task to fire a simple event named timeupdate at the element." So, while the ended property will return 'true' in this case, it is incorrect to fire an 'ended' event. Ack, worse than that: "The ended attribute must return true if, the last time the event loop reached step 1, the media element had ended playback and the direction of playback was forwards, and false otherwise." So this entire behavior is incorrect. Currently, we fire an 'ended' event, and return false from video.ended. |