Summary: | Always display the media controls when requiresFullscreenForVideoPlayback() is true | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yi Shen <max.hong.shen> | ||||||||
Component: | Media | Assignee: | Yi Shen <max.hong.shen> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ademar, commit-queue, eric.carlson, jer.noble, kling, menard, nancy.piedra, vestbo | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Yi Shen
2011-02-11 13:17:12 PST
Created attachment 82165 [details]
first draft
Please give me some input about this requirement & implementation. Thank you!
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. (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 :) (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. (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 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() Created attachment 82962 [details]
move the logical to HTMLMediaElement::controls()
Thanks for suggestion and review :)
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. Created attachment 82981 [details]
remove unnecessary test
Let's get rid of that test at this moment :)
Comment on attachment 82981 [details] remove unnecessary test Clearing flags on attachment: 82981 Committed r79085: <http://trac.webkit.org/changeset/79085> All reviewed patches have been landed. Closing bug. Revision r79085 cherry-picked into qtwebkit-2.1.x with commit c2ed50b <http://gitorious.org/webkit/qtwebkit/commit/c2ed50b> |