Bug 54308 – Always display the media controls when requiresFullscreenForVideoPlayback() is true

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

[Yi Shen]

As a requirement, we try to always display the play button when chrome::requiresFullscreenForVideoPlayback() returns true, even the controls is NOT specified in video tag. One reason for this desire is that, for a mobile device which only can play html5 video in full screen mode (NO inline-mode at all), it would be convenience to always show the play button, which can be used to start playing the video.

For inline mode, it can work without controls to start playing video. For e.g. we can use autoplay in the video tag to start playing. However, for fullscreen only mode, we can't just automatically go into full screen mode when the page loads.

[Yi Shen]

Created attachment 82165 [details]
first draft

Please give me some input about this requirement & implementation. Thank you!

[Alexis Menard (darktears)]

Can it be catched before the MediaControl code? Like in HTMLMediaElement and HTMLVideoElement when those guys calls void RenderMedia::updateFromElement() which then call update on the controls.

[Yi Shen]

(In reply to comment #2)
> Can it be catched before the MediaControl code? Like in HTMLMediaElement and HTMLVideoElement when those guys calls void RenderMedia::updateFromElement() which then call update on the controls.

Thanks for your input, Alexis. I think the main concern for this patch is the requirement self, which may break the html5 spec -- even the control is not specified, the media control is still visible when requiresFullscreenForVideoPlayback() is true. 

If people think this is acceptable, I will rework on the implementation based on your suggestion. Please let me know your thought. Thanks :)

[Ademar Reis]

(In reply to comment #3)
> (In reply to comment #2)
> > Can it be catched before the MediaControl code? Like in HTMLMediaElement and HTMLVideoElement when those guys calls void RenderMedia::updateFromElement() which then call update on the controls.
> 
> Thanks for your input, Alexis. I think the main concern for this patch is the requirement self, which may break the html5 spec -- even the control is not specified, the media control is still visible when requiresFullscreenForVideoPlayback() is true. 
> 
> If people think this is acceptable, I will rework on the implementation based on your suggestion. Please let me know your thought. Thanks :)

Yi, please prepare the patch using this new approach, just in case. We need it for this weekly build, even if it's just for internal testing.

[Yi Shen]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Can it be catched before the MediaControl code? Like in HTMLMediaElement and HTMLVideoElement when those guys calls void RenderMedia::updateFromElement() which then call update on the controls.
> > 
> > Thanks for your input, Alexis. I think the main concern for this patch is the requirement self, which may break the html5 spec -- even the control is not specified, the media control is still visible when requiresFullscreenForVideoPlayback() is true. 
> > 
> > If people think this is acceptable, I will rework on the implementation based on your suggestion. Please let me know your thought. Thanks :)
> 
> Yi, please prepare the patch using this new approach, just in case. We need it for this weekly build, even if it's just for internal testing.

No problem, I will work on it.

[Tor Arne Vestbø]

Comment on attachment 82165 [details]
first draft

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

> Source/WebCore/html/shadow/MediaControls.cpp:126
> +    if ((!media->controls() && !m_mediaElement->document()->page()->chrome()->requiresFullscreenForVideoPlayback()) || !media->inActiveDocument()) {

This logic should be in HTMLMediaElement::controls()

[Yi Shen]

Created attachment 82962 [details]
move the logical to HTMLMediaElement::controls()

Thanks for suggestion and review :)

[Tor Arne Vestbø]

Comment on attachment 82962 [details]
move the logical to HTMLMediaElement::controls()

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

> Source/WebCore/manual-tests/video-controls-when-fullscreenplayback-required.html:19
> +<html>
> +    <head>
> +    </head>
> +    
> +<body>
> +
> +    <p>TEST: Video should have controls when Chrome::requiresFullscreenForVideoPlayback() is true.</p>
> +
> +    <video 
> +        src="http://iop1.nokia-boston.com/html5/video/phase1/the-ninja-cat.mp4">
> +    </video>
> +
> +    <p>TEST: Audio should have NO controls if the controls attribute is not specified.</p>
> +    <audio 
> +        src="http://iop1.nokia-boston.com/html5/audio/test.mp3">
> +    </audio>
> +
> +</body>
> +</html>

I don't think this test gives us much, you could do the same with just a simple data:text/html,<video><audio>, you don't need a source. If you want to make a test for this it should include DRT plumbing to enable requiresFullscreenForVideoPlayback and then make a normal test out of it.

[Yi Shen]

Created attachment 82981 [details]
remove unnecessary test

Let's get rid of that test at this moment :)

[WebKit Commit Bot]

Comment on attachment 82981 [details]
remove unnecessary test

Clearing flags on attachment: 82981

Committed r79085: <http://trac.webkit.org/changeset/79085>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r79085 cherry-picked into qtwebkit-2.1.x with commit c2ed50b <http://gitorious.org/webkit/qtwebkit/commit/c2ed50b>