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.
(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();
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 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
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
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();
(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 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
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.
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
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.
(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.
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.
2013-11-06 00:49 PST, Piotr Grad
2013-11-06 00:54 PST, Piotr Grad
2013-11-07 00:35 PST, Piotr Grad
2013-11-07 01:32 PST, Build Bot
2013-11-07 01:43 PST, Piotr Grad
buildbot: commit-queue-
2013-11-07 03:13 PST, Build Bot
2013-11-07 04:44 PST, Build Bot
2013-11-07 05:41 PST, Build Bot
2013-11-08 05:17 PST, Piotr Grad
2013-11-08 05:22 PST, Piotr Grad
2013-11-08 06:04 PST, Build Bot
2013-11-08 06:12 PST, Build Bot
2013-11-08 06:56 PST, Build Bot
2013-11-08 07:15 PST, Build Bot
2013-11-08 14:08 PST, Piotr Grad