Bug 89344

Created attachment 148210 [details] patch for discussion

The attached patch seem very invasive just to query the width of the video element and remove elements from the controls panel. Also, I have found that updating the padding and height of the panel element doesn't work. I need a better plan of attack... any ideas?

Comment on attachment 148210 [details] patch for discussion Attachment 148210 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12988088 New failing tests: media/media-document-audio-repaint.html fullscreen/full-screen-stacking-context.html media/video-no-audio.html media/controls-strict.html media/controls-styling.html media/video-display-toggle.html media/audio-repaint.html media/audio-controls-rendering.html media/video-zoom-controls.html http/tests/media/video-buffered-range-contains-currentTime.html media/video-controls-rendering.html media/controls-without-preload.html media/media-controls-clone.html fast/layers/video-layer.html media/video-empty-source.html media/video-playing-and-pause.html media/controls-layout-direction.html media/controls-after-reload.html

Created attachment 148331 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 148210 [details] patch for discussion View in context: https://bugs.webkit.org/attachment.cgi?id=148210&action=review > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:303 > + if (!mediaElement || !mediaElement->renderer() || !mediaElement->renderer()->isVideo()) > + return false; You don't want to render controls for an <audio> element? > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:341 > + fprintf(stderr, "controlsWidth=%d, padding=%d, height=%d\n", controlsWidth, padding, 30 + padding); Oops!

Created attachment 160628 [details] Patch

(In reply to comment #5) > (From update of attachment 148210 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148210&action=review > > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:303 > > + if (!mediaElement || !mediaElement->renderer() || !mediaElement->renderer()->isVideo()) > > + return false; > > You don't want to render controls for an <audio> element? > > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:341 > > + fprintf(stderr, "controlsWidth=%d, padding=%d, height=%d\n", controlsWidth, padding, 30 + padding); > > Oops! The new patch is a complete work-over. Please re-review.

Comment on attachment 160628 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160628&action=review > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:91 > + if (mediaWidth < 350) Unnamed magic values are bad. Named constants would be an improvement, but methods on a shared base class would be even better because controlling the visibility of control elements based on the media element width is not a new idea (see RenderMediaControlTimeDisplay::layout). Oh wait - there still isn't a shared base class, is there? > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:96 > + if (mediaWidth < 275) Ditto. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:101 > + if (mediaWidth < 210) Ditto. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:106 > + if (mediaWidth < 150) Ditto. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:111 > + if (mediaWidth < 100) Ditto. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:116 > + // Shrink padding between 400px to 160px video width from 5px to 0px. This comment doesn't appear to have anything to do with this code. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:148 > + float padding = 5; Another unnamed magic value. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:149 > + if (mediaWidth <= 160) Ditto. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:151 > + else if (mediaWidth <= 220) Ditto. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:153 > + else if (mediaWidth <= 280) Ditto. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:155 > + else if (mediaWidth <= 340) Ditto. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:157 > + else if (mediaWidth <= 400) Ditto. > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:121 > + void hideVolumeSlider(); > + void showTimeDisplay(); > + void hideTimeDisplay(); > + void showMuteButton(); > + void hideMuteButton(); > + void showFullscreenButton(); > + void hideFullscreenButton(); > + void showTimeline(); > + void hideTimeline(); Event more differentiation between MediaControlRootElementChromium and MediaControlRootElement? > LayoutTests/ChangeLog:23 > + * media/controls-audio-sizes-expected.txt: Added. > + * media/controls-audio-sizes.html: Added. > + Tests if the correct elements are shown for audio elements. > + * media/controls-video-sizes-expected.txt: Added. > + * media/controls-video-sizes.html: Added. > + Tests if the correct elements are shown for video elements. > + * media/controls-video-sizes-padding-expected.txt: Added. > + * media/controls-video-sizes-padding.html: Added. > + Tests if the correct padding is applied to the enclosure of the video controls. These results will fail on all but the Chromium ports. > LayoutTests/media/controls-audio-sizes.html:49 > + function init() { A function's opening brace goes on a new line. > LayoutTests/media/controls-audio-sizes.html:69 > + <p>Loose time display at 349px:</p> > + <audio controls src="content/short.wav" style="width: 350px;"></audio> > + <audio controls src="content/short.wav" style="width: 349px;"></audio> > + <p>Loose volume slider at 274px:</p> > + <audio src="content/short.wav" controls style="width: 275px;"></audio> > + <audio src="content/short.wav" controls style="width: 274px;"></audio> > + <p>Loose mute button at 209px:</p> > + <audio src="content/short.wav" controls style="width: 210px;"></audio> > + <audio src="content/short.wav" controls style="width: 209px;"></audio> > + <p>Loose transport bar at 99px:</p> Typo: "Loose" -> "Lose" > LayoutTests/media/controls-video-sizes-padding.html:9 > + function test(event, video, padding) { A function's opening brace goes on a new line. > LayoutTests/media/controls-video-sizes-padding.html:19 > + function init() { Ditto. > LayoutTests/media/controls-video-sizes-padding.html:22 > + var paddings = [5, 4, 4, 3, 3, 2, 2, 1, 1, 0]; Nit: you have extra spaces between some values. > LayoutTests/media/controls-video-sizes.html:31 > + if (width >= 350) { > + testExpected("getComputedStyle(remainingtime)['display']", 'block', "=="); > + } else { > + testExpected("getComputedStyle(remainingtime)['display']", 'none', "=="); > + } Nit: braces are not needed for single line conditionals.

(In reply to comment #8) > (From update of attachment 160628 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160628&action=review > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:91 > > + if (mediaWidth < 350) > > Unnamed magic values are bad. Named constants would be an improvement, DONE. > but methods on a shared base class would be even better because controlling the visibility of control elements based on the media element width is not a new idea (see RenderMediaControlTimeDisplay::layout). > > Oh wait - there still isn't a shared base class, is there? I know that bug is still open and I will to address it. I am first trying to come to terms with all the features we need for the Chromium controls, many of which are not in the WebKit controls. It seems easier to do this separately first. Is this the wrong approach? > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:96 > > + if (mediaWidth < 275) > > Ditto. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:101 > > + if (mediaWidth < 210) > > Ditto. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:106 > > + if (mediaWidth < 150) > > Ditto. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:111 > > + if (mediaWidth < 100) > > Ditto. DONE all of these. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:116 > > + // Shrink padding between 400px to 160px video width from 5px to 0px. > > This comment doesn't appear to have anything to do with this code. DONE. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:148 > > + float padding = 5; > > Another unnamed magic value. > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:149 > > + if (mediaWidth <= 160) > > Ditto. > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:151 > > + else if (mediaWidth <= 220) > > Ditto. > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:153 > > + else if (mediaWidth <= 280) > > Ditto. > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:155 > > + else if (mediaWidth <= 340) > > Ditto. > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:157 > > + else if (mediaWidth <= 400) > > Ditto. DONE for all of these. I've taken an algorithmic approach instead which needs less magic values. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:121 > > + void hideVolumeSlider(); > > + void showTimeDisplay(); > > + void hideTimeDisplay(); > > + void showMuteButton(); > > + void hideMuteButton(); > > + void showFullscreenButton(); > > + void hideFullscreenButton(); > > + void showTimeline(); > > + void hideTimeline(); > > Event more differentiation between MediaControlRootElementChromium and MediaControlRootElement? I was going to ask you when creating a common base class what to do with these. Do you think they will be generally useful? > > LayoutTests/ChangeLog:23 > > + * media/controls-audio-sizes-expected.txt: Added. > > + * media/controls-audio-sizes.html: Added. > > + Tests if the correct elements are shown for audio elements. > > + * media/controls-video-sizes-expected.txt: Added. > > + * media/controls-video-sizes.html: Added. > > + Tests if the correct elements are shown for video elements. > > + * media/controls-video-sizes-padding-expected.txt: Added. > > + * media/controls-video-sizes-padding.html: Added. > > + Tests if the correct padding is applied to the enclosure of the video controls. > > These results will fail on all but the Chromium ports. Oops, you're right. I could always just do image tests I guess, but I wanted to avoid that. Is there a better solution? > > LayoutTests/media/controls-audio-sizes.html:49 > > + function init() { > > A function's opening brace goes on a new line. DONE. > > LayoutTests/media/controls-audio-sizes.html:69 > > + <p>Loose time display at 349px:</p> > > + <audio controls src="content/short.wav" style="width: 350px;"></audio> > > + <audio controls src="content/short.wav" style="width: 349px;"></audio> > > + <p>Loose volume slider at 274px:</p> > > + <audio src="content/short.wav" controls style="width: 275px;"></audio> > > + <audio src="content/short.wav" controls style="width: 274px;"></audio> > > + <p>Loose mute button at 209px:</p> > > + <audio src="content/short.wav" controls style="width: 210px;"></audio> > > + <audio src="content/short.wav" controls style="width: 209px;"></audio> > > + <p>Loose transport bar at 99px:</p> > > Typo: "Loose" -> "Lose" DONE. > > LayoutTests/media/controls-video-sizes-padding.html:9 > > + function test(event, video, padding) { > > A function's opening brace goes on a new line. DONE. > > LayoutTests/media/controls-video-sizes-padding.html:19 > > + function init() { > > Ditto. > > > LayoutTests/media/controls-video-sizes-padding.html:22 > > + var paddings = [5, 4, 4, 3, 3, 2, 2, 1, 1, 0]; > > Nit: you have extra spaces between some values. > > > LayoutTests/media/controls-video-sizes.html:31 > > + if (width >= 350) { > > + testExpected("getComputedStyle(remainingtime)['display']", 'block', "=="); > > + } else { > > + testExpected("getComputedStyle(remainingtime)['display']", 'none', "=="); > > + } > > Nit: braces are not needed for single line conditionals. Thanks!

Created attachment 160933 [details] feedback included

(In reply to comment #9) > > I know that bug is still open and I will to address it. I am first trying to come to terms with all the features we need for the Chromium controls, many of which are not in the WebKit controls. > It seems easier to do this separately first. Is this the wrong approach? > I don't know that it is easier to make this change first - every additional change to MediaControlRootElementChromium makes the eventual refactoring bigger. I mentioned it in this patch because this general feature *is* one is shared by all ports (see RenderMediaControlTimeDisplay::layout). > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:121 > > > + void hideVolumeSlider(); > > > + void showTimeDisplay(); > > > + void hideTimeDisplay(); > > > + void showMuteButton(); > > > + void hideMuteButton(); > > > + void showFullscreenButton(); > > > + void hideFullscreenButton(); > > > + void showTimeline(); > > > + void hideTimeline(); > > > > Event more differentiation between MediaControlRootElementChromium and MediaControlRootElement? > > I was going to ask you when creating a common base class what to do with these. > Do you think they will be generally useful? > I think so, other ports already adjust the visibility of control elements based on width. > > > LayoutTests/ChangeLog:23 > > > + * media/controls-audio-sizes-expected.txt: Added. > > > + * media/controls-audio-sizes.html: Added. > > > + Tests if the correct elements are shown for audio elements. > > > + * media/controls-video-sizes-expected.txt: Added. > > > + * media/controls-video-sizes.html: Added. > > > + Tests if the correct elements are shown for video elements. > > > + * media/controls-video-sizes-padding-expected.txt: Added. > > > + * media/controls-video-sizes-padding.html: Added. > > > + Tests if the correct padding is applied to the enclosure of the video controls. > > > > These results will fail on all but the Chromium ports. > > Oops, you're right. I could always just do image tests I guess, but I wanted to avoid that. > Is there a better solution? > Put the results in LayoutTests/platform/chromium and skip the tests on all other platforms.

Comment on attachment 160933 [details] feedback included View in context: https://bugs.webkit.org/attachment.cgi?id=160933&action=review r- because this will still break all non-chromium ports. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:72 > + virtual void layout(); Please add OVERRIDE. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:154 > +static const int videoControlsHeight = 30; > +static const int maxPadding = 5; > +static const int minPadding = 0; > +static const int minPaddingAtWidth = 160; > +static const int decreaseStep = 60; I assume some of these are the same values you use in the media controls CSS? If so, it would be good to have a comment to remind people that they must stay in sync. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:162 > + if (padding > maxPadding) Nit: padding can't be both < minPadding and > maxPadding so you could have an "else" here. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:399 > + if (!m_hiddenTimeDisplay && now > 0) { Do you really need "m_hiddenTimeDisplay"? Would it not work to check the visibility of m_durationDisplay? > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:566 > + if (m_mediaController->hasAudio()) > + m_panelMuteButton->show(); Nit: the other methods in this class use early return. You should use one style of the other throughout the class. > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:76 > + virtual RenderObject* createRenderer(RenderArena*, RenderStyle*); Please add OVERRIDE. > LayoutTests/media/controls-audio-sizes.html:16 > + for (var i = 0; i < audios.length; i++) { > + var audio = audios[i]; > + audio.addEventListener('loadedmetadata', function (evt) { /* just catch it */ }, true); > + } Why is this necessary? > LayoutTests/media/controls-video-sizes-padding.html:26 > + setSrcByTagName('video', findMediaFile('video', 'content/test')); > + var videos = document.getElementsByTagName("video"); > + for (var i = 0; i < videos.length; i++) { > + var video = videos[i]; > + video.addEventListener('loadedmetadata', test(event, video), false); > + } Nit: setSrcByTagName just loops over the elements and sets the src attribute. You immediately loop over the same elements to add event listeners so you might as well do both in the same loop. > LayoutTests/media/controls-video-sizes.html:16 > + var videos = document.getElementsByTagName("video"); > + for (var i = 0; i < videos.length; i++) { > + var video = videos[i]; > + video.addEventListener('loadedmetadata', function (evt) { /* just catch it */ }, true); > + } What does this do?

(In reply to comment #11) > (In reply to comment #9) > > > > I know that bug is still open and I will to address it. I am first trying to come to terms with all the features we need for the Chromium controls, many of which are not in the WebKit controls. > > It seems easier to do this separately first. Is this the wrong approach? > > > I don't know that it is easier to make this change first - every additional change to MediaControlRootElementChromium makes the eventual refactoring bigger. I mentioned it in this patch because this general feature *is* one is shared by all ports (see RenderMediaControlTimeDisplay::layout). So that's in MediaControlElements.cpp. The refactoring that I'm looking at is about merging/subclassing MediaControlRootElement.cpp and MediaControlRootElementChromium.cpp - I wasn't going to touch MediaControlElements.cpp. Are you suggesting to merge all of these? That would indeed become a much bigger issue. > > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:121 > > > > + void hideVolumeSlider(); > > > > + void showTimeDisplay(); > > > > + void hideTimeDisplay(); > > > > + void showMuteButton(); > > > > + void hideMuteButton(); > > > > + void showFullscreenButton(); > > > > + void hideFullscreenButton(); > > > > + void showTimeline(); > > > > + void hideTimeline(); > > > > > > Event more differentiation between MediaControlRootElementChromium and MediaControlRootElement? > > > > I was going to ask you when creating a common base class what to do with these. > > Do you think they will be generally useful? > > > I think so, other ports already adjust the visibility of control elements based on width. There's none of this in any of the MediaControlRootElement ports, so it's Chrome desktop specific for now. I'm happy to move it to a base class, though, considering it may become useful to all the ports. I just don't know whether thats best practice. > > > > LayoutTests/ChangeLog:23 > > > > + * media/controls-audio-sizes-expected.txt: Added. > > > > + * media/controls-audio-sizes.html: Added. > > > > + Tests if the correct elements are shown for audio elements. > > > > + * media/controls-video-sizes-expected.txt: Added. > > > > + * media/controls-video-sizes.html: Added. > > > > + Tests if the correct elements are shown for video elements. > > > > + * media/controls-video-sizes-padding-expected.txt: Added. > > > > + * media/controls-video-sizes-padding.html: Added. > > > > + Tests if the correct padding is applied to the enclosure of the video controls. > > > > > > These results will fail on all but the Chromium ports. > > > > Oops, you're right. I could always just do image tests I guess, but I wanted to avoid that. > > Is there a better solution? > > > Put the results in LayoutTests/platform/chromium and skip the tests on all other platforms. Thanks, will do.

Thanks, Eric! (In reply to comment #12) > (From update of attachment 160933 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160933&action=review > > r- because this will still break all non-chromium ports. Added skips to the TestExpectations files of the other platforms. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:72 > > + virtual void layout(); > > Please add OVERRIDE. Done. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:154 > > +static const int videoControlsHeight = 30; > > +static const int maxPadding = 5; > > +static const int minPadding = 0; > > +static const int minPaddingAtWidth = 160; > > +static const int decreaseStep = 60; > > I assume some of these are the same values you use in the media controls CSS? If so, it would be good to have a comment to remind people that they must stay in sync. Done, good idea. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:162 > > + if (padding > maxPadding) > > Nit: padding can't be both < minPadding and > maxPadding so you could have an "else" here. Done. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:399 > > + if (!m_hiddenTimeDisplay && now > 0) { > > Do you really need "m_hiddenTimeDisplay"? Would it not work to check the visibility of m_durationDisplay? m_durationDisplay is swapped out for m_currentTimeDisplay in Chromium after playback starts - only one of these two is visible at any one point in time. I need a different means to keep track of whether the time display area should be shown at all. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:566 > > + if (m_mediaController->hasAudio()) > > + m_panelMuteButton->show(); > > Nit: the other methods in this class use early return. You should use one style of the other throughout the class. Done. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:76 > > + virtual RenderObject* createRenderer(RenderArena*, RenderStyle*); > > Please add OVERRIDE. Done. > > LayoutTests/media/controls-audio-sizes.html:16 > > + for (var i = 0; i < audios.length; i++) { > > + var audio = audios[i]; > > + audio.addEventListener('loadedmetadata', function (evt) { /* just catch it */ }, true); > > + } > > Why is this necessary? You're right, it's dumb - it's just providing time to load the audio elements. I've gone back to text only tests and this time for Chromium only. > > LayoutTests/media/controls-video-sizes-padding.html:26 > > + setSrcByTagName('video', findMediaFile('video', 'content/test')); > > + var videos = document.getElementsByTagName("video"); > > + for (var i = 0; i < videos.length; i++) { > > + var video = videos[i]; > > + video.addEventListener('loadedmetadata', test(event, video), false); > > + } > > Nit: setSrcByTagName just loops over the elements and sets the src attribute. You immediately loop over the same elements to add event listeners so you might as well do both in the same loop. Done. Good catch. > > LayoutTests/media/controls-video-sizes.html:16 > > + var videos = document.getElementsByTagName("video"); > > + for (var i = 0; i < videos.length; i++) { > > + var video = videos[i]; > > + video.addEventListener('loadedmetadata', function (evt) { /* just catch it */ }, true); > > + } > > What does this do? See above. Here, too, I have gone back to the previous text only test.

(In reply to comment #13) > (In reply to comment #11) > > (In reply to comment #9) > > > > > > I know that bug is still open and I will to address it. I am first trying to come to terms with all the features we need for the Chromium controls, many of which are not in the WebKit controls. > > > It seems easier to do this separately first. Is this the wrong approach? > > > > > I don't know that it is easier to make this change first - every additional change to MediaControlRootElementChromium makes the eventual refactoring bigger. I mentioned it in this patch because this general feature *is* one is shared by all ports (see RenderMediaControlTimeDisplay::layout). > > > So that's in MediaControlElements.cpp. The refactoring that I'm looking at is about merging/subclassing MediaControlRootElement.cpp and MediaControlRootElementChromium.cpp - I wasn't going to touch MediaControlElements.cpp. Are you suggesting to merge all of these? That would indeed become a much bigger issue. > Please don't be hyperbolic, refactoring MediaControlRootElement.cpp and MediaControlRootElementChromium.cpp is exactly what I have been talking about too. I mentioned RenderMediaControlTimeDisplay::layout because this patch is adding similar logic to the Chromium port, but in a completely different part of the code. If the refactoring had already been done I would have suggested consolidating the two approaches into shared code. > > > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:121 > > > > > + void hideVolumeSlider(); > > > > > + void showTimeDisplay(); > > > > > + void hideTimeDisplay(); > > > > > + void showMuteButton(); > > > > > + void hideMuteButton(); > > > > > + void showFullscreenButton(); > > > > > + void hideFullscreenButton(); > > > > > + void showTimeline(); > > > > > + void hideTimeline(); > > > > > > > > Event more differentiation between MediaControlRootElementChromium and MediaControlRootElement? > > > > > > I was going to ask you when creating a common base class what to do with these. > > > Do you think they will be generally useful? > > > > > I think so, other ports already adjust the visibility of control elements based on width. > > There's none of this in any of the MediaControlRootElement ports, so it's Chrome desktop specific for now. I'm happy to move it to a base class, though, considering it may become useful to all the ports. I just don't know whether thats best practice. > Best practice is to reduce unnecessary code duplication. Anything that is done by multiple ports should generally be done in shared code when possible/practical.

Created attachment 161440 [details] rewrote tests

(In reply to comment #15) > (In reply to comment #13) > > (In reply to comment #11) > > > (In reply to comment #9) > > > > > > > > I know that bug is still open and I will to address it. I am first trying to come to terms with all the features we need for the Chromium controls, many of which are not in the WebKit controls. > > > > It seems easier to do this separately first. Is this the wrong approach? > > > > > > > I don't know that it is easier to make this change first - every additional change to MediaControlRootElementChromium makes the eventual refactoring bigger. I mentioned it in this patch because this general feature *is* one is shared by all ports (see RenderMediaControlTimeDisplay::layout). > > > > > > So that's in MediaControlElements.cpp. The refactoring that I'm looking at is about merging/subclassing MediaControlRootElement.cpp and MediaControlRootElementChromium.cpp - I wasn't going to touch MediaControlElements.cpp. Are you suggesting to merge all of these? That would indeed become a much bigger issue. > > > > Please don't be hyperbolic, refactoring MediaControlRootElement.cpp and MediaControlRootElementChromium.cpp is exactly what I have been talking about too. I mentioned RenderMediaControlTimeDisplay::layout because this patch is adding similar logic to the Chromium port, but in a completely different part of the code. If the refactoring had already been done I would have suggested consolidating the two approaches into shared code. OK, I will look at that then too. > > > > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:121 > > > > > > + void hideVolumeSlider(); > > > > > > + void showTimeDisplay(); > > > > > > + void hideTimeDisplay(); > > > > > > + void showMuteButton(); > > > > > > + void hideMuteButton(); > > > > > > + void showFullscreenButton(); > > > > > > + void hideFullscreenButton(); > > > > > > + void showTimeline(); > > > > > > + void hideTimeline(); > > > > > > > > > > Event more differentiation between MediaControlRootElementChromium and MediaControlRootElement? > > > > > > > > I was going to ask you when creating a common base class what to do with these. > > > > Do you think they will be generally useful? > > > > > > > I think so, other ports already adjust the visibility of control elements based on width. > > > > There's none of this in any of the MediaControlRootElement ports, so it's Chrome desktop specific for now. I'm happy to move it to a base class, though, considering it may become useful to all the ports. I just don't know whether thats best practice. > > > > Best practice is to reduce unnecessary code duplication. Anything that is done by multiple ports should generally be done in shared code when possible/practical. OK, I'll take this discussion to the other bug. :-) Thanks. And sorry this new patch took so long - I didn't want to create a flaky test, so had to move back to image test for the videos, because there are too many of them.

Comment on attachment 161440 [details] rewrote tests View in context: https://bugs.webkit.org/attachment.cgi?id=161440&action=review > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:123 > + // Shrink padding between 400px to 160px video width from 5px to 0px. Nit: this comment does not describe this code. > LayoutTests/platform/chromium/media/controls-video-sizes.html:31 > + for (var i = 0; i < videos.length; i++) { > + videos[i].src = "../../../media/content/test.ogv"; > + videos[i].addEventListener('canplaythrough', function () { > + ready += 1; > + }, false); > + } > + waitForAllVideosReady(); Nit: a more efficient way to do this would be to call "endTest()" from the event listener once ready == videos.length instead of using a timer to watch for the transition.

Comment on attachment 161440 [details] rewrote tests Attachment 161440 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13682821 New failing tests: platform/chromium/media/controls-video-sizes.html compositing/video/video-controls-layer-creation.html

Thanks, this is great feedback. (In reply to comment #18) > (From update of attachment 161440 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161440&action=review > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:123 > > + // Shrink padding between 400px to 160px video width from 5px to 0px. > > Nit: this comment does not describe this code. FIXED - changed comment. > > LayoutTests/platform/chromium/media/controls-video-sizes.html:31 > > + for (var i = 0; i < videos.length; i++) { > > + videos[i].src = "../../../media/content/test.ogv"; > > + videos[i].addEventListener('canplaythrough', function () { > > + ready += 1; > > + }, false); > > + } > > + waitForAllVideosReady(); > > Nit: a more efficient way to do this would be to call "endTest()" from the event listener once ready == videos.length instead of using a timer to watch for the transition. FIXED - removed active waiting loops both for video and audio sizes test. So much simpler now!

Created attachment 163021 [details] Layout test fixes

Comment on attachment 163021 [details] Layout test fixes Clearing flags on attachment: 163021 Committed r128075: <http://trac.webkit.org/changeset/128075>

This made http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/media/controls-video-sizes.html time out on all of our bots. Could you take a look?

platform/chromium/media/controls-audio-sizes.html is very flaky on win, linux, and mac bots. Example failure diff: --- /mnt/data/b/build/slave/Webkit_Linux/build/layout-test-results/platform/chromium/media/controls-audio-sizes-expected.txt +++ /mnt/data/b/build/slave/Webkit_Linux/build/layout-test-results/platform/chromium/media/controls-audio-sizes-actual.txt @@ -35,7 +35,7 @@ ** The audio width is 274 ** EXPECTED (getComputedStyle(enclosure)['display'] == 'block') OK EXPECTED (getComputedStyle(remainingtime)['display'] == 'none') OK -EXPECTED (getComputedStyle(volumeslider)['display'] == 'none') OK +EXPECTED (getComputedStyle(volumeslider)['display'] == 'none'), OBSERVED '-webkit-box' FAIL EXPECTED (getComputedStyle(mutebutton)['display'] == '-webkit-box') OK EXPECTED (getComputedStyle(timeline)['display'] == '-webkit-box') OK it's failing roughly 1/3 to 1/2 of the time

This patch is being rolled out in https://bugs.webkit.org/show_bug.cgi?id=96435, due both to the flakiness of the included tests and content_browsertest failures (see http://code.google.com/p/chromium/issues/detail?id=148525)

Will take a look.

Thanks. Rolled out in http://trac.webkit.org/changeset/128235.

(In reply to comment #23) > This made http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/media/controls-video-sizes.html time out on all of our bots. Could you take a look? It is a slow test, because it's waiting for a bunch of videos to be loaded. Is there something I can do to allow it more time?

You can break the test up into smaller pieces that take less time.

(In reply to comment #29) > You can break the test up into smaller pieces that take less time. TL;DR; I'll give it a go. Thoughts: To get consistent output, I need to wait for all videos to be ready before testing their properties and printing them. I can try collecting their properties in an array and printing from there - not sure that will help. There's just too many videos. I can try doing the changes to one video in sequence and probe for properties along the way. What I'd like to avoid is ripping each test into 10 different tests. I'll try. (Let me know if you have a better idea.)

I would really strongly recommend you break this test up into different tests, one for each case that's interesting. Think about this - when a test fails, the main piece of information you will have is the name of the test that fails. If all you know is "video-control-sizes.html" fails, does that tell you where to look for the bug? No, by itself it's kind of useless. If "video-control-sizes-lose-display-time.html" fails but all the other tests pass, does that help? Writing mega-tests feels like an economy of effort since you only have to set up the test cases once, but it's a false economy. Most of the time most of your effort is spent maintaining and bugfixing, not setting up the initial code+tests. Optimize for maintainability. Future you and every gardener will thank you.

Good points, thanks!

I've broken down the video layout tests, which indeed stops them from being flaky. However, I cannot replicate the failure of comment #c24 . I also can't replicate the content_browsertest failures linked in #c25 (excerpt) : MediaLayoutTest.VideoLoopTest: [5519:-1408298304:0911/141346:1061678767541:INFO:layout_browsertest.cc(186)] Navigating to URL file:///var/folders/_8/gvsht6_s7bx9c6xd69hb02hw0000gp/T/.org.chromium.Chromium.RPCwJ1/LayoutTests/media/video-loop.html and blocking. [5519:-1408298304:0911/141347:1062650059529:INFO:layout_browsertest.cc(190)] Navigation completed, now waiting for title. [5520:-1408298304:0911/141348:1063120175893:ERROR:command_buffer_proxy_impl.cc(506)] Failed to Initialize GPU decoder on profile: -1 ASSERTION FAILED: enclosingIntRect(rendererMappedResult) == enclosingIntRect(FloatQuad(result).boundingBox()) ../../third_party/WebKit/Source/WebCore/rendering/RenderGeometryMap.cpp(85) : WebCore::FloatRect WebCore::RenderGeometryMap::absoluteRect(const WebCore::FloatRect &) const [..] Any suggestions?

(In reply to comment #33) > I also can't replicate the content_browsertest failures linked in #c25 (excerpt) : > > MediaLayoutTest.VideoLoopTest: > [5519:-1408298304:0911/141346:1061678767541:INFO:layout_browsertest.cc(186)] Navigating to URL file:///var/folders/_8/gvsht6_s7bx9c6xd69hb02hw0000gp/T/.org.chromium.Chromium.RPCwJ1/LayoutTests/media/video-loop.html and blocking. > [5519:-1408298304:0911/141347:1062650059529:INFO:layout_browsertest.cc(190)] Navigation completed, now waiting for title. > [5520:-1408298304:0911/141348:1063120175893:ERROR:command_buffer_proxy_impl.cc(506)] Failed to Initialize GPU decoder on profile: -1 > ASSERTION FAILED: enclosingIntRect(rendererMappedResult) == enclosingIntRect(FloatQuad(result).boundingBox()) > ../../third_party/WebKit/Source/WebCore/rendering/RenderGeometryMap.cpp(85) : WebCore::FloatRect WebCore::RenderGeometryMap::absoluteRect(const WebCore::FloatRect &) const > [..] > > Any suggestions? I was able to reproduce this only on cr-mac debug. Are you testing on mac?

(In reply to comment #34) > (In reply to comment #33) > > I also can't replicate the content_browsertest failures linked in #c25 (excerpt) : > > > > MediaLayoutTest.VideoLoopTest: > > [5519:-1408298304:0911/141346:1061678767541:INFO:layout_browsertest.cc(186)] Navigating to URL file:///var/folders/_8/gvsht6_s7bx9c6xd69hb02hw0000gp/T/.org.chromium.Chromium.RPCwJ1/LayoutTests/media/video-loop.html and blocking. > > [5519:-1408298304:0911/141347:1062650059529:INFO:layout_browsertest.cc(190)] Navigation completed, now waiting for title. > > [5520:-1408298304:0911/141348:1063120175893:ERROR:command_buffer_proxy_impl.cc(506)] Failed to Initialize GPU decoder on profile: -1 > > ASSERTION FAILED: enclosingIntRect(rendererMappedResult) == enclosingIntRect(FloatQuad(result).boundingBox()) > > ../../third_party/WebKit/Source/WebCore/rendering/RenderGeometryMap.cpp(85) : WebCore::FloatRect WebCore::RenderGeometryMap::absoluteRect(const WebCore::FloatRect &) const > > [..] > > > > Any suggestions? > > I was able to reproduce this only on cr-mac debug. Are you testing on mac? I am developing on Mac and have never seen these failures. Not on 10.6 nor on 10.8. I've even gone ahead and removed the ifdef around the assertion to make sure I wouldn't miss it by accident. But I just can't trigger it.

(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #33) > > > I also can't replicate the content_browsertest failures linked in #c25 (excerpt) : > > > > > > MediaLayoutTest.VideoLoopTest: > > > [5519:-1408298304:0911/141346:1061678767541:INFO:layout_browsertest.cc(186)] Navigating to URL file:///var/folders/_8/gvsht6_s7bx9c6xd69hb02hw0000gp/T/.org.chromium.Chromium.RPCwJ1/LayoutTests/media/video-loop.html and blocking. > > > [5519:-1408298304:0911/141347:1062650059529:INFO:layout_browsertest.cc(190)] Navigation completed, now waiting for title. > > > [5520:-1408298304:0911/141348:1063120175893:ERROR:command_buffer_proxy_impl.cc(506)] Failed to Initialize GPU decoder on profile: -1 > > > ASSERTION FAILED: enclosingIntRect(rendererMappedResult) == enclosingIntRect(FloatQuad(result).boundingBox()) > > > ../../third_party/WebKit/Source/WebCore/rendering/RenderGeometryMap.cpp(85) : WebCore::FloatRect WebCore::RenderGeometryMap::absoluteRect(const WebCore::FloatRect &) const > > > [..] > > > > > > Any suggestions? > > > > I was able to reproduce this only on cr-mac debug. Are you testing on mac? > > I am developing on Mac and have never seen these failures. Not on 10.6 nor on 10.8. I've even gone ahead and removed the ifdef around the assertion to make sure I wouldn't miss it by accident. But I just can't trigger it. Don't know what to recommend in that case, other than re-landing and coordinating with the gardener/sheriff to see if it repros on the bots.

Created attachment 165532 [details] split up flaky video tests

Created attachment 167923 [details] video controls adaptations via layout Please review if layout()\(\) is the right approach, including for chldren of enclosure. Also note that merging controls-video-sizes-padding0 and controls-video-sizes-padding1 into one layout test will make the layout test that DumpRenderTree runs right after it fail - a very curious bug that goes away when running layout tests with --run-singly. Any pointers appreciated.

I have this curious DumpRenderTree video bug: If I add the video elements of controls-video-sizes-padding0 and controls-video-sizes-padding1 together in one layout test to create: <body onload="init()"> <p>Padding on controls enclosure changes from 5px to 4px at 400px width in Chromium</p> <video controls width="401px"></video> <video controls width="400px"></video> </body> instead of <body onload="init()"> <p>Padding on controls enclosure changes from 5px to 4px at 400px width in Chromium</p> <video controls width="401px"></video> </body> this will cause the next video controls layout test that DumpRenderTree touches to become flaky (i.e. it will hang the first time (and get killed), and pass with flying colors the second time). I have not been able to track down what state hangover DumpRenderTree has from controls-video-sizes-padding with these large-ish videos. Any ideas would be very much appreciated.

Created attachment 167931 [details] rebased on master - merged TestExpectations file

(In reply to comment #39) > I have this curious DumpRenderTree video bug: > > If I add the video elements of controls-video-sizes-padding0 and controls-video-sizes-padding1 together in one layout test to create: > > <body onload="init()"> > <p>Padding on controls enclosure changes from 5px to 4px at 400px width in Chromium</p> > <video controls width="401px"></video> > <video controls width="400px"></video> I forgot to mention: the bug only happens if the second video has a width of between 350px and 400px.

Comment on attachment 167931 [details] rebased on master - merged TestExpectations file Attachment 167931 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14245404 New failing tests: platform/chromium/virtual/softwarecompositing/video/video-controls-layer-creation.html

Comment on attachment 167931 [details] rebased on master - merged TestExpectations file View in context: https://bugs.webkit.org/attachment.cgi?id=167931&action=review Some drive-by comments...one of the media folk should probably do a final review. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:67 > +class RenderMediaControlPanelEnclosureElement : public RenderBlock { This isn't an Element. It's a RenderObject. How about just calling it RenderMediaControlPanelEnclosure? > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:84 > +static const int hideTimeDisplayWidth = 350; > +static const int hideVolumeDisplayWidth = 275; > +static const int hideMuteButtonWidth = 210; > +static const int hideFullscreenButtonWidth = 150; > +static const int hideTimelineWidth = 100; Does this work right with zoom? Might be worth writing a test-case to cover that. It looks like there already is video-zoom-controls.html. Does that cover this? > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:89 > + if (style()->display() == NONE) > + return; Is this necessary? Shouldn't this renderer be destroyed if it's display:none? > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:95 > + float mediaWidthFloat = toRenderMedia(mediaElement->renderer())->contentBoxRect().width().toFloat(); I think you can s/contextBoxRect().width()/contentWdith(). > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:97 > + Nit: extra line break > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:130 > + setChildNeedsLayout(true); While this shouldn't break anything, I don't think this line is necessary and it might cause unnecessary layouts. > LayoutTests/platform/chromium/TestExpectations:3638 > +# Need rebaseline. Can you add the results for whichever platform you're developing on?

Emil, Levi, mind taking a quick look at the float math here and confirming it doesn't whatever it needs to to properly work with sub-pixel?

(In reply to comment #43) > > Some drive-by comments...one of the media folk should probably do a final review. Thanks, very much appreciated! > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:67 > > +class RenderMediaControlPanelEnclosureElement : public RenderBlock { > > This isn't an Element. It's a RenderObject. How about just calling it RenderMediaControlPanelEnclosure? OK. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:84 > > +static const int hideTimeDisplayWidth = 350; > > +static const int hideVolumeDisplayWidth = 275; > > +static const int hideMuteButtonWidth = 210; > > +static const int hideFullscreenButtonWidth = 150; > > +static const int hideTimelineWidth = 100; > > Does this work right with zoom? Might be worth writing a test-case to cover that. It looks like there already is video-zoom-controls.html. Does that cover this? I have tested it with zoom, so can confirm it works. But I can make one of the tests have a zoom, just to be explicit. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:89 > > + if (style()->display() == NONE) > > + return; > > Is this necessary? Shouldn't this renderer be destroyed if it's display:none? You're right. Thanks for noticing. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:95 > > + float mediaWidthFloat = toRenderMedia(mediaElement->renderer())->contentBoxRect().width().toFloat(); > > I think you can s/contextBoxRect().width()/contentWdith(). OK. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:97 > > + > > Nit: extra line break OK. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:130 > > + setChildNeedsLayout(true); > > While this shouldn't break anything, I don't think this line is necessary and it might cause unnecessary layouts. OK. > > LayoutTests/platform/chromium/TestExpectations:3638 > > +# Need rebaseline. > > Can you add the results for whichever platform you're developing on? I only added those for my new tests, but this is a good idea, thanks.

Just found another problem with this - when the video is small and I mouse over the volume display, the volume slider (that is supposed to be hidden) reappears. Gotto sort this out first.

To fix that mouse-over problem I would need to create a new subclass Chromium-specific subclass of MediaControlPanelMuteButtonElement to override the defaultEventHandler of that class, so I am leaving this for the reorg in bug 88871. It's a feature for now, since we also don't have keyboard shortcuts to increase/decrease volume.

Rather than committing something flaky, I will first address bug 88871.

dependency between tests - which made them flaky - should be addressed with the fix in bug 93195 (thanks Noel for pointing it out!)