[webkit-reviews] review granted: [Bug 56851] Media controls must use full screen style when in new full screen mode. : [Attachment 87017] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 56851: Media controls must use full screen style when in new full screen
mode.
https://bugs.webkit.org/show_bug.cgi?id=56851

Attachment 87017: Patch
https://bugs.webkit.org/attachment.cgi?id=87017&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87017&action=review

Nice, this is *much* cleaner than the earlier version!

> Source/WebCore/html/shadow/MediaControls.cpp:150
> +    m_fullScreenMinVolumeButton =
MediaControlFullscreenVolumeMinButtonElement::create(mediaElement);
> +    m_fullScreenMinVolumeButton->attachToParent(m_panel.get());
> +    
> +    m_fullScreenVolumeSlider =
MediaControlFullscreenVolumeSliderElement::create(mediaElement);
> +    m_fullScreenVolumeSlider->attachToParent(m_panel.get());
> +    
> +    m_fullScreenMaxVolumeButton =
MediaControlFullscreenVolumeMaxButtonElement::create(mediaElement);
> +    m_fullScreenMaxVolumeButton->attachToParent(m_panel.get());
> +    

It would be nice if it was possible to create these controls only when the
element goes into fullscreen, maybe via an explicit call from the element? I
think it can be done in a follow-up patch, but it would be a nice optimization
because most <video> elements will never go fullscreen, and *no* <audio>
element ever will. It might be good to have a FIXME here with a bug number for
the enhancement so we don't forget to do it.