Bug 71410 - MediaControls should use MediaController if present.
Summary: MediaControls should use MediaController if present.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL: http://www.w3.org/TR/html5/video.html...
Keywords: InRadar
: 53001 (view as bug list)
Depends on: 71408
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-02 15:07 PDT by Jer Noble
Modified: 2011-12-04 20:57 PST (History)
6 users (show)

See Also:


Attachments
Patch (80.45 KB, patch)
2011-11-02 16:45 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (92.20 KB, patch)
2011-11-08 09:42 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (92.20 KB, patch)
2011-11-14 10:20 PST, Jer Noble
eric.carlson: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-11-02 15:07:28 PDT
MediaControls should use MediaController if present. <http://www.w3.org/TR/html5/video.html#user-interface>
Comment 1 Jer Noble 2011-11-02 16:45:29 PDT
Created attachment 113403 [details]
Patch
Comment 2 Jer Noble 2011-11-02 16:49:57 PDT
<rdar://problem/10387345>
Comment 3 Jer Noble 2011-11-08 09:42:50 PST
Created attachment 114096 [details]
Patch
Comment 4 Eric Carlson 2011-11-09 08:00:11 PST
(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.
Comment 5 Eric Carlson 2011-11-09 08:19:25 PST
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.
Comment 6 Jer Noble 2011-11-09 09:05:02 PST
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. ;)
Comment 7 Jer Noble 2011-11-14 10:20:20 PST
Created attachment 114974 [details]
Patch

Updating the patch, now that dependeds-on patches have landed, to let the EWS bots chew on it.
Comment 8 WebKit Review Bot 2011-11-14 10:52:37 PST
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
Comment 9 Jer Noble 2011-12-02 09:36:04 PST
Committed r101810: <http://trac.webkit.org/changeset/101810>
Comment 10 Hajime Morrita 2011-12-04 20:57:41 PST
*** Bug 53001 has been marked as a duplicate of this bug. ***