Bug 54308 - Always display the media controls when requiresFullscreenForVideoPlayback() is true
Summary: Always display the media controls when requiresFullscreenForVideoPlayback() i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yi Shen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-11 13:17 PST by Yi Shen
Modified: 2011-02-21 05:56 PST (History)
8 users (show)

See Also:


Attachments
first draft (1.47 KB, patch)
2011-02-11 13:21 PST, Yi Shen
vestbo: review-
Details | Formatted Diff | Diff
move the logical to HTMLMediaElement::controls() (2.36 KB, patch)
2011-02-18 08:27 PST, Yi Shen
no flags Details | Formatted Diff | Diff
remove unnecessary test (1.37 KB, patch)
2011-02-18 10:48 PST, Yi Shen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yi Shen 2011-02-11 13:17:12 PST
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.
Comment 1 Yi Shen 2011-02-11 13:21:52 PST
Created attachment 82165 [details]
first draft

Please give me some input about this requirement & implementation. Thank you!
Comment 2 Alexis Menard (darktears) 2011-02-17 05:17:51 PST
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.
Comment 3 Yi Shen 2011-02-17 11:05:09 PST
(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 :)
Comment 4 Ademar Reis 2011-02-17 13:24:33 PST
(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.
Comment 5 Yi Shen 2011-02-17 13:33:12 PST
(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.
Comment 6 Tor Arne Vestbø 2011-02-18 05:22:13 PST
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()
Comment 7 Yi Shen 2011-02-18 08:27:17 PST
Created attachment 82962 [details]
move the logical to HTMLMediaElement::controls()

Thanks for suggestion and review :)
Comment 8 Tor Arne Vestbø 2011-02-18 08:46:31 PST
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.
Comment 9 Yi Shen 2011-02-18 10:48:03 PST
Created attachment 82981 [details]
remove unnecessary test

Let's get rid of that test at this moment :)
Comment 10 WebKit Commit Bot 2011-02-18 19:58:53 PST
Comment on attachment 82981 [details]
remove unnecessary test

Clearing flags on attachment: 82981

Committed r79085: <http://trac.webkit.org/changeset/79085>
Comment 11 WebKit Commit Bot 2011-02-18 19:58:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Ademar Reis 2011-02-21 05:56:35 PST
Revision r79085 cherry-picked into qtwebkit-2.1.x with commit c2ed50b <http://gitorious.org/webkit/qtwebkit/commit/c2ed50b>