[webkit-reviews] review denied: [Bug 89344] [Chromium] Handle smaller sizes of media elements in Chromium video controls : [Attachment 160628] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Silvia Pfeiffer
<silviapfeiffer1 at gmail.com>'s request for review:
Bug 89344: [Chromium] Handle smaller sizes of media elements in Chromium video
controls
https://bugs.webkit.org/show_bug.cgi?id=89344

Attachment 160628: Patch
https://bugs.webkit.org/attachment.cgi?id=160628&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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.