WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
71408
Implement MediaController.
https://bugs.webkit.org/show_bug.cgi?id=71408
Summary
Implement MediaController.
Jer Noble
Reported
2011-11-02 15:04:01 PDT
Implement MediaController. <
http://www.w3.org/TR/html5/video.html#mediacontroller
>
Attachments
Patch
(70.11 KB, patch)
2011-11-02 16:44 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(82.09 KB, patch)
2011-11-03 14:25 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(85.52 KB, patch)
2011-11-03 15:13 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(92.99 KB, patch)
2011-11-04 14:18 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(93.21 KB, patch)
2011-11-08 08:59 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(93.38 KB, patch)
2011-11-08 09:31 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(93.38 KB, patch)
2011-11-08 09:52 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(93.38 KB, patch)
2011-11-08 10:12 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(98.42 KB, patch)
2011-11-09 00:13 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(98.41 KB, patch)
2011-11-09 07:45 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(103.15 KB, patch)
2011-11-10 00:51 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(104.95 KB, patch)
2011-11-10 21:07 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(104.93 KB, patch)
2011-11-10 21:13 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(104.96 KB, patch)
2011-11-10 22:44 PST
,
Jer Noble
eric.carlson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(13)
View All
Add attachment
proposed patch, testcase, etc.
Jer Noble
Comment 1
2011-11-02 16:44:14 PDT
Created
attachment 113402
[details]
Patch
Jer Noble
Comment 2
2011-11-02 16:49:36 PDT
<
rdar://problem/10327011
>
Adam Barth
Comment 3
2011-11-02 16:56:12 PDT
Comment on
attachment 113402
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=113402&action=review
Thanks. It might be nice to add a link to the spec:
http://www.whatwg.org/specs/web-apps/current-work/#mediacontroller
I'm glad the EventTargetFactory.in file worked for you.
> Source/WebCore/bindings/js/JSHTMLMediaElementCustom.cpp:31 > +#if ENABLE(VIDEO)
Usually we put this between config.h and the first header file.
> Source/WebCore/dom/EventTargetFactory.in:16 > +MediaController
No conditional=VIDEO?
> Source/WebCore/html/MediaController.cpp:41 > + static HashMap<String, MediaController*>* map = new HashMap<String, MediaController*>();
DEFINE_STATIC_LOCAL ?
> Source/WebCore/html/MediaControllerInterface.h:39 > +class MediaControllerInterface {
So many virtual functions! :)
Adam Barth
Comment 4
2011-11-02 16:56:46 PDT
(I'll let someone who actually understands media do a real review.)
Jer Noble
Comment 5
2011-11-02 17:16:44 PDT
Comment on
attachment 113402
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=113402&action=review
>> Source/WebCore/bindings/js/JSHTMLMediaElementCustom.cpp:31 >> +#if ENABLE(VIDEO) > > Usually we put this between config.h and the first header file.
I'll move it.
>> Source/WebCore/dom/EventTargetFactory.in:16 >> +MediaController > > No conditional=VIDEO?
Whoops! Yes, I'll add that. This is the first I've seen of EventTargetFactory.
>> Source/WebCore/html/MediaController.cpp:41 >> + static HashMap<String, MediaController*>* map = new HashMap<String, MediaController*>(); > > DEFINE_STATIC_LOCAL ?
Sure thing.
Adam Barth
Comment 6
2011-11-02 17:20:46 PDT
> Whoops! Yes, I'll add that. This is the first I've seen of EventTargetFactory.
Yeah, that's why I got interested in your patch. I added it recently to generate all the boilerplate code for event targets.
Sam Weinig
Comment 7
2011-11-03 09:21:57 PDT
Comment on
attachment 113402
[details]
Patch r-ing due to Adam's comments.
Eric Carlson
Comment 8
2011-11-03 10:31:14 PDT
Comment on
attachment 113402
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=113402&action=review
> Source/WebCore/bindings/js/JSHTMLMediaElementCustom.cpp:3 > + * Copyright (C) 2008 Apple Inc. All Rights Reserved. > + *
How long have you been working on this patch? ;-)
> Source/WebCore/html/HTMLMediaElement.cpp:2488 > // of playback is forwards and the media element does not have a loop attribute specified, > float now = currentTime(); > if (m_playbackRate > 0) > - return dur > 0 && now >= dur && !loop(); > + return dur > 0 && now >= dur && (!loop() || m_mediaController); > > // or the current playback position is the earliest possible position and the direction > // of playback is backwards
The comments in the function are quoted directly from the spec. Please update the comments to match current spec text.
> Source/WebCore/html/HTMLMediaElement.cpp:3211 > + float currentTime = m_mediaController->currentTime(); > + if (currentTime < startTime() || currentTime > startTime() + duration()) > + return true;
It might be worth renaming "currentTime" to make it clear that it is the "media controller position".
> Source/WebCore/html/HTMLMediaElement.h:426 > + bool isLoading() const { return !m_currentSrc.isEmpty(); }
"isLoading" implies (to me) that the is networkState is NETWORK_LOADING, not that it has a url. I don't have another suggestion because I am not sure how this will be used.
> Source/WebCore/html/MediaController.cpp:163 > +void MediaController::setCurrentTime(float time, ExceptionCode& code) > +{ > + m_clock->setCurrentTime(time); > + for (size_t index = 0; index < m_mediaElements.size(); ++index) > + m_mediaElements[index]->seek(time, code); > +}
Do we need to call updateReadyState here?
> Source/WebCore/html/MediaController.cpp:170 > + if (m_playbackState == ENDED) { > + ExceptionCode code = 0; > + setCurrentTime(0, code); > + }
I don't see this step in the spec, although I might have overlooked. Please include spec sections and quote the spec text for this and other controller logic as we do in HTMLMediaElement.
> Source/WebCore/html/MediaController.cpp:174 > + m_paused = false; > + updateMediaElements(); > +}
You need to fire a 'play' event here. Also, don't you need to call updatePlaybackState here to "report the controller state of the MediaController."?
> Source/WebCore/html/MediaController.cpp:181 > +void MediaController::pause() > +{ > + m_paused = true; > + for (size_t index = 0; index < m_mediaElements.size(); ++index) > + m_mediaElements[index]->pause(); > +}
This should return early if m_paused is already true. It should fire a 'pause' event and "report the controller state of the MediaController."
> Source/WebCore/html/MediaController.cpp:186 > +void MediaController::setDefaultPlaybackRate(float rate) > +{ > + m_defaultPlaybackRate = rate; > +}
This needs to fire a 'ratechange' event.
> Source/WebCore/html/MediaController.cpp:195 > +void MediaController::setPlaybackRate(float rate) > +{ > + m_playbackRate = rate; > + m_clock->setPlayRate(rate); > + > + for (size_t index = 0; index < m_mediaElements.size(); ++index) > + m_mediaElements[index]->setPlaybackRate(rate); > +}
This needs to fire a 'ratechange' event.
> Source/WebCore/html/MediaController.cpp:202 > +void MediaController::setVolume(float level, ExceptionCode&) > +{ > + m_volume = level; > + for (size_t index = 0; index < m_mediaElements.size(); ++index) > + m_mediaElements[index]->updateVolume(); > +}
This should throw an IndexSizeError exception if the value is greater than 1 or less than 0. If it does change, it needs to fire a 'volumechange' event.
> Source/WebCore/html/MediaController.cpp:209 > +void MediaController::setMuted(bool flag) > +{ > + m_muted = flag; > + for (size_t index = 0; index < m_mediaElements.size(); ++index) > + m_mediaElements[index]->updateVolume(); > +}
This needs to fire a 'volumechange' event.
> Source/WebCore/html/MediaController.cpp:427 > + allPaused &= element->paused();
This is clever, but I had to scratch my head for a minute to figure out if it was correct so a simple if statement would probably be better so simpletons like me don't get confused.
> Source/WebCore/html/MediaController.cpp:443 > + allHaveEnded &= m_mediaElements[index]->ended();
Ditto.
Jer Noble
Comment 9
2011-11-03 11:25:26 PDT
(In reply to
comment #8
)
> (From update of
attachment 113402
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=113402&action=review
> > > Source/WebCore/bindings/js/JSHTMLMediaElementCustom.cpp:3 > > + * Copyright (C) 2008 Apple Inc. All Rights Reserved. > > + * > > How long have you been working on this patch? ;-)
Not that long. ;)
> > Source/WebCore/html/HTMLMediaElement.cpp:2488 > > // of playback is forwards and the media element does not have a loop attribute specified, > > float now = currentTime(); > > if (m_playbackRate > 0) > > - return dur > 0 && now >= dur && !loop(); > > + return dur > 0 && now >= dur && (!loop() || m_mediaController); > > > > // or the current playback position is the earliest possible position and the direction > > // of playback is backwards > > The comments in the function are quoted directly from the spec. Please update the comments to match current spec text.
Will do.
> > Source/WebCore/html/HTMLMediaElement.cpp:3211 > > + float currentTime = m_mediaController->currentTime(); > > + if (currentTime < startTime() || currentTime > startTime() + duration()) > > + return true; > > It might be worth renaming "currentTime" to make it clear that it is the "media controller position".
Sure thing.
> > Source/WebCore/html/HTMLMediaElement.h:426 > > + bool isLoading() const { return !m_currentSrc.isEmpty(); } > > "isLoading" implies (to me) that the is networkState is NETWORK_LOADING, not that it has a url. I don't have another suggestion because I am not sure how this will be used.
Well crud, I should have thought of that. Should the MediaController have an accessor for networkState which is the minimum networkState of its slaved elements?
> > Source/WebCore/html/MediaController.cpp:163 > > +void MediaController::setCurrentTime(float time, ExceptionCode& code) > > +{ > > + m_clock->setCurrentTime(time); > > + for (size_t index = 0; index < m_mediaElements.size(); ++index) > > + m_mediaElements[index]->seek(time, code); > > +} > > Do we need to call updateReadyState here?
Amazingly enough, the spec says no. Though if the /element's/ readyState changes as the result of a seek, the media controller's readyState will be updated anyway.
> > Source/WebCore/html/MediaController.cpp:170 > > + if (m_playbackState == ENDED) { > > + ExceptionCode code = 0; > > + setCurrentTime(0, code); > > + } > > I don't see this step in the spec, although I might have overlooked. Please include spec sections and quote the spec text for this and other controller logic as we do in HTMLMediaElement.
No, you're right. The spec doesn't say to do this. I'll remove it.
> > Source/WebCore/html/MediaController.cpp:174 > > + m_paused = false; > > + updateMediaElements(); > > +} > > You need to fire a 'play' event here. Also, don't you need to call updatePlaybackState here to "report the controller state of the MediaController."?
Yes on both counts. And come to think of it, this class really needs a "reportControllerState()" function for readability's sake.
> > Source/WebCore/html/MediaController.cpp:181 > > +void MediaController::pause() > > +{ > > + m_paused = true; > > + for (size_t index = 0; index < m_mediaElements.size(); ++index) > > + m_mediaElements[index]->pause(); > > +} > > This should return early if m_paused is already true. It should fire a 'pause' event and "report the controller state of the MediaController."
Ditto. In fact, I'll fill in the spec text for all of these functions.
> > Source/WebCore/html/MediaController.cpp:186 > > +void MediaController::setDefaultPlaybackRate(float rate) > > +{ > > + m_defaultPlaybackRate = rate; > > +} > > This needs to fire a 'ratechange' event.
Will change.
> > Source/WebCore/html/MediaController.cpp:195 > > +void MediaController::setPlaybackRate(float rate) > > +{ > > + m_playbackRate = rate; > > + m_clock->setPlayRate(rate); > > + > > + for (size_t index = 0; index < m_mediaElements.size(); ++index) > > + m_mediaElements[index]->setPlaybackRate(rate); > > +} > > This needs to fire a 'ratechange' event.
Ditto.
> > Source/WebCore/html/MediaController.cpp:202 > > +void MediaController::setVolume(float level, ExceptionCode&) > > +{ > > + m_volume = level; > > + for (size_t index = 0; index < m_mediaElements.size(); ++index) > > + m_mediaElements[index]->updateVolume(); > > +} > > This should throw an IndexSizeError exception if the value is greater than 1 or less than 0. If it does change, it needs to fire a 'volumechange' event.
Ok.
> > Source/WebCore/html/MediaController.cpp:209 > > +void MediaController::setMuted(bool flag) > > +{ > > + m_muted = flag; > > + for (size_t index = 0; index < m_mediaElements.size(); ++index) > > + m_mediaElements[index]->updateVolume(); > > +} > > This needs to fire a 'volumechange' event.
Ok.
> > Source/WebCore/html/MediaController.cpp:427 > > + allPaused &= element->paused(); > > This is clever, but I had to scratch my head for a minute to figure out if it was correct so a simple if statement would probably be better so simpletons like me don't get confused.
Ok.
> > Source/WebCore/html/MediaController.cpp:443 > > + allHaveEnded &= m_mediaElements[index]->ended(); > > Ditto.
Ok.
Jer Noble
Comment 10
2011-11-03 14:25:59 PDT
Created
attachment 113559
[details]
Patch Addressed all of Eric's and Adam's comments.
Early Warning System Bot
Comment 11
2011-11-03 14:45:14 PDT
Comment on
attachment 113559
[details]
Patch
Attachment 113559
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/10290110
WebKit Review Bot
Comment 12
2011-11-03 14:47:04 PDT
Comment on
attachment 113559
[details]
Patch
Attachment 113559
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10292002
Jer Noble
Comment 13
2011-11-03 15:13:04 PDT
Created
attachment 113563
[details]
Patch Added MediaController to various build files, hopefully solving the qt and chromium build errors.
Early Warning System Bot
Comment 14
2011-11-03 15:31:20 PDT
Comment on
attachment 113563
[details]
Patch
Attachment 113563
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/10288130
WebKit Review Bot
Comment 15
2011-11-03 15:39:50 PDT
Comment on
attachment 113563
[details]
Patch
Attachment 113563
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10289116
Gustavo Noronha (kov)
Comment 16
2011-11-03 17:06:05 PDT
Comment on
attachment 113563
[details]
Patch
Attachment 113563
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10289145
Jer Noble
Comment 17
2011-11-04 14:18:52 PDT
Created
attachment 113712
[details]
Patch Added fixes for chromium, qt, and windows ports.
Early Warning System Bot
Comment 18
2011-11-04 14:29:12 PDT
Comment on
attachment 113712
[details]
Patch
Attachment 113712
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/10336081
WebKit Review Bot
Comment 19
2011-11-04 15:00:01 PDT
Comment on
attachment 113712
[details]
Patch
Attachment 113712
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10337119
Gustavo Noronha (kov)
Comment 20
2011-11-04 15:08:56 PDT
Comment on
attachment 113712
[details]
Patch
Attachment 113712
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10338100
Arun Patole
Comment 21
2011-11-08 01:20:00 PST
Hi Jer, Just had a look at your patch. Below are some comments, may be useful.
> void HTMLMediaElement::setPlaybackRate(float rate) > { >- LOG(Media, "HTMLMediaElement::setPlaybackRate(%f)", rate); >+ if (m_mediaController) >+ return; >+ >+ setPlaybackRateInternal(rate); >+}
As per the spec: on setting the attribute must be set to the new value, and the playback will change speed (if the element is potentially playing and there is no current media controller). So, I think m_mediaController check should only be on below line and not the early return: if (m_player && potentiallyPlaying() && m_player->rate() != rate) I mean, playback rate should be set to new value even with mediacontroller but playback speed should be changed only when the condition mentioned in above line is met and there is no mediacontroller.
>@@ -2229,19 +2253,35 @@ void HTMLMediaElement::mediaPlayerTimeChanged(MediaPlayer*) >+ // Queue a task to fire a simple event named ended at the media element. > if (!m_sentEndEvent) { > m_sentEndEvent = true; > scheduleEvent(eventNames().endedEvent); >+ updateMediaController(); >+ } >+ // If the media element has a current media controller, >+ if (m_mediaController) { >+ // then report the controller state for the media element's current media controller. >+ m_mediaController->reportControllerState();
} I think reportControllerState() is getting called twice incase of !m_sentEndEvent (as updateMediaController() just calls reportControllerState), may not be what you want.
>+float MediaController::duration() const >+{ >+ float maxDuration = 0; >+ for (size_t index = 0; index < m_mediaElements.size(); ++index) >+ maxDuration = max(maxDuration, m_mediaElements[index]->duration()); >+ return maxDuration; >+}
should we have !isnan() check for m_mediaElements[index]->duration() here? One more thing, I think we can also cache the duration in MediaController and change it only if any of the slave media element's duration changes.
>+void MediaController::setDefaultPlaybackRate(float rate) >+{ >+ // The defaultPlaybackRate attribute, on setting, must set the MediaController's media controller >+ // default playback rate to the new value, >+ m_defaultPlaybackRate = rate; >+ >+ // then queue a task to fire a simple event named ratechange at the MediaController. >+ scheduleEvent(eventNames().ratechangeEvent); >+}
I think we need to check for (m_defaultPlaybackRate != rate) before actually changing rate and sending event.
>+void MediaController::setPlaybackRate(float rate) >+{ >+ // The playbackRate attribute, on setting, must set the MediaController's media controller >+ // playback rate to the new value, >+ m_clock->setPlayRate(rate); >+ >+ // then queue a task to fire a simple event named ratechange at the MediaController. >+ scheduleEvent(eventNames().ratechangeEvent); >+}
Ditto, I think we need to check for ( rate != m_clock->playRate()) before actually changing rate and sending event.
>+void MediaController::setVolume(float level, ExceptionCode& code) >+{ >+
Ditto, I think we need to check for ( m_volume != level) before actually changing volume and sending event.
>+void MediaController::setMuted(bool flag) >+{ >+ // The muted attribute, on setting, must set the MediaController's media controller mute override >+ // to the new value >+ m_muted = flag;
Ditto, I think we need (m_muted != flag) before changing it and sending event.
>+void MediaController::updateReadyState() >+{ >+ >+ case HAVE_FUTURE_DATA: >+ eventName = eventNames().canplayEvent; >+ break; >+ case HAVE_ENOUGH_DATA: >+ eventName = eventNames().canplaythroughEvent; >+ break;
I am not sure but I think, like HTMLMediaElement, you may also have to check for old ready states as well before deciding any event to send. For example, HTMLMediaElement checks for (oldState <= HAVE_CURRENT_DATA) before scheduling canplayEvent. similarily (oldState < HAVE_METADATA) for loadedMetaDataEvent, etc.
Jer Noble
Comment 22
2011-11-08 08:55:31 PST
(In reply to
comment #21
)
> As per the spec: > on setting the attribute must be set to the new value, and the playback will change speed (if the element is potentially playing and there is no current media controller). > > So, I think m_mediaController check should only be on below line and not the early return: > if (m_player && potentiallyPlaying() && m_player->rate() != rate) > > I mean, playback rate should be set to new value even with mediacontroller but playback speed should be changed only when the condition mentioned in above line is met and there is no mediacontroller.
That comma splice is ambiguous, but I see what you're saying. I'll change this.
> I think reportControllerState() is getting called twice incase of !m_sentEndEvent (as updateMediaController() just calls reportControllerState), may not be what you want.
Indeed it is! I'll remove the extra one.
> >+float MediaController::duration() const > >+{ > >+ float maxDuration = 0; > >+ for (size_t index = 0; index < m_mediaElements.size(); ++index) > >+ maxDuration = max(maxDuration, m_mediaElements[index]->duration()); > >+ return maxDuration; > >+} > > should we have !isnan() check for m_mediaElements[index]->duration() here? One more thing, I think we can also cache the duration in MediaController and change it only if any of the slave media element's duration changes.
Okay, I'll add a NaN check. I don't think I'll cache the max duration, though, because it's not cached at the HTMLMediaElement level, and it would add a little too much complexity to track when individual slaved elements' durations change. This could definitely be an optimization in the future, though. I'll add a FIXME.
> I think we need to check for (m_defaultPlaybackRate != rate) before actually changing rate and sending event.
Will do.
> Ditto, I think we need to check for ( rate != m_clock->playRate()) before actually changing rate and sending event.
Will do.
> Ditto, I think we need to check for ( m_volume != level) before actually changing volume and sending event.
Will do.
> Ditto, I think we need (m_muted != flag) before changing it and sending event.
Will do.
> I am not sure but I think, like HTMLMediaElement, you may also have to check for old ready states as well before deciding any event to send. For example, HTMLMediaElement checks for (oldState <= HAVE_CURRENT_DATA) before scheduling canplayEvent. similarily (oldState < HAVE_METADATA) for loadedMetaDataEvent, etc.
I think that MediaController is different from the HTMLMediaElement, though. It's much more likely for the readyState to bounce around as slaved media elements are added and removed. I could be wrong about this, but it seems to me that the spec is pretty cut and dried on this point. For example, for the "loadeddata" event: HTMLMediaElement: "readyState newly increased to HAVE_CURRENT_DATA or greater for the first time." MediaController: "All the slaved media elements newly have readyState set to HAVE_CURRENT_DATA or greater." So, I think I'll leave this the way it is, for now. Thanks for reviewing!
Jer Noble
Comment 23
2011-11-08 08:59:54 PST
Created
attachment 114086
[details]
Patch
Jer Noble
Comment 24
2011-11-08 09:07:57 PST
(In reply to
comment #22
)
> (In reply to
comment #21
) > > I am not sure but I think, like HTMLMediaElement, you may also have to check for old ready states as well before deciding any event to send. For example, HTMLMediaElement checks for (oldState <= HAVE_CURRENT_DATA) before scheduling canplayEvent. similarily (oldState < HAVE_METADATA) for loadedMetaDataEvent, etc. > > I think that MediaController is different from the HTMLMediaElement, though. It's much more likely for the readyState to bounce around as slaved media elements are added and removed. I could be wrong about this, but it seems to me that the spec is pretty cut and dried on this point. For example, for the "loadeddata" event: > > HTMLMediaElement: "readyState newly increased to HAVE_CURRENT_DATA or greater for the first time." > MediaController: "All the slaved media elements newly have readyState set to HAVE_CURRENT_DATA or greater." > > So, I think I'll leave this the way it is, for now.
Oh wow, I just realized that there's a glaring inconsistency in the spec. In 4.8.10.11.2, the table in step 2 of "report the controller state" doesn't match the table in 4.8.10.17 for Media Controllers. The former talks about the "readyState newly being X" and the latter talks about the "readyState newly being X or greater". I was treating the first table as normative and not the second. Depending on which one is correct, you may indeed be right about checking the oldState. I'll ask Hixie.
Jer Noble
Comment 25
2011-11-08 09:16:09 PST
I filed
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14731
to track the inconsistency in readyState events.
Jer Noble
Comment 26
2011-11-08 09:31:47 PST
Created
attachment 114093
[details]
Patch
Early Warning System Bot
Comment 27
2011-11-08 09:48:28 PST
Comment on
attachment 114093
[details]
Patch
Attachment 114093
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/10374135
Jer Noble
Comment 28
2011-11-08 09:52:35 PST
Created
attachment 114099
[details]
Patch Fixed an accidental bitwise-and -> logical-and mistake which (in addition to being wrong) caused a build error on the qt-ews bot.
Jer Noble
Comment 29
2011-11-08 10:12:40 PST
Created
attachment 114107
[details]
Patch Fixed another build error in MediaController.
Early Warning System Bot
Comment 30
2011-11-08 10:57:38 PST
Comment on
attachment 114107
[details]
Patch
Attachment 114107
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/10375153
WebKit Review Bot
Comment 31
2011-11-08 11:52:50 PST
Comment on
attachment 114107
[details]
Patch
Attachment 114107
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10365163
Gustavo Noronha (kov)
Comment 32
2011-11-08 21:10:24 PST
Comment on
attachment 114107
[details]
Patch
Attachment 114107
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10369295
Jer Noble
Comment 33
2011-11-09 00:13:24 PST
Created
attachment 114217
[details]
Patch Build fixes for win, qt, and chromium.
Jer Noble
Comment 34
2011-11-09 07:45:45 PST
Created
attachment 114277
[details]
Patch Rebased.
WebKit Review Bot
Comment 35
2011-11-09 08:55:42 PST
Comment on
attachment 114277
[details]
Patch
Attachment 114277
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10365467
New failing tests: media/media-controller-playback.html media/media-controller.html
Jer Noble
Comment 36
2011-11-10 00:51:45 PST
Created
attachment 114451
[details]
Patch Fixed the Chromium layout test failures. They were due to a horrible misdesign on my part. The MediaController mediaGroup->controller map was a global one, so media elements in separate documents with the same mediaGroup would get the same mediaController. This has been fixed, as has a infinite-recursion bug in ClockGeneric.
WebKit Review Bot
Comment 37
2011-11-10 01:47:33 PST
Comment on
attachment 114451
[details]
Patch
Attachment 114451
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10395273
New failing tests: media/media-controller.html
Gustavo Noronha (kov)
Comment 38
2011-11-10 02:34:22 PST
Comment on
attachment 114451
[details]
Patch
Attachment 114451
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10399295
Collabora GTK+ EWS bot
Comment 39
2011-11-10 02:45:31 PST
Comment on
attachment 114451
[details]
Patch
Attachment 114451
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10404268
Arun Patole
Comment 40
2011-11-10 04:08:41 PST
Couple of more things Jer,
>+++ b/Source/WebCore/html/HTMLMediaElement.idl >+ attribute [CustomSetter] MediaController mediaController;
HTMLMediaElement's media controller attribute name should be "controller" instead of "mediaController"
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-media-controller
Also, durationchange event needs to be fired whenever media controller's duration (The duration attribute :
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-mediacontroller-duration
) has just been updated
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#event-mediacontroller-durationchange
But I think this can be handled later.
Jer Noble
Comment 41
2011-11-10 21:04:41 PST
In response to <
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14731
> Ian Hickson modified <
http://html5.org/tools/web-apps-tracker?from=6823&to=6824
> the HTML5 specification to make explicit that events should be fired when moving through intermediate ready states. I've updated the patch to match the new spec. (In reply to
comment #40
)
> Couple of more things Jer, > >+++ b/Source/WebCore/html/HTMLMediaElement.idl > >+ attribute [CustomSetter] MediaController mediaController; > > HTMLMediaElement's media controller attribute name should be "controller" instead of "mediaController" >
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-media-controller
Changed. I'll get a new patch posted shortly.
Jer Noble
Comment 42
2011-11-10 21:07:24 PST
Created
attachment 114620
[details]
Patch
Jer Noble
Comment 43
2011-11-10 21:13:22 PST
Created
attachment 114621
[details]
Patch Rebased.
Early Warning System Bot
Comment 44
2011-11-10 21:31:02 PST
Comment on
attachment 114621
[details]
Patch
Attachment 114621
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/10424528
Gustavo Noronha (kov)
Comment 45
2011-11-10 22:20:38 PST
Comment on
attachment 114621
[details]
Patch
Attachment 114621
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10423463
Jer Noble
Comment 46
2011-11-10 22:44:42 PST
Created
attachment 114629
[details]
Patch Fixed an enum-related compile error.
Jer Noble
Comment 47
2011-11-11 00:03:36 PST
For posterity's sake, I'd like to note the latest patch is passing all the EWS bots. (Save the chromium bot, which is having problems with all patches.)
Eric Carlson
Comment 48
2011-11-11 15:12:51 PST
Comment on
attachment 114629
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=114629&action=review
> Source/JavaScriptCore/wtf/Platform.h:1110 > -#if PLATFORM(MAC) || (PLATFORM(WIN) && !OS(WINCE) && !PLATFORM(WIN_CAIRO)) > +#if PLATFORM(MAC)
This didn't make it into a change log.
> Source/WebCore/bindings/js/JSHTMLMediaElementCustom.cpp:43 > + // On setting, it must first remove the element's mediagroup attribute, if any, > + imp->setMediaGroup(String()); > + // and then set the current media controller to the given value. > + imp->setController(toMediaController(value));
Nit: it would be helpful to have the spec section noted here.
> Source/WebCore/bindings/v8/custom/V8HTMLMediaElementCustom.cpp:50 > + if (!controller) { > + throwError("Value is not of type MediaController"); > + return; > + } > + > + imp->setController(controller);
Doesn't this also need to clear the 'mediagroup' attribute as you do in JSC?
> Source/WebCore/html/HTMLMediaElement.cpp:142 > +typedef HashMap<Document*, HashSet<HTMLMediaElement*> > DocElementSetMap;
Might as well spell it out: Doc -> Document
> Source/WebCore/html/HTMLMediaElement.cpp:143 > +static DocElementSetMap& documentToElementSetMap()
None of these static functions made it into the ChangeLog.
> Source/WebCore/html/HTMLMediaElement.cpp:2288 > + // then seek to the earliest possible position of the media resource and abort these steps. > seek(0, ignoredException);
m_player->startTime() is the "earliest possible position" (although in practice I think it return 0 for all ports currently).
> Source/WebCore/html/HTMLMediaElement.cpp:3219 > + // 2. Let m have no current media controller, if it currently has one. > + setController(0); > + // 3. If m's mediagroup attribute is being removed, then abort these steps.
Nit: I think a blank line between code and the next comment make it easier to read.
> Source/WebCore/html/HTMLMediaElement.h:427 > + bool hasSource() const { return m_networkState != NETWORK_EMPTY || m_networkState != NETWORK_NO_SOURCE; }
I am not wild about "haveSource", I assumed it meant "have a 'src' attribute".
> Source/WebCore/html/MediaController.cpp:224 > + m_clock->setPlayRate(rate); > + > + // then queue a task to fire a simple event named ratechange at the MediaController. > + scheduleEvent(eventNames().ratechangeEvent); > +}
Don't you also need to set the rates of all attached elements?
> Source/WebCore/html/MediaController.cpp:376 > + // then queue a task that, if the MediaController object is a playing media controller, and > + // all of the MediaController's slaved media elements have still ended playback, and the > + // media controller playback rate is still positive or zero, > + if (!m_paused && hasEnded()) { > + // changes the MediaController object to a paused media controller > + m_paused = true; > + m_clock->stop(); > + > + // and then fires a simple event named pause at the MediaController object. > + scheduleEvent(eventNames().pauseEvent); > + }
"then queue a task ..." means the logic is supposed to happen after the current run loop cycle ends. I think it would be fine to file a bug for this and fix it in a follow up patch.
> Source/WebCore/html/MediaController.cpp:443 > + // or if any of its slaved media elements whose autoplaying flag is true still have their > + // paused attribute set to true, > + if (element->autoplay() && element->paused()) > + return true;
The "autoplaying flag" is not the same as the 'autoplay' attribute, t is a private state kept in m_autoplaying.
> Source/WebCore/html/MediaController.cpp:456 > + if (m_clock->playRate() <= 0) > + return false;
This confused me when I read it, can you please include the spec text as you have done above?
> Source/WebCore/html/MediaController.cpp:491 > +ScriptExecutionContext* MediaController::scriptExecutionContext() const > +{ > + if (m_mediaElements.isEmpty()) > + return 0; > + return m_mediaElements.first()->document();
This assumes that all elements are in the same document. Is this always true?
> LayoutTests/ChangeLog:1 > +2011-11-03 Jer Noble <
jer.noble@apple.com
>
One...
> LayoutTests/ChangeLog:13 > +2011-11-03 Jer Noble <
jer.noble@apple.com
>
Two...
> LayoutTests/ChangeLog:25 > +2011-11-02 Jer Noble <
jer.noble@apple.com
>
Three times the fun!
> LayoutTests/ChangeLog:35 > + * media/media-controller-expected.txt: Added. > + * media/media-controller-playback-expected.txt: Added. > + * media/media-controller-playback.html: Added. > + * media/media-controller.html: Added.
We definitely need more tests for this feature! Please file a master bug and we can block others on it.
> LayoutTests/ChangeLog:36 > +
I think you also need to update fast/dom/Window/window-properties-expected.txt and fast/js/global-constructors-expected.txt because of the changes to DOMWindow.idl
> LayoutTests/media/media-controller-playback.html:16 > + video = document.getElementsByTagName('video')[0]; > + video2 = document.getElementsByTagName('video')[1]; > + run('controller = video.mediaController'); > + controller.addEventListener('canplaythrough', canplaythrough, true); > + setSrcByTagName('video', findMediaFile('video', 'content/test'));
Three calls to "document.getElementsByTagName('video')" in one function?
> LayoutTests/platform/mac-lion/fast/regions/render-region-custom-style-mark-expected.txt:1 > +layer at (0,0) size 800x600
Did you mean to change this result?
Jer Noble
Comment 49
2011-11-11 16:55:53 PST
(In reply to
comment #48
)
> (From update of
attachment 114629
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=114629&action=review
> > > Source/JavaScriptCore/wtf/Platform.h:1110 > > -#if PLATFORM(MAC) || (PLATFORM(WIN) && !OS(WINCE) && !PLATFORM(WIN_CAIRO)) > > +#if PLATFORM(MAC) > > This didn't make it into a change log.
Whoops. Added.
> > Source/WebCore/bindings/js/JSHTMLMediaElementCustom.cpp:43 > > + // On setting, it must first remove the element's mediagroup attribute, if any, > > + imp->setMediaGroup(String()); > > + // and then set the current media controller to the given value. > > + imp->setController(toMediaController(value)); > > Nit: it would be helpful to have the spec section noted here.
Okay, I added it here and to the V8 version.
> > Source/WebCore/bindings/v8/custom/V8HTMLMediaElementCustom.cpp:50 > > + if (!controller) { > > + throwError("Value is not of type MediaController"); > > + return; > > + } > > + > > + imp->setController(controller); > > Doesn't this also need to clear the 'mediagroup' attribute as you do in JSC?
Yes it does.
> > Source/WebCore/html/HTMLMediaElement.cpp:142 > > +typedef HashMap<Document*, HashSet<HTMLMediaElement*> > DocElementSetMap; > > Might as well spell it out: Doc -> Document
Done.
> > Source/WebCore/html/HTMLMediaElement.cpp:143 > > +static DocElementSetMap& documentToElementSetMap() > > None of these static functions made it into the ChangeLog.
Added.
> > Source/WebCore/html/HTMLMediaElement.cpp:2288 > > + // then seek to the earliest possible position of the media resource and abort these steps. > > seek(0, ignoredException); > > m_player->startTime() is the "earliest possible position" (although in practice I think it return 0 for all ports currently).
Indeed. Changed it anyway.
> > Source/WebCore/html/HTMLMediaElement.cpp:3219 > > + // 2. Let m have no current media controller, if it currently has one. > > + setController(0); > > + // 3. If m's mediagroup attribute is being removed, then abort these steps. > > Nit: I think a blank line between code and the next comment make it easier to read.
Inserted.
> > Source/WebCore/html/HTMLMediaElement.h:427 > > + bool hasSource() const { return m_networkState != NETWORK_EMPTY || m_networkState != NETWORK_NO_SOURCE; } > > I am not wild about "haveSource", I assumed it meant "have a 'src' attribute".
Changed this to "isLoading()". It's here to avoid having to pull the NetworkState enum into MediaControllerInterface.
> > Source/WebCore/html/MediaController.cpp:224 > > + m_clock->setPlayRate(rate); > > + > > + // then queue a task to fire a simple event named ratechange at the MediaController. > > + scheduleEvent(eventNames().ratechangeEvent); > > +} > > Don't you also need to set the rates of all attached elements?
I do. Added a function called "updatePlaybackRate()" to HTMLMediaElement which does this.
> > Source/WebCore/html/MediaController.cpp:376 > > + // then queue a task that, if the MediaController object is a playing media controller, and > > + // all of the MediaController's slaved media elements have still ended playback, and the > > + // media controller playback rate is still positive or zero, > > + if (!m_paused && hasEnded()) { > > + // changes the MediaController object to a paused media controller > > + m_paused = true; > > + m_clock->stop(); > > + > > + // and then fires a simple event named pause at the MediaController object. > > + scheduleEvent(eventNames().pauseEvent); > > + } > > "then queue a task ..." means the logic is supposed to happen after the current run loop cycle ends. I think it would be fine to file a bug for this and fix it in a follow up patch.
Okay.
> > Source/WebCore/html/MediaController.cpp:443 > > + // or if any of its slaved media elements whose autoplaying flag is true still have their > > + // paused attribute set to true, > > + if (element->autoplay() && element->paused()) > > + return true; > > The "autoplaying flag" is not the same as the 'autoplay' attribute, t is a private state kept in m_autoplaying.
Okay. I added a private, const accessor which returns the value of this variable, for use in MediaController.
> > Source/WebCore/html/MediaController.cpp:456 > > + if (m_clock->playRate() <= 0) > > + return false; > > This confused me when I read it, can you please include the spec text as you have done above?
Done.
> > Source/WebCore/html/MediaController.cpp:491 > > +ScriptExecutionContext* MediaController::scriptExecutionContext() const > > +{ > > + if (m_mediaElements.isEmpty()) > > + return 0; > > + return m_mediaElements.first()->document(); > > This assumes that all elements are in the same document. Is this always true?
Nope. It may not be in a document at all. Changed the IDL to generate a constructor that takes a ScriptExecutionContext and changed the MediaController constructor to match.
> > LayoutTests/ChangeLog:1 > > +2011-11-03 Jer Noble <
jer.noble@apple.com
> > > One... > > > LayoutTests/ChangeLog:13 > > +2011-11-03 Jer Noble <
jer.noble@apple.com
> > > Two... > > > LayoutTests/ChangeLog:25 > > +2011-11-02 Jer Noble <
jer.noble@apple.com
> > > Three times the fun!
Un-triplicated.
> > LayoutTests/ChangeLog:35 > > + * media/media-controller-expected.txt: Added. > > + * media/media-controller-playback-expected.txt: Added. > > + * media/media-controller-playback.html: Added. > > + * media/media-controller.html: Added. > > We definitely need more tests for this feature! Please file a master bug and we can block others on it.
Sure thing!
> > LayoutTests/ChangeLog:36 > > + > > I think you also need to update fast/dom/Window/window-properties-expected.txt and fast/js/global-constructors-expected.txt because of the changes to DOMWindow.idl
Done.
> > LayoutTests/media/media-controller-playback.html:16 > > + video = document.getElementsByTagName('video')[0]; > > + video2 = document.getElementsByTagName('video')[1]; > > + run('controller = video.mediaController'); > > + controller.addEventListener('canplaythrough', canplaythrough, true); > > + setSrcByTagName('video', findMediaFile('video', 'content/test')); > > Three calls to "document.getElementsByTagName('video')" in one function?
Okay, made this one call.
> > LayoutTests/platform/mac-lion/fast/regions/render-region-custom-style-mark-expected.txt:1 > > +layer at (0,0) size 800x600 > > Did you mean to change this result?
Nope, reverted.
Jer Noble
Comment 50
2011-11-14 09:55:31 PST
Committed
r100159
: <
http://trac.webkit.org/changeset/100159
>
Simon Fraser (smfr)
Comment 51
2011-11-15 16:07:38 PST
This broke some tests:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100159%20(34689)/fast/dom/constructed-objects-prototypes-pretty-diff.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100159%20(34689)/fast/dom/Window/window-property-descriptors-pretty-diff.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100159%20(34689)/fast/dom/prototype-inheritance-2-pretty-diff.html
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug