Last Comment Bug 778902 - When a video finished playing, "pause" should be fired, and |paused| should be true.
: When a video finished playing, "pause" should be fired, and |paused| should b...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Paul Adenot (:padenot) (in PTO until early September)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-30 13:27 PDT by Paul Adenot (:padenot) (in PTO until early September)
Modified: 2012-08-08 09:31 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch. (968 bytes, patch)
2012-07-30 13:31 PDT, Paul Adenot (:padenot) (in PTO until early September)
no flags Details | Diff | Splinter Review
Patch with tests updated. (2.76 KB, patch)
2012-07-31 16:51 PDT, Paul Adenot (:padenot) (in PTO until early September)
cpearce: review+
Details | Diff | Splinter Review
When a video finished playing, "pause" should be fired, and |paused| should be true. (2.78 KB, patch)
2012-08-06 13:25 PDT, Paul Adenot (:padenot) (in PTO until early September)
padenot: review+
Details | Diff | Splinter Review

Description Paul Adenot (:padenot) (in PTO until early September) 2012-07-30 13:27:47 PDT
From the spec [1]:

> Queue a task that, if the media element does not have a current media
> controller, and the media element has still ended playback, and the direction
> of playback is still forwards, and paused is false, changes paused to true and
> fires a simple event named pause at the media element.

We don't have a MediaController, so the media element should be in paused state, and we should fire a paused event, here.

[1]: http://www.whatwg.org/html/the-video-element.html#ended-playback
Comment 1 Paul Adenot (:padenot) (in PTO until early September) 2012-07-30 13:31:53 PDT
Created attachment 647276 [details] [diff] [review]
Patch.

If the reading of the spec is correct (i.e. we don't have a media controller), here is a patch.
Comment 2 Paul Adenot (:padenot) (in PTO until early September) 2012-07-30 19:07:51 PDT
Changed the relevant tests, green locally (including video controls tests), pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=701737d85fc5
Comment 3 Paul Adenot (:padenot) (in PTO until early September) 2012-07-31 16:51:34 PDT
Created attachment 647767 [details] [diff] [review]
Patch with tests updated.

The try push is green with this patch that has updated tests.

There is a behavioral change with this patch. If we seek to the beginning of a media after it has reached the end, it won't start automatically, i.e. the user will have to press play (or call play()) for the media to start playing again. 

Maybe we want to change that to keep the current behavior.
Comment 4 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-08-05 15:15:23 PDT
(In reply to Paul ADENOT (:padenot) from comment #2)
> Changed the relevant tests, green locally (including video controls tests),
> pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=701737d85fc5

You should do a try run with all tests enabled; <video> is used in several different test suites, not only in mochitest-1. For example the a11y tests use video, and they're not in mochitest-1.  I usually just do a try-all run when I make platform/API changes; it hard to keep track of who's added code which uses the APIs we're working on!
Comment 5 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-08-05 15:23:49 PDT
Comment on attachment 647767 [details] [diff] [review]
Patch with tests updated.

Review of attachment 647767 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with two minor changes.

You should do a try-all run before landing this, particularly once you've changed to call Pause() rather than re-duplicating the pause logic, since that may change behaviour further...

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2771,5 @@
>      SetCurrentTime(0);
>      return;
>    }
>  
> +  mPaused = true;

I think you should call Pause() here, rather than trying to manage all the state... For example, you're not resetting mAutoPlaying here, and I'm pretty sure you should be...

::: content/media/test/test_paused_after_ended.html
@@ +14,5 @@
>  
>  function ended(evt) {
>    var v = evt.target;
> +  is(v.gotPause, true, "We should have received a \"pause\" event.")
> +  is(v.paused, true, v._name + " must not be paused after end");

Actually, you're asserting that we *are* paused after [playback has reached the] end! ;)

Did you mean:

v._name + "must have received ended after pause event" ?
Comment 6 Paul Adenot (:padenot) (in PTO until early September) 2012-08-06 13:25:04 PDT
Created attachment 649387 [details] [diff] [review]
When a video finished playing, "pause" should be fired, and |paused| should be true.
Comment 7 Paul Adenot (:padenot) (in PTO until early September) 2012-08-06 13:27:02 PDT
Comment on attachment 649387 [details] [diff] [review]
When a video finished playing, "pause" should be fired, and |paused| should be true.

Addressed comment, green on try [1]. Carrying forward r+.

[1]: https://tbpl.mozilla.org/?tree=Try&rev=859080478900
Comment 8 Matthew Gregan [:kinetik] 2012-08-07 15:59:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2940e190783f
Comment 9 Ed Morley [:emorley] 2012-08-08 09:31:42 PDT
https://hg.mozilla.org/mozilla-central/rev/2940e190783f

Note You need to log in before you can comment on or make changes to this bug.