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

[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 160933: feedback included
https://bugs.webkit.org/attachment.cgi?id=160933&action=review

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