Bug 26304 – [GTK] Add controls for playing html5 video.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Eric]

Many sites such as tinyvid.tv do not implement controls for video, but rely on the browser's built in controls.  Currently webkit-gtk with gstreamer video does not support this.

[Xan Lopez]

*** Bug 26369 has been marked as a duplicate of this bug. ***

[Xan Lopez]

*** This bug has been marked as a duplicate of bug 28249 ***

[Xan Lopez]

I dupped this the wrong way around, sorry.

[Xan Lopez]

*** Bug 28249 has been marked as a duplicate of this bug. ***

[Martin Sourada]

Sorry about making a duplicate, I somehow didn't find this one when I searched for it (probably because I searched for '<video>' instead of just 'video') :-/

[Zan Dobersek]

Created attachment 38452 [details]
Media controls impl., 1.

Implements media controls painting.

From ChangeLog entry:

        Implements controls for media elements.

        Adds a new initializator to the Color class, which makes it possible
        to initialize Color with a GdkColor and an alpha channel.

        Adds a new loading function to the Image class, loadPlatformThemeIcon,
        which loads only platform theme icons. Available only for the Gtk+ port.
        Also modifies the whole process of loading platform resources so there
        is a maximum sharing of code between this new function and the
        loadPlatformResource function.

        Adds a stylesheet that overrides some of the default style settings of
        the media control panel.

        Changes by this patch in RenderThemeGtk class:
        - media slider thumb's size gets properly adjusted,
        - gtkContainer's 'style-set' signal is connected to the appropriate callback,
        which forces reinitialization of icons and colors on a style change,
        - implements paintMedia* functions, which now properly paint media controls.

I believe there can be some style or naming issues, I'll be glad to fix them.

[Zan Dobersek]

Created attachment 38456 [details]
Media controls impl. 2.

Removed an unneeded fprintf call.

[Xan Lopez]

Comment on attachment 38456 [details]
Media controls impl. 2.

> +webstylesheetsdir = ${datadir}/webkit-1.0/css
> +dist_webstylesheetsdir = \

This should be dist_webstylesheets_DATA = \ AFAIK

