Bug 71410 – MediaControls should use MediaController if present.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

MediaControls should use MediaController if present. <http://www.w3.org/TR/html5/video.html#user-interface>

[Jer Noble]

Created attachment 113403 [details]
Patch

[Jer Noble]

<rdar://problem/10387345>

[Jer Noble]

Created attachment 114096 [details]
Patch

[Eric Carlson]

(In reply to comment #0)
> MediaControls should use MediaController if present. <http://www.w3.org/TR/html5/video.html#user-interface>

You should use the editors draft, http://dev.w3.org/html5/spec/the-iframe-element.html#user-interface, not the current working draft.

[Eric Carlson]

Comment on attachment 114096 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114096&action=review

> Source/WebCore/html/shadow/MediaControlElements.cpp:693
> +        ExceptionCode code = 0;
> +        mediaController()->setCurrentTime(max(0.0f, mediaController()->currentTime() - 30), code);
>          event->setDefaultHandled();

Nit: you ignore an exception so initialization isn't helpful.

> Source/WebCore/html/shadow/MediaControlElements.cpp:852
>      slider->setAttribute(maxAttr, "1");
> -    slider->setAttribute(valueAttr, String::number(mediaElement->volume()));
>      return slider.release();

Why does this no longer set the current value?

> Source/WebCore/html/shadow/MediaControlElements.cpp:902
>      slider->setAttribute(maxAttr, "1");
> -    slider->setAttribute(valueAttr, String::number(mediaElement->volume()));
>      return slider.release();

Ditto.

[Jer Noble]

Comment on attachment 114096 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114096&action=review

>> Source/WebCore/html/shadow/MediaControlElements.cpp:693
>>          event->setDefaultHandled();
> 
> Nit: you ignore an exception so initialization isn't helpful.

Sure thing.  I'll rename this to "ignoredCode" or something similar.

>> Source/WebCore/html/shadow/MediaControlElements.cpp:852
>>      return slider.release();
> 
> Why does this no longer set the current value?

This is due to passing in a Document* instead of a HTMLMediaElement*.

At the time at which MediaControlVolumeSliderElement ::create() is called, the root element does not have a MediaControllerInterface yet.  When MediaControlRootElement::setMediaController() is called, it will call reset(), which will update the volume on the slider.  This will happen in the same runloop, just after the shadow elements are all created.

>> Source/WebCore/html/shadow/MediaControlElements.cpp:902
>>      return slider.release();
> 
> Ditto.

Ditto. ;)

[Jer Noble]

Created attachment 114974 [details]
Patch

Updating the patch, now that dependeds-on patches have landed, to let the EWS bots chew on it.

[WebKit Review Bot]

Comment on attachment 114974 [details]
Patch

Attachment 114974 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10362281

New failing tests:
media/media-document-audio-repaint.html
media/controls-strict.html
media/controls-drag-timebar.html
media/controls-styling.html
fullscreen/video-controls-override.html
media/audio-repaint.html
media/controls-right-click-on-timebar.html
http/tests/inspector/network-preflight-options.html
media/audio-controls-rendering.html
media/audio-controls-do-not-fade-out.html
media/controls-without-preload.html
media/media-controls-invalid-url.html
media/media-controls-clone.html
fast/layers/video-layer.html
media/audio-delete-while-slider-thumb-clicked.html
media/controls-after-reload.html

[Jer Noble]

Committed r101810: <http://trac.webkit.org/changeset/101810>

[Hajime Morrita]

*** Bug 53001 has been marked as a duplicate of this bug. ***