[webkit-reviews] review denied: [Bug 89344] [Chromium] Handle smaller sizes of media elements in Chromium video controls : [Attachment 167931] rebased on master - merged TestExpectations file

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Silvia Pfeiffer
<silviapf at chromium.org>'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 167931: rebased on master - merged TestExpectations file
https://bugs.webkit.org/attachment.cgi?id=167931&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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?