Bug 89344

Summary: [Chromium] Handle smaller sizes of media elements in Chromium video controls
Product: WebKit Reporter: Silvia Pfeiffer <silviapf>
Component: MediaAssignee: Silvia Pfeiffer <silviapf>
Status: RESOLVED WONTFIX    
Severity: Normal CC: adamk, annacc, cmarcelo, dglazkov, eae, eric.carlson, eric, feature-media-reviews, jamesr, leviw, macpherson, menard, ojan, schenney, tony, vcarbune, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 88871    
Bug Blocks: 84672    
Attachments:
Description Flags
patch for discussion
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
feedback included
none
rewrote tests
none
Layout test fixes
none
split up flaky video tests
none
video controls adaptations via layout
none
rebased on master - merged TestExpectations file ojan: review-, webkit.review.bot: commit-queue-

Description Silvia Pfeiffer 2012-06-18 06:15:47 PDT
As part of the Chrome video controls visual update, individual interactive elements drop successively as the video/audio element gets smaller.
Comment 1 Silvia Pfeiffer 2012-06-18 17:33:05 PDT
Created attachment 148210 [details]
patch for discussion
Comment 2 Silvia Pfeiffer 2012-06-18 17:35:06 PDT
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 3 WebKit Review Bot 2012-06-19 07:32:14 PDT
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
Comment 4 WebKit Review Bot 2012-06-19 07:32:19 PDT
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 5 Eric Carlson 2012-06-26 14:55:45 PDT
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!
Comment 6 Silvia Pfeiffer 2012-08-26 21:54:29 PDT
Created attachment 160628 [details]
Patch
Comment 7 Silvia Pfeiffer 2012-08-27 03:13:52 PDT
(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 8 Eric Carlson 2012-08-27 07:27:24 PDT
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.
Comment 9 Silvia Pfeiffer 2012-08-28 01:24:39 PDT
(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!
Comment 10 Silvia Pfeiffer 2012-08-28 02:16:25 PDT
Created attachment 160933 [details]
feedback included
Comment 11 Eric Carlson 2012-08-28 09:42:48 PDT
(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 12 Eric Carlson 2012-08-28 09:58:58 PDT
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?
Comment 13 Silvia Pfeiffer 2012-08-28 17:03:10 PDT
(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.
Comment 14 Silvia Pfeiffer 2012-08-29 01:45:20 PDT
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.
Comment 15 Eric Carlson 2012-08-29 11:01:00 PDT
(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.
Comment 16 Silvia Pfeiffer 2012-08-30 04:11:20 PDT
Created attachment 161440 [details]
rewrote tests
Comment 17 Silvia Pfeiffer 2012-08-30 04:16:00 PDT
(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 18 Eric Carlson 2012-08-30 10:09:14 PDT
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 19 WebKit Review Bot 2012-08-30 12:45:27 PDT
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
Comment 20 Silvia Pfeiffer 2012-09-09 17:45:13 PDT
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!
Comment 21 Silvia Pfeiffer 2012-09-09 17:53:45 PDT
Created attachment 163021 [details]
Layout test fixes
Comment 22 WebKit Review Bot 2012-09-10 10:15:21 PDT
Comment on attachment 163021 [details]
Layout test fixes

Clearing flags on attachment: 163021

Committed r128075: <http://trac.webkit.org/changeset/128075>
Comment 23 James Robinson 2012-09-11 09:47:26 PDT
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?
Comment 24 James Robinson 2012-09-11 12:16:07 PDT
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
Comment 25 Adam Klein 2012-09-11 15:41:23 PDT
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)
Comment 26 Silvia Pfeiffer 2012-09-11 15:42:00 PDT
Will take a look.
Comment 27 Adam Klein 2012-09-11 16:24:52 PDT
Thanks. Rolled out in http://trac.webkit.org/changeset/128235.
Comment 28 Silvia Pfeiffer 2012-09-11 20:13:14 PDT
(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?
Comment 29 James Robinson 2012-09-11 22:30:40 PDT
You can break the test up into smaller pieces that take less time.
Comment 30 Silvia Pfeiffer 2012-09-11 23:07:12 PDT
(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.)
Comment 31 James Robinson 2012-09-11 23:24:55 PDT
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.
Comment 32 Silvia Pfeiffer 2012-09-12 00:14:01 PDT
Good points, thanks!
Comment 33 Silvia Pfeiffer 2012-09-18 18:32:50 PDT
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?
Comment 34 Adam Klein 2012-09-19 09:16:30 PDT
(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?
Comment 35 Silvia Pfeiffer 2012-09-19 16:43:35 PDT
(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.
Comment 36 Adam Klein 2012-09-19 16:50:29 PDT
(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.
Comment 37 Silvia Pfeiffer 2012-09-24 23:39:54 PDT
Created attachment 165532 [details]
split up flaky video tests
Comment 38 Silvia Pfeiffer 2012-10-09 22:15:42 PDT
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.
Comment 39 Silvia Pfeiffer 2012-10-09 22:25:20 PDT
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.
Comment 40 Silvia Pfeiffer 2012-10-09 23:16:40 PDT
Created attachment 167931 [details]
rebased on master - merged TestExpectations file
Comment 41 Silvia Pfeiffer 2012-10-09 23:37:30 PDT
(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 42 WebKit Review Bot 2012-10-10 00:08:42 PDT
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 43 Ojan Vafai 2012-10-10 09:12:04 PDT
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?
Comment 44 Ojan Vafai 2012-10-10 09:13:10 PDT
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?
Comment 45 Silvia Pfeiffer 2012-10-10 21:16:06 PDT
(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.
Comment 46 Silvia Pfeiffer 2012-10-23 18:09:38 PDT
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.
Comment 47 Silvia Pfeiffer 2012-10-23 19:42:42 PDT
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.
Comment 48 Silvia Pfeiffer 2012-10-25 15:32:55 PDT
Rather than committing something flaky, I will first address bug 88871.
Comment 49 Silvia Pfeiffer 2012-10-31 21:08:30 PDT
dependency between tests - which made them flaky - should be addressed with the fix in bug 93195 (thanks Noel for pointing it out!)