> +	$(WebCore)/css/gtk/mediaControls-extras.css
> +
>  # Clean rules for WebCore
>  
>  CLEANFILES += \
> diff --git a/WebCore/css/gtk/mediaControls-extras.css b/WebCore/css/gtk/mediaControls-extras.css
> new file mode 100644
> index 0000000..35d0c5f
> --- /dev/null
> +++ b/WebCore/css/gtk/mediaControls-extras.css
> @@ -0,0 +1,68 @@
> +/*
> + * WebKitGTK+ specific overrides for HTML5 media elements.
> + *
> + * Copyright (C) 2009 Zan Dobersek <[e-mail]>
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

Are you sure you are using the correct license header here? :)

> diff --git a/WebCore/platform/gtk/RenderThemeGtk.cpp b/WebCore/platform/gtk/RenderThemeGtk.cpp
> index fdef9c2..bf80e2a 100644
> --- a/WebCore/platform/gtk/RenderThemeGtk.cpp

(...)

> +using namespace HTMLNames;
> +
>  PassRefPtr<RenderTheme> RenderThemeGtk::create()
>  {
>      return adoptRef(new RenderThemeGtk());
> @@ -446,6 +452,14 @@ void RenderThemeGtk::systemFont(int, FontDescription&) const
>      notImplemented();
>  }
>  
> +void RenderThemeGtk::adjustSliderThumbSize(RenderObject* o) const
> +{
> +    if (o->style()->appearance() == MediaSliderThumbPart) {
> +        o->style()->setWidth(Length(12, Fixed));
> +        o->style()->setHeight(Length(12, Fixed));
> +    }
> +}

This could possibly go in a different patch...

> +
>  static void gtkStyleSetCallback(GtkWidget* widget, GtkStyle* previous, RenderTheme* renderTheme)
>  {
>      // FIXME: Make sure this function doesn't get called many times for a single GTK+ style change signal.
> @@ -459,6 +473,7 @@ GtkContainer* RenderThemeGtk::gtkContainer() const
>  
>      m_gtkWindow = gtk_window_new(GTK_WINDOW_POPUP);
>      m_gtkContainer = GTK_CONTAINER(gtk_fixed_new());
> +    g_signal_connect(GTK_WIDGET(m_gtkWindow), "style-set", G_CALLBACK(gtkStyleSetCallback), const_cast<RenderThemeGtk*>(this));

No need to cast m_gtkWindow to GTK_WIDGET here.

>      gtk_container_add(GTK_CONTAINER(m_gtkWindow), GTK_WIDGET(m_gtkContainer));
>      gtk_widget_realize(m_gtkWindow);
>  
> @@ -491,4 +506,216 @@ GtkWidget* RenderThemeGtk::gtkTreeView() const
>      return m_gtkTreeView;
>  }
>  
> +void RenderThemeGtk::platformColorsDidChange()
> +{
> +#if ENABLE(VIDEO)
> +    initMediaStyling(true);
> +#endif
> +    RenderTheme::platformColorsDidChange();
> +}
> +
> +#if ENABLE(VIDEO)
> +String RenderThemeGtk::extraMediaControlsStyleSheet()
> +{
> +    GString* fileName = g_string_new(0);
> +    g_string_printf(fileName, "%s/webkit-1.0/css/mediaControls-extras.css", DATA_DIR);

You can simply use g_strdup_printf here.

> +
> +    String returnString = String();
> +
> +    GOwnPtr<gchar> content;
> +    gsize length;
> +    if (g_file_get_contents(fileName->str, &content.outPtr(), &length, 0))
> +        returnString = String(content.get(), length);
> +
> +    g_string_free(fileName, TRUE);
> +
> +    return returnString;
> +}
> +
> +char* getIconNameForTextDirection(const char* name)
> +{
> +    GString* directionName = g_string_new(name);
> +    GtkTextDirection direction = gtk_widget_get_default_direction();
> +
> +    if (direction == GTK_TEXT_DIR_RTL)
> +        g_string_append(directionName, "-rtl");
> +    else if (direction == GTK_TEXT_DIR_LTR)
> +        g_string_append(directionName, "-ltr");
> +
> +    gchar* returnString = g_strdup(directionName->str);
> +    g_string_free(directionName, TRUE);
> +
> +    return returnString;

You can do 'return g_string_free(directionName, FALSE)'

> +}
> +
> +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 Image* fullscreenButton = 0;
> +static Image* muteButton = 0;
> +static Image* unmuteButton = 0;
> +static Image* playButton = 0;
> +static Image* pauseButton = 0;
> +static Image* seekBackButton = 0;
> +static Image* seekForwardButton = 0;
> +
> +void RenderThemeGtk::initMediaStyling(bool force)
> +{
> +    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);
> +        colorControlPanelForeground = style->bg[GTK_STATE_NORMAL];
> +        colorSliderBackground = style->bg[GTK_STATE_ACTIVE];
> +        colorSliderThumb = style->bg[GTK_STATE_SELECTED];
> +
> +        if (playButtonIconName)
> +            g_free(playButtonIconName);
> +        if (seekBackButtonIconName)
> +            g_free(seekBackButtonIconName);
> +        if (seekForwardButtonIconName)
> +            g_free(seekForwardButtonIconName);

g_free is NULL-safe, no nee to check.

> +        playButtonIconName = getIconNameForTextDirection("gtk-media-play");
> +        seekBackButtonIconName = getIconNameForTextDirection("gtk-media-rewind");
> +        seekForwardButtonIconName = getIconNameForTextDirection("gtk-media-forward");
> +
> +        fullscreenButton = Image::loadPlatformThemeIcon("gtk-fullscreen", 16).releaseRef();
> +        muteButton = Image::loadPlatformThemeIcon("audio-volume-muted", 16).releaseRef();
> +        unmuteButton = Image::loadPlatformThemeIcon("audio-volume-high", 16).releaseRef();
> +        playButton = Image::loadPlatformThemeIcon(reinterpret_cast<const char*>(playButtonIconName), 32).releaseRef();
> +        pauseButton = Image::loadPlatformThemeIcon("gtk-media-pause", 32).releaseRef();
> +        seekBackButton = Image::loadPlatformThemeIcon(reinterpret_cast<const char*>(seekBackButtonIconName), 32).releaseRef();
> +        seekForwardButton = Image::loadPlatformThemeIcon(reinterpret_cast<const char*>(seekForwardButtonIconName), 32).releaseRef();

Mmm, and what about the previous images, don't you need to unref them before?

> +
> +        stylingInitialized = true;
> +    }
> +}
> +
> +bool RenderThemeGtk::paintMediaFullscreenButton(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& r)
> +{
> +    initMediaStyling();
> +
> +    paintInfo.context->fillRoundedRect(r, IntSize(0, 0), IntSize(5, 5), IntSize(0, 0), IntSize(5, 5), colorControlPanelBackground);
> +    paintInfo.context->fillRoundedRect(IntRect(r.x(), r.y() + 10, 20, 20),
> +                                       IntSize(0, 0), IntSize(5, 5), IntSize(0, 0), IntSize(5, 5), colorControlPanelForeground);
> +    paintInfo.context->drawImage(fullscreenButton,
> +                                 IntRect(r.x() + 2, r.y() + 12, 16, 16));
> +    return false;
> +}
> +
> +bool RenderThemeGtk::paintMediaMuteButton(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& r)
> +{
> +    initMediaStyling();
> +    HTMLMediaElement* mediaElement = getMediaElementFromRenderObject(o);
> +    if (!mediaElement)
> +        return false;
> +
> +    paintInfo.context->fillRoundedRect(r, IntSize(5, 5), IntSize(0, 0), IntSize(5, 5), IntSize(0, 0), colorControlPanelBackground);
> +    paintInfo.context->fillRoundedRect(IntRect(r.x() + 2, r.y() + 10, 20, 20),
> +                                       IntSize(5, 5), IntSize(0, 0), IntSize(5, 5), IntSize(0, 0), colorControlPanelForeground);
> +    paintInfo.context->drawImage(mediaElement->muted() ? unmuteButton : muteButton,
> +                                 IntRect(r.x() + 4, r.y() + 12, 16, 16));
> +    return false;
> +}
> +
> +bool RenderThemeGtk::paintMediaPlayButton(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& r)
> +{
> +    initMediaStyling();
> +    HTMLMediaElement* mediaElement = getMediaElementFromRenderObject(o);
> +    if (!mediaElement)
> +        return false;
> +
> +    // If media has audio, the mute button will be painted. If not,
> +    // play/pause button will be the most left-positioned in the control panel,
> +    // so the background rect should be rounded appropriately.
> +    int xOffset = 0;
> +    if (mediaElement->hasAudio())
> +        paintInfo.context->fillRect(FloatRect(r), colorControlPanelBackground);
> +    else {
> +        paintInfo.context->fillRoundedRect(r, IntSize(5, 5), IntSize(0, 0), IntSize(5, 5), IntSize(0, 0), colorControlPanelBackground);
> +        xOffset = 2;
> +    }
> +    paintInfo.context->fillRoundedRect(IntRect(r.x() + xOffset, r.y() + 2, 36 - xOffset, 36),
> +                                       IntSize(5, 5), IntSize(5, 5), IntSize(5, 5), IntSize(5, 5), colorControlPanelForeground);
> +    paintInfo.context->drawImage(mediaElement->canPlay() ? playButton : pauseButton,
> +                                 IntRect(r.x() + xOffset / 2 + 2, r.y() + 4, 32, 32));
> +    return false;
> +}
> +
> +bool RenderThemeGtk::paintMediaSeekBackButton(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& r)
> +{
> +    initMediaStyling();
> +
> +    paintInfo.context->fillRect(FloatRect(r), colorControlPanelBackground);
> +    paintInfo.context->fillRoundedRect(IntRect(r.x(), r.y() + 2, 36, 36),
> +                                       IntSize(5, 5), IntSize(0, 0), IntSize(5, 5), IntSize(0, 0), colorControlPanelForeground);
> +    paintInfo.context->drawImage(seekBackButton,
> +                                 IntRect(r.x() + 2, r.y() + 4, 32, 32));
> +    return false;
> +}
> +
> +bool RenderThemeGtk::paintMediaSeekForwardButton(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& r)
> +{
> +    initMediaStyling();
> +    HTMLMediaElement* mediaElement = getMediaElementFromRenderObject(o);
> +    if (!mediaElement)
> +        return false;
> +
> +    // If media supports fullscreen, a fullscreen button will be painted to the right
> +    // of this button. If fullscreen is not supported, this button is the most
> +    // right-positioned in the control panel, therefor the background rect
> +    // should be properly rounded.
> +    int xOffset = 0;
> +    if (mediaElement->supportsFullscreen()) {
> +        paintInfo.context->fillRect(FloatRect(r), colorControlPanelBackground);
> +        xOffset = 2;
> +    } else
> +        paintInfo.context->fillRoundedRect(r, IntSize(0, 0), IntSize(5, 5), IntSize(0, 0), IntSize(5, 5), colorControlPanelBackground);
> +    paintInfo.context->fillRoundedRect(IntRect(r.x(), r.y() + 2, 36 + xOffset, 36),
> +                                       IntSize(0, 0), IntSize(5, 5), IntSize(0, 0), IntSize(5, 5), colorControlPanelForeground);
> +    paintInfo.context->drawImage(seekForwardButton,
> +                                 IntRect(r.x() + 2, r.y() + 4, 32, 32));
> +    return false;
> +}
> +
> +bool RenderThemeGtk::paintMediaSliderTrack(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& r)
> +{
> +    initMediaStyling();
> +
> +    paintInfo.context->fillRect(FloatRect(r), colorControlPanelBackground);
> +    paintInfo.context->fillRect(FloatRect(IntRect(r.x(), r.y() + 8, r.width(), 24)), colorControlPanelForeground);
> +
> +    paintInfo.context->fillRoundedRect(IntRect(r.x() + 2, r.y() + 13, r.width() - 4, 14),
> +                                       IntSize(3, 3), IntSize(3, 3), IntSize(3, 3), IntSize(3, 3),
> +                                       colorSliderBackground);
> +    return false;
> +}
> +
> +bool RenderThemeGtk::paintMediaSliderThumb(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& r)
> +{
> +    initMediaStyling();
> +
> +    paintInfo.context->fillRoundedRect(r, IntSize(3, 3), IntSize(3, 3), IntSize(3, 3), IntSize(3, 3), colorSliderThumb);
> +    return false;
> +}
> +#endif

There's lots and lots of hardcoded numbers here, I don't like that :/

At the very least I think you should make them constants with names so it's easier to see what's going on. On top of that I see there's quite a few of completely hardcoded sizes, is it really meant to be like that?

I'm marking this r- while we work on the raised issues.

[Zan Dobersek]

Created attachment 39464 [details]
Media controls impl. 2.

Fixes issues presented in previous comment.

adjustSliderThumbSize will follow in a separate patch, as suggested.

[Zan Dobersek]

Created attachment 39466 [details]
Media controls impl. 3.

Adds the missing css stylesheet file that got away when diffing out the previous patch.

[Zan Dobersek]

Created attachment 39467 [details]
Media controls impl. 3

Makes the patch buildable - images are now stored in PassRefPtrs so they can be cleared when re-initializing.

[Jan Alonzo]

(In reply to comment #9)
> Created an attachment (id=39464) [details]
> Media controls impl. 2.
> 
> Fixes issues presented in previous comment.
> 
> adjustSliderThumbSize will follow in a separate patch, as suggested.

Is this for review? It's not marked as r?.

[Jan Alonzo]

Comment on attachment 39467 [details]
Media controls impl. 3


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

[Zan Dobersek]

It's obvious that the whole drawing and pimping that is being done now isn't actually necessary to . I've combined this and two more methods of drawing/rendering these controls into this list:

- Pimping the hell out of it: Panel is rounded, partially transparent, it just looks cool, web 2.0-like. That's what's being done now and uses many magic numbers or at least a bit of math to center the things appropriately. It's of course using system theme's colors and icons and, again, looks cool and sort of complete.
- Keeping it simple: Simple rectangular panel, with system theme's colors and buttons. No heavy math, just some basic positioning of the icons to a proper place. Simple, yet not so bad looking.
- Keeping it even more simple: Everything is drawn with what's in WebCore - triangle as a play button, two rectangles for a pause button, primitive-looking mute/unmute button ... not so stylish, but very simple. Qt does that in some way.

I'd go for either pimping or the less simple variant. Both offer good style integration into the theme. For the sake of things getting done, the less simple variant could be the first one coded and reviewed, with the second one done if there's actually a need for the good looks and the nerves for doing it. Of course, any comments, advices, thoughts on these methods are warmly welcomed.

There's also the aspect of size - currently, the pimping method's control panel is 40px high and 400px wide by default (for audio elements). Size is such because of the use of 32x32 pixels sized icons. I'm not sure we should go for such a big size, it's probably just not normal to encounter a 400x40px controls in some sites. Therefor, if we used the less simple variant, I'd go for 20px height and the use of 16px icons. Any comments, thoughts?

Regards,
Zan Dobersek

[Zan Dobersek]

Created attachment 39819 [details]
Media controls, the simple approach

This patch utilizes the 'Keep it simple' approach.

Panel is now 20 pixels high, while 16 pixel icons are used. Panel is 300 pixels wide on <audio> elements.
Icons are drawn into their rects with equal padding on top, bottom, left and right. Extra media stylesheet is now loaded as user agent style sheet and is also compiled as that.

There's more code sharing going on in ImageGtk.cpp. Media styling is now initialized at RenderThemeGtk constructor (calling it from every paintMedia* function is useless) and now takes a GtkStyle as an argument. Some functions now also became static.

Regards,
Zan Dobersek.

[Eric Seidel (no email)]

Comment on attachment 39819 [details]
Media controls, the simple approach

Why?
+    paintInfo.context->fillRect(FloatRect(r), panelColor);
+    paintInfo.context->fillRect(FloatRect(r), panelColor);

Can't we share more of this drawing code?  It seems that you could write a static inline function which did the fillRect as well as the drawImage call and save a bunch of duplicated code.

[Eric Seidel (no email)]

Adding our resident media expert. :)

[Zan Dobersek]

Created attachment 40178 [details]
Media controls, the simple approach

paintMediaButton function now used to fill the button's rect and paint the button. 'Makes code look beter.'

[Eric Carlson]

The media controller specific parts of this look good to me, but I would prefer if a GTK reviewer gave the final go-ahead as I am utterly unqualified to review that part of the patch.

[Xan Lopez]

Comment on attachment 40178 [details]
Media controls, the simple approach

>@@ -3433,6 +3434,7 @@ webcore_dist += \
> 	WebCore/bindings/scripts/InFilesParser.pm \
> 	WebCore/bindings/scripts/generate-bindings.pl \
> 	WebCore/bindings/scripts/CodeGeneratorJS.pm \
>+	WebCore/css/gtk/mediaControlsGtk.css
> 	WebCore/css/html.css \
> 	WebCore/css/quirks.css \
> 	WebCore/css/view-source.css \

Missing \ at the end of the new line, this won't work.

Also:

../../WebCore/platform/gtk/RenderThemeGtk.cpp: In member function ‘virtual WebCore::String WebCore::RenderThemeGtk::extraMediaControlsStyleSheet()’:
../../WebCore/platform/gtk/RenderThemeGtk.cpp:598: error: ‘mediaControlsGtkUserAgentStyleSheet’ was not declared in this scope
make[1]: *** [WebCore/platform/gtk/libWebCore_la-RenderThemeGtk.lo] Error 1
make[1]: Leaving directory `/home/xan/git/WebKit/build/normal'

Leftover from other patches?

Other than that, a couple of comments (after fixing the patch to compile):

>+static Color panelColor = 0;
>+static Color sliderColor = 0;
>+static Color sliderThumbColor = 0;
>+
>+// Names of these icons can vary because of text direction.
>+static char* playButtonIconName = 0;
>+static char* seekBackButtonIconName = 0;
>+static char* seekForwardButtonIconName = 0;
>+
>+static int mediaIconSize = 16;
>+static int mediaSliderHeight = 14;

This should be const. Also, I'd say these sizes should be calculated dynamically, maybe based on the size of the video element we are showing?

>+#if ENABLE(VIDEO)
>+    initMediaStyling(gtk_rc_get_style(GTK_WIDGET(gtkContainer())));
>+#endif

Do you actually need to use gtk_rc_get_style or would gtk_widget_get_style be enough?

The progress bar does not seem to work at all in my testing? (Eg, http://people.mozilla.com/~vladimir/mdemos/video1.html).

The controls seem to be over the video content, is this the intended behavior? The spec does not seem to say much about it though.

Marking as r- for now.

[Zan Dobersek]

(In reply to comment #20)
> Also:
> 
> ../../WebCore/platform/gtk/RenderThemeGtk.cpp: In member function ‘virtual
> WebCore::String WebCore::RenderThemeGtk::extraMediaControlsStyleSheet()’:
> ../../WebCore/platform/gtk/RenderThemeGtk.cpp:598: error:
> ‘mediaControlsGtkUserAgentStyleSheet’ was not declared in this scope
> make[1]: *** [WebCore/platform/gtk/libWebCore_la-RenderThemeGtk.lo] Error 1
> make[1]: Leaving directory `/home/xan/git/WebKit/build/normal'
> 
> Leftover from other patches?

Actually, mediaControlsGtk.css stylesheet was not included in the patch, resulting in these errors.

> >+static Color panelColor = 0;
> >+static Color sliderColor = 0;
> >+static Color sliderThumbColor = 0;
> >+
> >+// Names of these icons can vary because of text direction.
> >+static char* playButtonIconName = 0;
> >+static char* seekBackButtonIconName = 0;
> >+static char* seekForwardButtonIconName = 0;
> >+
> >+static int mediaIconSize = 16;
> >+static int mediaSliderHeight = 14;
> 
> This should be const. Also, I'd say these sizes should be calculated
> dynamically, maybe based on the size of the video element we are showing?

Sizes of the control panel, timeline container and control buttons are all set in the extra stylesheet that is provided by this patch. With all these sizes determined in that stylesheet, I think we can afford having hard-coded icon sizes and slider height. It wouldn't make sense to do any calculations based on the size of the video element.

> >+#if ENABLE(VIDEO)
> >+    initMediaStyling(gtk_rc_get_style(GTK_WIDGET(gtkContainer())));
> >+#endif
> 
> Do you actually need to use gtk_rc_get_style or would gtk_widget_get_style be
> enough?

Some testing showed that gtk_widget_get_style apparently returns style for the default 'Gnome' theme, while gtk_rc_get_style returns the currently used theme (Human, Dust, whatever theme is used).

> The progress bar does not seem to work at all in my testing? (Eg,
> http://people.mozilla.com/~vladimir/mdemos/video1.html).

With only this patch applied, it really doesn't work properly. To fix this, adjustSliderThumbSize should be implemented, setting some (again, we can afford a hard-coded one) value for height and width of a media slider thumb.

First patches included this implementation, but it was later requested to move it to another patch. I plan to upload a patch for that issue in near future.

> The controls seem to be over the video content, is this the intended behavior?
> The spec does not seem to say much about it though.

Controls should be positioned at the bottom of the video element, with height of 20 pixels. When mouse cursor moves out, the controls fade out, unless the video is paused, so that's not really a problem.

Regards,
Zan Dobersek

[Zan Dobersek]

Created attachment 40722 [details]
Media controls, the simple approach

Adds missing stylesheet file, defines some variables as constant.

[Zan Dobersek]

Created attachment 40723 [details]
Adjust the media slider thumb size

Sets the height and width of the media slider thumb.

[Alexander Butenko]

ping? :)

[Xan Lopez]

(In reply to comment #24)
> ping? :)

I'm testing the patch with r49901 and I can't get any video to reproduce. Is it just me?

[Zan Dobersek]

(In reply to comment #25)
> (In reply to comment #24)
> > ping? :)
> 
> I'm testing the patch with r49901 and I can't get any video to reproduce. Is it
> just me?

Using r50025 with both patches applied, the controls are working for me. Anything specific you want to reproduce?

Regards,
Zan Dobersek

[Benjamin Otte]

disclaimer: I want this in as this stuff mising is really making lots of videos unusable. So please get these patches in and take my comments as followups.


The control sizes should very likely not be hardcoded, in particular due to a11y concerns - you want to give people with the large-print gtk theme larger icons. From a quick look I'm not sure Webkit's core allows reloading CSS yet, though.

I've also wondered if we could put the icons into the CSS (using background-image or so) and avoid the whole icon rendering machinery in this patch, but I don't think that'll work, as the mute button icon depends on state?

I'm also wondering how well it would work if we were to use gtk themeing functions to render controls - like gtk_paint_handle() for the slider thumb. This would get rid of quite some magic numbers while painting. On the other hand it might look crappy. Also, remaining magic numbers should likely be queried using gtk style properties, so people can script them.

Shouldn't the static variables be members of the Theme object? You might want to style different webviews differently. Also, you p[robably don't need to cache the names of the icons after loading, so the xxxButtonIconName variables can be local.

Gtk code usually uses GtkIconSet and gtk_icon_set_render_icon() to lookup icons. This requires a widget pointer, but gets around things like the text direction magic that the current code is doing, but it only gives you a GdkPixbuf, not a filename. But that shouldn't matter as we can easily create an Image from that.

[Adam Barth]

Checking style for patch 40722 from bug 26304.
Running check-webkit-style
WebCore/platform/graphics/gtk/ImageGtk.cpp:86:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Done processing WebCore/platform/graphics/gtk/ImageGtk.cpp
Done processing WebCore/platform/gtk/RenderThemeGtk.h
Done processing WebCore/platform/graphics/Image.h
Done processing WebCore/platform/gtk/RenderThemeGtk.cpp
Total errors found: 1

[Philippe Normand]

Created attachment 43786 [details]
fix controls patch compilation

It was broken since r51212.

[Philippe Normand]

So I tested the patches, two remarks :)

- mute and un-mute seem to be inverted here but this could be related with my setup and pulseaudio as I have been playing with mute on both Pulseaudio and our player on another patch
- the progress bar doesn't update. at all ;) Will have a look and see if I can fix that here

Otherwise it seems to work fine.

[Philippe Normand]

(In reply to comment #30)
> So I tested the patches, two remarks :)
> 
> - mute and un-mute seem to be inverted here but this could be related with my
> setup and pulseaudio as I have been playing with mute on both Pulseaudio and
> our player on another patch
> - the progress bar doesn't update. at all ;) Will have a look and see if I can
> fix that here
> 
> Otherwise it seems to work fine.

Sorry I didn't read comment 21 mentioning the progress bar will be enabled in another patch ;)

[Philippe Normand]

The progress bar updates fine for local files and some media containers served over http. For instance it works with mov files but not with ogg (because oggdemux doesn't report the stream duration if it is configured in push-mode).

Anyway, here are some good tips&tricks to configure your webserver for ogg streaming ;) http://pearce.org.nz/2009/08/configuring-web-servers-for-html5-ogg.html

So I would say this set of patches does the job well :) We will improve it later (slider for volume, maybe a button for fullscreen mode,..)

[Adam Barth]

Attachment 40722 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gtk/ImageGtk.cpp:86:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Done processing WebCore/platform/graphics/gtk/ImageGtk.cpp
Done processing WebCore/platform/gtk/RenderThemeGtk.h
Done processing WebCore/platform/graphics/Image.h
Done processing WebCore/platform/gtk/RenderThemeGtk.cpp
Total errors found: 1

[Philippe Normand]

Created attachment 44416 [details]
rebased the 3 patches in one

This patch was written by Zan Dobersek. In addition I fixed the compilation errors and re-enabled some of the media tests involving the controls UI.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44416 [details] without any errors.

[Oliver Hunt]

Comment on attachment 44416 [details]
rebased the 3 patches in one

I don't like this patch -- the RenderThemeGtk changes just seem bad.

getIconNameForTextDirection leaks the character data underlying the string

All the global resources (panelColor, playButtonIconName, muteButton, etc) should not be global and instead should be hung off the RenderThemeGtk object

The use of PassRefPtr is (as the name suggests) wrong for storing a reference to an object -- PassRefPtr will clear itself once the underlying object is retrieved -- you should use RefPtr.

Honestly I also believe that webkit/gtk would greatly benefit from adding a C++ wrapper for gtk objects that does automatic refcounting so you could more easily (for instance) just use gstring's everywhere, and also so you can remove destructors that are simply there to manually deref objects.

--Oliver

[Oliver Hunt]

Comment on attachment 40722 [details]
Media controls, the simple approach

This looks to simply be part of the patch i just reviewed

[Oliver Hunt]

Comment on attachment 40723 [details]
Adjust the media slider thumb size

This patch looks good, but it has been rolled into the other patch that I r-'d -- Do you want to keep this separate or should I r+? (it doesn't seem to make much sense on its own)

[Philippe Normand]

Created attachment 44604 [details]
rebased the 3 control patches into one.

I applied the tweaks suggested by Olivier and also fixed some coding
style issues.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44604 [details] without any errors.

[Philippe Normand]

Created attachment 45088 [details]
rebased the 3 control patches into one.

After discussion with Xan I updated getThemeIconFileName() to avoid
making it return an empty string. And I also reverted the
g_string_free() change.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45088 [details] without any errors.

[Xan Lopez]

Comment on attachment 45088 [details]
rebased the 3 control patches into one.

Go!

[Philippe Normand]

Landed as r52266.

[Eric Seidel (no email)]

Comment on attachment 40723 [details]
Adjust the media slider thumb size

This bug is closed, so there shouldn't be any r=? patches on it.  If this one still needs review, please file a new bug.  This bug is way too long as is. :)