[webkit-reviews] review denied: [Bug 26304] [GTK] Add controls for playing html5 video. : [Attachment 38456] Media controls impl. 2.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 24 23:58:00 PDT 2009
Xan Lopez <xan.lopez 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 38456: Media controls impl. 2.
https://bugs.webkit.org/attachment.cgi?id=38456&action=review
------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
> +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 <zandobersek at gmail.com>
> + *
> + * 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.
More information about the webkit-reviews
mailing list