[webkit-reviews] review denied: [Bug 26304] [GTK] Add controls for playing html5 video. : [Attachment 39467] Media controls impl. 3

[bugzilla-daemon at webkit.org]

Jan Alonzo <jmalonzo at gmail.com> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 26304: [GTK] Add controls for playing html5 video.
https://bugs.webkit.org/show_bug.cgi?id=26304

Attachment 39467: Media controls impl. 3
https://bugs.webkit.org/attachment.cgi?id=39467&action=review

------- Additional Comments from Jan Alonzo <jmalonzo at gmail.com>

> +PassRefPtr<Image> Image::loadPlatformThemeIcon(const char* name, int size)
> +{
> +    RefPtr<BitmapImage> img = BitmapImage::create();
> +    RefPtr<SharedBuffer> buffer =
loadResourceSharedBuffer(getThemeIconFileName(name, size));
>      img->setData(buffer.release(), true);
>      return img.release();

Can we share code with loadPlatformResource here? Also, what do you think about
overriding loadPlatformResource with a size parameter?

> +void RenderThemeGtk::platformColorsDidChange()
> +{
> +#if ENABLE(VIDEO)
> +    initMediaStyling(true);
> +#endif
> +    RenderTheme::platformColorsDidChange();
> +}
> +
> +#if ENABLE(VIDEO)
> +String RenderThemeGtk::extraMediaControlsStyleSheet()
> +{
> +    gchar* fileName =
g_strdup_printf("%s/webkit-1.0/css/mediaControls-extras.css", DATA_DIR);
> +    String returnString = String();
> +
> +    GOwnPtr<gchar> content;
> +    gsize length;
> +    if (g_file_get_contents(fileName, &content.outPtr(), &length, 0))
> +	   returnString = String(content.get(), length);
> +
> +    g_free(fileName);

This doesn't look right. Other ports are using UserAgentStyleSheet for this. Is
there a reason why we're not doing the same?

> +HTMLMediaElement*
RenderThemeGtk::getMediaElementFromRenderObject(RenderObject* o) const

> +char* getIconNameForTextDirection(const char* name)

I think this functions can be made static?

> +static Color colorControlPanelBackground = 0;
> +static Color colorControlPanelForeground = 0;
> +static Color colorSliderBackground = 0;
> +static Color colorSliderThumb = 0;
> +
> +static char* playButtonIconName = 0;
> +static char* seekBackButtonIconName = 0;
> +static char* seekForwardButtonIconName = 0;
> +
> +static PassRefPtr<Image> fullscreenButton = 0;
> +static PassRefPtr<Image> muteButton = 0;
> +static PassRefPtr<Image> unmuteButton = 0;
> +static PassRefPtr<Image> playButton = 0;
> +static PassRefPtr<Image> pauseButton = 0;
> +static PassRefPtr<Image> seekBackButton = 0;
> +static PassRefPtr<Image> seekForwardButton = 0;
> +
> +static const int largeIconSize = 32;
> +static const int smallIconSize = 16;
> +static const int panelButtonPadding = 2;
> +static const int buttonImagePadding = 2;

Please put this at the top of the file so they're more visible.

> +
> +void RenderThemeGtk::initMediaStyling(bool force)
> +{

This could just be 'static'. Is it going to be used somewhere else?

> +    static bool stylingInitialized = false;
> +
> +    if (!stylingInitialized || force) {
> +	   GtkContainer* container = gtkContainer();
> +	   GtkStyle* style = gtk_rc_get_style(GTK_WIDGET(container));
> +	   colorControlPanelBackground = Color(style->bg[GTK_STATE_NORMAL],
140);

What's 140?

> +    // There could be a mute button to the right, so don't round the panel
rect just yet.
> +    IntSize panelCornerRounding = mediaElement->hasAudio() ? IntSize(0, 0) :
IntSize(5, 5);
> +    IntSize buttonCornerRounding = mediaElement->hasAudio() ? IntSize(0, 0)
: IntSize(3, 3);
> +    // If there's a mute button to the right, we add a padding on the left
side of
> +    // the fullscreen button and concatenate the two buttons. If there is no
mute button,
> +    // there's no padding added and a padding appears on the right.
> +    int leftPadding = mediaElement->hasAudio() ? panelButtonPadding : 0;
> +
> +    paintInfo.context->fillRoundedRect(r, IntSize(0, 0),
panelCornerRounding, IntSize(0, 0), panelCornerRounding,
colorControlPanelBackground);
> +    paintInfo.context->fillRoundedRect(IntRect(r.x() + leftPadding,
> +						  r.y() + (r.height() -
(smallIconSize + buttonImagePadding * 2)) / 2,
> +						  smallIconSize +
buttonImagePadding * 2, smallIconSize + buttonImagePadding * 2),
> +					  IntSize(3, 3), buttonCornerRounding,
IntSize(3, 3), buttonCornerRounding, colorControlPanelForeground);
> +    paintInfo.context->drawImage(fullscreenButton.get(),
> +				    IntRect(r.x() + leftPadding +
buttonImagePadding,
> +					    r.y() + (r.height() -
smallIconSize) / 2,
> +					    smallIconSize, smallIconSize));

There are too many magic numbers here. What are they for? Have you looked at
the layout tests and see if these numbers will affect our control metrics
(e.g., in terms of comparing render trees with other ports)? 

   
> +#if ENABLE(VIDEO)
> +    HTMLMediaElement* getMediaElementFromRenderObject(RenderObject*) const;
> +    void initMediaStyling(bool force = false);
> +#endif
> +

These should just be statics. 

r- because the painting uses too many magic numbers and need confirmation if it
affects layout tests. Might be good to enable some media-related or create
layout tests for these to make sure we're at least close or consistent with
other ports.