[webkit-reviews] review denied: [Bug 83869] [Gtk] HTML5 Media controls require a design refresh : [Attachment 184253] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Xabier Rodríguez Calvar
<calvaris at igalia.com>'s request for review:
Bug 83869: [Gtk] HTML5 Media controls require a design refresh
https://bugs.webkit.org/show_bug.cgi?id=83869

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=184253&action=review


The controls are looking awesome. I can't wait to try them out. Yet, there are
a few things wrong with this patch. The new test results are in the WebKit2
directory (they should be in platform/gtk) and they are missing pixel results.
There are some other things I noticed as well:

>> LayoutTests/platform/gtk/TestExpectations:446
>> +fullscreen/video-controls-drag.html [ Failure ]
> 
> Test lacks BUG modifier.  [test/expectations] [5]

Please add bug modifiers for these lines. If there are not bugs open yet for
GTK+, open them.

> Source/WebCore/css/mediaControlsGtk.css:118
> +    font-family: Cantarell;

The use of Cantarell here is questionable. Cantarell isn't the official GTK+
font. If you are interested in using the correct font for system controls you
should use one of the internal CSS control fonts like -webkit-small-control.

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:66
> +    ExceptionCode ec;

Please use a name like exceptionCode.

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:214
> +#if ENABLE(VIDEO_TRACK)
> +void MediaControlsGtk::createTextTrackDisplay()

I still would like to know if GStreamer has VIDEO_TRACK support.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:147
> +    // We add here the media control elements to avoid WebCore
> +    // painting the focus ring as it is only painted if the focus ring
> +    // is supported

Hrm. I'm not sure I understand this comment. Maybe you can just remove it.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:592
> +    int radiusTopLeftWidth =
style->borderTopLeftRadius().width().intValue();
> +    int radiusTopLeftHeight =
style->borderTopLeftRadius().height().intValue();
> +    int radiusTopRightWidth =
style->borderTopRightRadius().width().intValue();
> +    int radiusTopRightHeight =
style->borderTopRightRadius().height().intValue();
> +    int radiusBottomLeftWidth =
style->borderBottomLeftRadius().width().intValue();
> +    int radiusBottomLeftHeight =
style->borderBottomLeftRadius().height().intValue();
> +    int radiusBottomRightWidth =
style->borderBottomRightRadius().width().intValue();
> +    int radiusBottomRightHeight =
style->borderBottomRightRadius().height().intValue();

Wow! This is a lot of code to shuffle a single value around. It would be a lot
cleaner to just have the radius (which is the same for all corners) as a static
const right before you use it.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:594
> +    Color color = style->visitedDependentColor(CSSPropertyColor);
> +    ColorSpace colorSpace = style->colorSpace();

There's no need to cache these values.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:636
> +    int topPadding = padding.top().intValue();
> +    int rightPadding = padding.right().intValue();
> +    int bottomPadding = padding.bottom().intValue();
> +    int leftPadding = padding.left().intValue();
> +    IntRect newRect = r;
> +    newRect.move(leftPadding, topPadding);
> +    newRect.contract(leftPadding + rightPadding, topPadding +
bottomPadding);

I'm almost certain the rectangle should already take into account the padding,
so this should really be done in CSS with something like margins or border.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:647
> +    LengthSize radiusTopLeft = style->borderTopLeftRadius();
> +    LengthSize radiusTopRight = style->borderTopRightRadius();
> +    LengthSize radiusBottomLeft = style->borderBottomLeftRadius();
> +    LengthSize radiusBottomRight = style->borderBottomRightRadius();
> +    paintInfo.context->fillRoundedRect(newRect,
> +	   IntSize(radiusTopLeft.width().intValue(),
radiusTopLeft.height().intValue()),
> +	   IntSize(radiusTopRight.width().intValue(),
radiusTopRight.height().intValue()),
> +	   IntSize(radiusBottomLeft.width().intValue(),
radiusBottomLeft.height().intValue()),
> +	   IntSize(radiusBottomRight.width().intValue(),
radiusBottomRight.height().intValue()),
> +	   style->visitedDependentColor(CSSPropertyColor),
> +	   style->colorSpace());

Again it would be better to use a constant declared locally instead of all this
value shuffling.

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:919
> +    GtkIconInfo* info =
gtk_icon_theme_lookup_icon(gtk_icon_theme_get_default(),
> +	   symbolicIconName,
> +	   iconSize,
> +	   static_cast<GtkIconLookupFlags>(GTK_ICON_LOOKUP_FORCE_SVG |
GTK_ICON_LOOKUP_FORCE_SIZE));

It's probably a bit more readable simply use two lines for this. Lines in
WebKit can go up to and past 120 characters long.