[Webkit-unassigned] [Bug 26304] [GTK] Add controls for playing html5 video.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26304


Jan Alonzo <jmalonzo at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #39467|review?                     |review-, commit-queue-
               Flag|                            |




--- Comment #13 from Jan Alonzo <jmalonzo at gmail.com>  2009-09-11 21:14:23 PDT ---
(From update of attachment 39467)

> +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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.