Bug 71408

Created attachment 113402 [details] Patch

<rdar://problem/10327011>

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! :)

(I'll let someone who actually understands media do a real review.)

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.

> 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.

Comment on attachment 113402 [details] Patch r-ing due to Adam's comments.

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.

(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.

Created attachment 113559 [details] Patch Addressed all of Eric's and Adam's comments.

Comment on attachment 113559 [details] Patch Attachment 113559 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10290110

Comment on attachment 113559 [details] Patch Attachment 113559 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10292002

Created attachment 113563 [details] Patch Added MediaController to various build files, hopefully solving the qt and chromium build errors.

Comment on attachment 113563 [details] Patch Attachment 113563 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10288130

Comment on attachment 113563 [details] Patch Attachment 113563 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10289116

Comment on attachment 113563 [details] Patch Attachment 113563 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10289145

Created attachment 113712 [details] Patch Added fixes for chromium, qt, and windows ports.

Comment on attachment 113712 [details] Patch Attachment 113712 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10336081

Comment on attachment 113712 [details] Patch Attachment 113712 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10337119

Comment on attachment 113712 [details] Patch Attachment 113712 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10338100

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.

(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!

Created attachment 114086 [details] Patch

(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.

I filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=14731 to track the inconsistency in readyState events.

Created attachment 114093 [details] Patch

Comment on attachment 114093 [details] Patch Attachment 114093 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10374135

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.

Created attachment 114107 [details] Patch Fixed another build error in MediaController.

Comment on attachment 114107 [details] Patch Attachment 114107 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10375153

Comment on attachment 114107 [details] Patch Attachment 114107 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10365163

Comment on attachment 114107 [details] Patch Attachment 114107 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10369295

Created attachment 114217 [details] Patch Build fixes for win, qt, and chromium.

Created attachment 114277 [details] Patch Rebased.

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

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.

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

Comment on attachment 114451 [details] Patch Attachment 114451 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10399295

Comment on attachment 114451 [details] Patch Attachment 114451 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10404268

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.

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.

Created attachment 114620 [details] Patch

Created attachment 114621 [details] Patch Rebased.

Comment on attachment 114621 [details] Patch Attachment 114621 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10424528

Comment on attachment 114621 [details] Patch Attachment 114621 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10423463

Created attachment 114629 [details] Patch Fixed an enum-related compile error.

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.)

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?

(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.

Committed r100159: <http://trac.webkit.org/changeset/100159>

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