54308 2011-02-11 13:17:12 -0800 Always display the media controls when requiresFullscreenForVideoPlayback() is true 2011-02-21 05:56:59 -0800 1 1 1 Unclassified WebKit Media 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 max.hong.shen max.hong.shen ademar commit-queue eric.carlson jer.noble kling menard nancy.piedra vestbo oldest_to_newest 349964 0 max.hong.shen 2011-02-11 13:17:12 -0800 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. 349966 1 82165 max.hong.shen 2011-02-11 13:21:52 -0800 Created attachment 82165 first draft Please give me some input about this requirement & implementation. Thank you! 352768 2 menard 2011-02-17 05:17:51 -0800 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. 353030 3 max.hong.shen 2011-02-17 11:05:09 -0800 (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 :) 353141 4 ademar 2011-02-17 13:24:33 -0800 (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. 353146 5 max.hong.shen 2011-02-17 13:33:12 -0800 (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. 353540 6 82165 vestbo 2011-02-18 05:22:13 -0800 Comment on attachment 82165 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() 353594 7 82962 max.hong.shen 2011-02-18 08:27:17 -0800 Created attachment 82962 move the logical to HTMLMediaElement::controls() Thanks for suggestion and review :) 353609 8 82962 vestbo 2011-02-18 08:46:31 -0800 Comment on attachment 82962 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. 353669 9 82981 max.hong.shen 2011-02-18 10:48:03 -0800 Created attachment 82981 remove unnecessary test Let's get rid of that test at this moment :) 354034 10 82981 commit-queue 2011-02-18 19:58:53 -0800 Comment on attachment 82981 remove unnecessary test Clearing flags on attachment: 82981 Committed r79085: <http://trac.webkit.org/changeset/79085> 354035 11 commit-queue 2011-02-18 19:58:59 -0800 All reviewed patches have been landed. Closing bug. 354544 12 ademar 2011-02-21 05:56:35 -0800 Revision r79085 cherry-picked into qtwebkit-2.1.x with commit c2ed50b <http://gitorious.org/webkit/qtwebkit/commit/c2ed50b> 82165 2011-02-11 13:21:52 -0800 2011-02-18 08:27:17 -0800 first draft 54308.patch text/plain 1508 max.hong.shen SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDc4MzYwKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEtMDItMTEgIFlpIFNoZW4g IDx5aS40LnNoZW5Abm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIEFsd2F5cyBkaXNwbGF5IHRoZSBtZWRpYSBjb250cm9scyB3aGVuIHJl cXVpcmVzRnVsbHNjcmVlbkZvclZpZGVvUGxheWJhY2soKSBpcyB0cnVlCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01NDMwOAorCisgICAgICAgIE5vIG5l dyB0ZXN0cy4KKworICAgICAgICAqIGh0bWwvc2hhZG93L01lZGlhQ29udHJvbHMuY3BwOgorICAg ICAgICAoV2ViQ29yZTo6TWVkaWFDb250cm9sczo6dXBkYXRlKToKKwogMjAxMS0wMi0xMSAgQ2hy aXMgUm9nZXJzICA8Y3JvZ2Vyc0Bnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEtl bm5ldGggUnVzc2VsbC4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2h0bWwvc2hhZG93L01lZGlhQ29u dHJvbHMuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2h0bWwvc2hhZG93L01lZGlh Q29udHJvbHMuY3BwCShyZXZpc2lvbiA3ODM0OCkKKysrIFNvdXJjZS9XZWJDb3JlL2h0bWwvc2hh ZG93L01lZGlhQ29udHJvbHMuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yOSw2ICsyOSw3IEBACiAj aWYgRU5BQkxFKFZJREVPKQogI2luY2x1ZGUgIk1lZGlhQ29udHJvbHMuaCIKIAorI2luY2x1ZGUg IkNocm9tZS5oIgogI2luY2x1ZGUgIkV2ZW50TmFtZXMuaCIKICNpbmNsdWRlICJGbG9hdENvbnZl cnNpb24uaCIKICNpbmNsdWRlICJIVE1MTmFtZXMuaCIKQEAgLTEyMiw3ICsxMjMsNyBAQCB2b2lk IE1lZGlhQ29udHJvbHM6OmRlc3Ryb3koKQogdm9pZCBNZWRpYUNvbnRyb2xzOjp1cGRhdGUoKQog ewogICAgIEhUTUxNZWRpYUVsZW1lbnQqIG1lZGlhID0gbV9tZWRpYUVsZW1lbnQ7Ci0gICAgaWYg KCFtZWRpYS0+Y29udHJvbHMoKSB8fCAhbWVkaWEtPmluQWN0aXZlRG9jdW1lbnQoKSkgeworICAg IGlmICgoIW1lZGlhLT5jb250cm9scygpICYmICFtX21lZGlhRWxlbWVudC0+ZG9jdW1lbnQoKS0+ cGFnZSgpLT5jaHJvbWUoKS0+cmVxdWlyZXNGdWxsc2NyZWVuRm9yVmlkZW9QbGF5YmFjaygpKSB8 fCAhbWVkaWEtPmluQWN0aXZlRG9jdW1lbnQoKSkgewogICAgICAgICBpZiAobV9jb250cm9sc1No YWRvd1Jvb3QpIHsKICAgICAgICAgICAgIG1fY29udHJvbHNTaGFkb3dSb290LT5kZXRhY2goKTsK ICAgICAgICAgICAgIG1fcGFuZWwgPSAwOwo= 82962 2011-02-18 08:27:17 -0800 2011-02-18 10:48:03 -0800 move the logical to HTMLMediaElement::controls() 54308.patch text/plain 2412 max.hong.shen SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDc5MDAwKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTEtMDItMTggIFlpIFNoZW4g IDx5aS40LnNoZW5Abm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIEFsd2F5cyBkaXNwbGF5IHRoZSBtZWRpYSBjb250cm9scyB3aGVuIHJl cXVpcmVzRnVsbHNjcmVlbkZvclZpZGVvUGxheWJhY2soKSBpcyB0cnVlCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01NDMwOAorCisgICAgICAgIEZvciB2 aWRlbyBlbGVtZW50LCBpdCBzaG91bGQgaGF2ZSBjb250cm9scyB3aGVuIAorICAgICAgICBDaHJv bWU6OnJlcXVpcmVzRnVsbHNjcmVlbkZvclZpZGVvUGxheWJhY2soKSBpcyB0cnVlLgorCisgICAg ICAgIFRlc3Q6IG1hbnVhbC10ZXN0cy92aWRlby1jb250cm9scy13aGVuLWZ1bGxzY3JlZW5wbGF5 YmFjay1yZXF1aXJlZC5odG1sCisKKyAgICAgICAgKiBodG1sL0hUTUxNZWRpYUVsZW1lbnQuY3Bw OgorICAgICAgICAoV2ViQ29yZTo6SFRNTE1lZGlhRWxlbWVudDo6Y29udHJvbHMpOgorICAgICAg ICAqIG1hbnVhbC10ZXN0cy92aWRlby1jb250cm9scy13aGVuLWZ1bGxzY3JlZW5wbGF5YmFjay1y ZXF1aXJlZC5odG1sOiBBZGRlZC4KKwogMjAxMS0wMi0xOCAgQW5kcmV5IEFkYWlraW4gIDxhYW5k cmV5QGdvb2dsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgUGF2ZWwgRmVsZG1hbi4KSW5k ZXg6IFNvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTE1lZGlhRWxlbWVudC5jcHAKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MTWVkaWFFbGVtZW50LmNwcAkocmV2aXNpb24gNzg5 OTgpCisrKyBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuY3BwCSh3b3JraW5n IGNvcHkpCkBAIC0xNDcwLDYgKzE0NzAsMTAgQEAgYm9vbCBIVE1MTWVkaWFFbGVtZW50Ojpjb250 cm9scygpIGNvbnN0CiAgICAgaWYgKGZyYW1lICYmICFmcmFtZS0+c2NyaXB0KCktPmNhbkV4ZWN1 dGVTY3JpcHRzKE5vdEFib3V0VG9FeGVjdXRlU2NyaXB0KSkKICAgICAgICAgcmV0dXJuIHRydWU7 CiAKKyAgICAvLyBhbHdheXMgc2hvdyBjb250cm9scyBmb3IgdmlkZW8gd2hlbiBmdWxsc2NyZWVu IHBsYXliYWNrIGlzIHJlcXVpcmVkLgorICAgIGlmIChpc1ZpZGVvKCkgJiYgZG9jdW1lbnQoKS0+ cGFnZSgpICYmIGRvY3VtZW50KCktPnBhZ2UoKS0+Y2hyb21lKCktPnJlcXVpcmVzRnVsbHNjcmVl bkZvclZpZGVvUGxheWJhY2soKSkKKyAgICAgICAgcmV0dXJuIHRydWU7CisKICAgICByZXR1cm4g aGFzQXR0cmlidXRlKGNvbnRyb2xzQXR0cik7CiB9CiAKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL21h bnVhbC10ZXN0cy92aWRlby1jb250cm9scy13aGVuLWZ1bGxzY3JlZW5wbGF5YmFjay1yZXF1aXJl ZC5odG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL21hbnVhbC10ZXN0cy92aWRlby1j b250cm9scy13aGVuLWZ1bGxzY3JlZW5wbGF5YmFjay1yZXF1aXJlZC5odG1sCShyZXZpc2lvbiAw KQorKysgU291cmNlL1dlYkNvcmUvbWFudWFsLXRlc3RzL3ZpZGVvLWNvbnRyb2xzLXdoZW4tZnVs bHNjcmVlbnBsYXliYWNrLXJlcXVpcmVkLmh0bWwJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMTkg QEAKKzxodG1sPgorICAgIDxoZWFkPgorICAgIDwvaGVhZD4KKyAgICAKKzxib2R5PgorCisgICAg PHA+VEVTVDogVmlkZW8gc2hvdWxkIGhhdmUgY29udHJvbHMgd2hlbiBDaHJvbWU6OnJlcXVpcmVz RnVsbHNjcmVlbkZvclZpZGVvUGxheWJhY2soKSBpcyB0cnVlLjwvcD4KKworICAgIDx2aWRlbyAK KyAgICAgICAgc3JjPSJodHRwOi8vaW9wMS5ub2tpYS1ib3N0b24uY29tL2h0bWw1L3ZpZGVvL3Bo YXNlMS90aGUtbmluamEtY2F0Lm1wNCI+CisgICAgPC92aWRlbz4KKworICAgIDxwPlRFU1Q6IEF1 ZGlvIHNob3VsZCBoYXZlIE5PIGNvbnRyb2xzIGlmIHRoZSBjb250cm9scyBhdHRyaWJ1dGUgaXMg bm90IHNwZWNpZmllZC48L3A+CisgICAgPGF1ZGlvIAorICAgICAgICBzcmM9Imh0dHA6Ly9pb3Ax Lm5va2lhLWJvc3Rvbi5jb20vaHRtbDUvYXVkaW8vdGVzdC5tcDMiPgorICAgIDwvYXVkaW8+CisK KzwvYm9keT4KKzwvaHRtbD4K 82981 2011-02-18 10:48:03 -0800 2011-02-18 19:58:53 -0800 remove unnecessary test 54308.patch text/plain 1402 max.hong.shen SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDc5MDExKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTEtMDItMTggIFlpIFNoZW4g IDx5aS40LnNoZW5Abm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIEFsd2F5cyBkaXNwbGF5IHRoZSBtZWRpYSBjb250cm9scyB3aGVuIHJl cXVpcmVzRnVsbHNjcmVlbkZvclZpZGVvUGxheWJhY2soKSBpcyB0cnVlCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01NDMwOAorCisgICAgICAgIEZvciB2 aWRlbyBlbGVtZW50LCBpdCBzaG91bGQgaGF2ZSBjb250cm9scyB3aGVuIAorICAgICAgICBDaHJv bWU6OnJlcXVpcmVzRnVsbHNjcmVlbkZvclZpZGVvUGxheWJhY2soKSBpcyB0cnVlLgorCisgICAg ICAgICogaHRtbC9IVE1MTWVkaWFFbGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkhUTUxN ZWRpYUVsZW1lbnQ6OmNvbnRyb2xzKToKKwogMjAxMS0wMi0xOCAgQmVuIFZhbmlrICA8YmVudmFu aWtAZ29vZ2xlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBLZW5uZXRoIFJ1c3NlbGwuCklu ZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTE1lZGlhRWxlbWVudC5jcHAJKHJldmlzaW9uIDc4 OTk4KQorKysgU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MTWVkaWFFbGVtZW50LmNwcAkod29ya2lu ZyBjb3B5KQpAQCAtMTQ3MCw2ICsxNDcwLDEwIEBAIGJvb2wgSFRNTE1lZGlhRWxlbWVudDo6Y29u dHJvbHMoKSBjb25zdAogICAgIGlmIChmcmFtZSAmJiAhZnJhbWUtPnNjcmlwdCgpLT5jYW5FeGVj dXRlU2NyaXB0cyhOb3RBYm91dFRvRXhlY3V0ZVNjcmlwdCkpCiAgICAgICAgIHJldHVybiB0cnVl OwogCisgICAgLy8gYWx3YXlzIHNob3cgY29udHJvbHMgZm9yIHZpZGVvIHdoZW4gZnVsbHNjcmVl biBwbGF5YmFjayBpcyByZXF1aXJlZC4KKyAgICBpZiAoaXNWaWRlbygpICYmIGRvY3VtZW50KCkt PnBhZ2UoKSAmJiBkb2N1bWVudCgpLT5wYWdlKCktPmNocm9tZSgpLT5yZXF1aXJlc0Z1bGxzY3Jl ZW5Gb3JWaWRlb1BsYXliYWNrKCkpCisgICAgICAgIHJldHVybiB0cnVlOworCiAgICAgcmV0dXJu IGhhc0F0dHJpYnV0ZShjb250cm9sc0F0dHIpOwogfQogCg==