Bug 83869 – [Gtk] HTML5 Media controls require a design refresh

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

HTML5 Media controls can end up rather strange-looking because the current design relies heavily on specific theme colors and stock icons. Instead of that, I recommend a couple of things for starters:
- make the media controls stylable through GtkStyleContext (i.e. theme designers can directly affect the controls look through Gtk+ themes)
- look up possible designs with the GNOME Design Team

[Xabier Rodríguez Calvar]

Created attachment 179135 [details]
Patch

Patch WiP. It creates new Gtk elements based con Chromium instead of Apple and forks Chromium controls to be more GNOME friendly. Forward and rewind buttons are removed. Fullscreen is hidden as it is disabled because of GStreamer 1.0. Next steps: fixing the timeline slider, fixing the volume thumb, adjusting minor things...

[Philippe Normand]

Please note only the Native Fullscreen support is disabled with GStreamer 1.0. Not the fullscreen API.

If you enable the fullscreen API websetting the fullscreen button should still work and you'd get the video element resized to use all the window space. If this is not the case please file a new bug!

[Cosimo Cecchi]

One thing that was discussed on #webkit-gtk today about this bug is that eventually it would be nice if this had some sort of integration with the theme used for the rest of the desktop (e.g. Adwaita).
An idea that floated by is to have the desktop theme drop a CSS file in a known location (specifying e.g. the images to be used for the sliders or defining the colors for backgrounds), which WebKit would then load in addition to its default theme when constructing the controls.

[Martin Robinson]

(In reply to comment #3)
> One thing that was discussed on #webkit-gtk today about this bug is that eventually it would be nice if this had some sort of integration with the theme used for the rest of the desktop (e.g. Adwaita).
> An idea that floated by is to have the desktop theme drop a CSS file in a known location (specifying e.g. the images to be used for the sliders or defining the colors for backgrounds), which WebKit would then load in addition to its default theme when constructing the controls.

Well, if it's important to match the GTK+ theme, we can just have the GTK+ theme engine draw the controls. It would just be a matter of adding the "overlay" style class,  I think, as you told me in A Coruña.

[Cosimo Cecchi]

(In reply to comment #4)

> Well, if it's important to match the GTK+ theme, we can just have the GTK+ theme engine draw the controls. It would just be a matter of adding the "overlay" style class,  I think, as you told me in A Coruña.

Oh, could we? That would be the best solution indeed.
I was told that this used something completely different to render the controls, with its own web CSS stylesheet - I haven't looked at the code myself.

[Xabier Rodríguez Calvar]

Created attachment 180498 [details]
Patch

Implemented the slider.

[Martin Robinson]

Comment on attachment 180498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180498&action=review

Nice work! I've pointed out a few simple stylistic nits below. My main concern with this patch is that it seems to introduce an implicit dependency on the Gnome symbolic icon theme. What happens when you try to use this patch without the theme installed? WebKitGTK+ doesn't really depend on Gnome proper at the moment.

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:51
> +#if !OS(ANDROID)
> +PassRefPtr<MediaControls> MediaControls::create(Document* document)
> +{
> +    return MediaControlsGtk::createControls(document);
> +}
> +#endif

You shouldn't need to handle Android when writing code for the GTK+ port.

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:73
> +    RefPtr<MediaControlPanelEnclosureElement> enclosure = MediaControlPanelEnclosureElement::create(document);
> +
> +    RefPtr<MediaControlPanelElement> panel = MediaControlPanelElement::create(document);
> +
> +    ExceptionCode ec;

I think you could probably remove the extra newlines here.

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

Does GTK+/GStreamer support video tracks? If not this is dead code.

> Source/WebCore/html/shadow/MediaControlsGtk.h:53
> +    static PassRefPtr<MediaControlsGtk> createControls(Document*);
> +
> +    virtual void setMediaController(MediaControllerInterface*) OVERRIDE;
> +
> +    virtual void reset() OVERRIDE;
> +
> +    virtual void playbackStarted() OVERRIDE;
> +
> +    void changedMute() OVERRIDE;
> +
> +    virtual void updateCurrentTimeDisplay() OVERRIDE;
> +
> +    virtual void showVolumeSlider() OVERRIDE;

I think it's fine to have one delcaration per-line without any blank lines between them. That's fairly common in C++/WebKit code.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:511
> +    GRefPtr<GdkPixbuf> icon = getStockSymbolicIconForWidgetType(GTK_TYPE_CONTAINER, iconName,
> +                                                                gtkTextDirection(renderObject->style()->direction()),
> +                                                                gtkIconState(this, renderObject),
> +                                                                iconRect.width());

WebKit style is to online indent 4 spaces more once you are inside the parenthesis.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:568
> +    int borderRadius = trackRect.height() / 2;
> +    IntSize radii(borderRadius, borderRadius);

It makes sense to put this right before you use it rather than right here.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:591
> +        if (!rangeRect.isEmpty()) {
> +            context->fillRoundedRect(rangeRect, radii, radii, radii, radii, Color::gray, ColorSpaceDeviceRGB);
> +        }

In WebKit you don't use curly braces for blocks that are only one line.

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

Instead of aligning these, they should just be indented four more spaces.

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:920
> +    GdkPixbuf* icon = gtk_icon_info_load_symbolic_for_context(info, context, NULL,  NULL);

In WebKit we use 0 instead of NULL except for varargs terminators.

[Xabier Rodríguez Calvar]

Of course, I'll observe your comments as soon as possible, but my intention now is to write a first version of the patch, without caring a lot of the style and other important things, so I think we can wait a bit for the final review until the patch is really ready, because we will need to change a lot of things on the way.

(In reply to comment #7)
> (From update of attachment 180498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180498&action=review
> 
> Nice work! I've pointed out a few simple stylistic nits below. My main concern with this patch is that it seems to introduce an implicit dependency on the Gnome symbolic icon theme. What happens when you try to use this patch without the theme installed? WebKitGTK+ doesn't really depend on Gnome proper at the moment.

It is included as a new jhbuild dependency.

I'll have a deeper look at the rest. Thanks for caring!

[Martin Robinson]

Yes, but but end users do not use our jhbuild. If the icon package is not installed do button icons appear? If not we either need to fall back gracefully or find another solution like bundling the icons.

[Xabier Rodríguez Calvar]

(In reply to comment #9)
> Yes, but but end users do not use our jhbuild. If the icon package is not installed do button icons appear? If not we either need to fall back gracefully or find another solution like bundling the icons.

I guess it will happen as it happens with the rest of the icons, that they won't be found if the corresponding package is not installed. I think we are currently having a similar problem with the other icons if I am not mistaken. Anyway, we could try to check for the package to be installed for more security or as you also say, we could bundle the icons too.

[Martin Robinson]

Yeah, if there's a chance it might not work, we either need to depend on a new package in configure.ac or bundle the icons. Bundling might be easier if the license works. Thanks for looking into it!

[Xabier Rodríguez Calvar]

(In reply to comment #11)
> Yeah, if there's a chance it might not work, we either need to depend on a new package in configure.ac or bundle the icons. Bundling might be easier if the license works. Thanks for looking into it!

I filed this bug and patch because there is no way to depend on the gnome-icon-theme-symbolic on configure.ac https://bugzilla.gnome.org/show_bug.cgi?id=690758

[Martin Robinson]

(In reply to comment #12)
> (In reply to comment #11)
> > Yeah, if there's a chance it might not work, we either need to depend on a new package in configure.ac or bundle the icons. Bundling might be easier if the license works. Thanks for looking into it!
> 
> I filed this bug and patch because there is no way to depend on the gnome-icon-theme-symbolic on configure.ac https://bugzilla.gnome.org/show_bug.cgi?id=690758

I think I'd prefer to not depend on more Gnome packages. Right now WebKitGTK+ is independent of Gnome to some extent, in that we only depend on some core libraries. Is there any way to check at runtime whether or not you loaded a valid icon and then fall back to some other, perhaps bundled, one?

[Xabier Rodríguez Calvar]

Created attachment 181889 [details]
Moved the slider thumb also to the volume slider, implemented some of Martin's comments, and added gnome-theme-icon and -symbolic as dependencies when compiling.

[Philippe Normand]

(In reply to comment #14)
> Created an attachment (id=181889) [details]
> Moved the slider thumb also to the volume slider, implemented some of Martin's comments, and added gnome-theme-icon and -symbolic as dependencies when compiling.

In comment #13 Martin asks to not depend on the gnome-theme-icon explicitely and this is what this patch does. What is going on? :)

[Xabier Rodríguez Calvar]

(In reply to comment #15)
> (In reply to comment #14)
> > Created an attachment (id=181889) [details] [details]
> > Moved the slider thumb also to the volume slider, implemented some of Martin's comments, and added gnome-theme-icon and -symbolic as dependencies when compiling.
> 
> In comment #13 Martin asks to not depend on the gnome-theme-icon explicitely and this is what this patch does. What is going on? :)

I was discussing privately with him, and this is a tricky question, because the icons are bundled with webkit and there is not even a dependency because they are not linked in anyway to webkit, but they are used and loaded in runtime, which can fail. I included it because we do need the symbolic icons to function properly, though I also included the stock icons as fallback. To solve this properly we should avoid this dependency and add an 'runtime' dependency which is what we have here, but AFAIK, it is something that has to be done at packaging level.

I added Carlos to the CC because he knows whether stock icons are always available in GTK, but I don't know about the symbolic ones.

[Xabier Rodríguez Calvar]

Created attachment 182087 [details]
Screenshot of the current state

For the people who don't want to compile the current patch now.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.list.am', u'Sou..." exit_code: 1
Source/WebCore/platform/gtk/RenderThemeGtk.h:200:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/gtk/RenderThemeGtk.cpp:62:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/gtk/RenderThemeGtk.cpp:506:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/gtk/RenderThemeGtk.cpp:533:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/gtk/RenderThemeGtk.cpp:547:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/gtk/RenderThemeGtk.cpp:554:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/gtk/RenderThemeGtk.cpp:560:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:881:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:882:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:896:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:906:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 11 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xabier Rodríguez Calvar]

Created attachment 183200 [details]
Patch

Reworked the volume slider track, now it looks like the time slider.

[Xabier Rodríguez Calvar]

Created attachment 183202 [details]
Screenshot

[Martin Robinson]

Comment on attachment 183200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183200&action=review

Okay. Here are some comment on the patch. It's looking good so far.

> Source/WebCore/css/mediaControlsGtk.css:28
> +/* Chromium default media controls */

Probably want to write "These are based on the Chromium media controls." or something to that effect. :)

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:34
> +using namespace std;

Instead of "using namespace std;" you should just say "std::" everywhere: http://www.webkit.org/coding/coding-style.html (see section 3 under "using" statements)

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:45
> +// MediaControls::create() for Android is defined in MediaControlsGtkAndroid.cpp.

Probably can just delete this comment since it doesn't apply to us.

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:229
> +    // In the Chromium case, that's the enclosure element.

Probably want to remove the part of the comment mentioning Chromium.

> Source/WebCore/platform/graphics/IntRect.cpp:132
> +void IntRect::inflateToSize(const IntSize &newSize)
> +{
> +    inflateX((newSize.width() - m_size.width()) / 2);
> +    inflateY((newSize.height() - m_size.height()) / 2);
> +}
> +

I think the semantics of this are different enough that we shouldn't add it. Instead I think you could do something like:

rect.expand(newSize - rect.size());

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:64
> +extern GRefPtr<GdkPixbuf> getStockSymbolicIconForWidgetType(GType widgetType, const char* symbolicIconName,
> +                                                            const char *fallbackStockIconName, gint direction,
> +                                                            gint state, gint iconSize);
>  

Put this one one line per WebKit style.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:506
> +bool RenderThemeGtk::paintMediaButton(RenderObject* renderObject, GraphicsContext* context, const IntRect& rect,
> +                                      const char* symbolicIconName, const char* fallbackStockIconName)

Ditto.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:515
> +    GRefPtr<GdkPixbuf> icon = getStockSymbolicIconForWidgetType(GTK_TYPE_CONTAINER, symbolicIconName,
> +        fallbackStockIconName,
> +        gtkTextDirection(renderObject->style()->direction()),
> +        gtkIconState(this, renderObject),
> +        iconRect.width());

Okay. This is a nit, but the inconsistency of using a new line for each parameter except the second drive me crazy! :)

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:534
> +                            muted ? "audio-volume-muted-symbolic" : "audio-volume-high-symbolic",
> +                            muted ? "audio-volume-muted" : "audio-volume-high");

These should only be indented 8 spaces.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:548
> +                            play ? "media-playback-start-symbolic" : "media-playback-pause-symbolic",
> +                            play ? GTK_STOCK_MEDIA_PLAY : GTK_STOCK_MEDIA_PAUSE);

Ditto.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:554
> +                            GTK_STOCK_MEDIA_REWIND);

This can just be one line.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:560
> +                            GTK_STOCK_MEDIA_FORWARD);

Ditto.

> Source/WebCore/platform/gtk/RenderThemeGtk.h:200
> +    bool paintMediaButton(RenderObject*, GraphicsContext*, const IntRect&, const char* symbolicIconName,
> +                          const char* fallbackStockIconName);

This should just be one line.

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:683
> +    IntRect newRect = rect;
> +    newRect.inflateToSize(IntSize(12, 12));
> +    return paintMediaSliderThumb(renderObject, paintInfo, newRect);

It seems a bit weird to paint all <input type="range"> widgets like the media controls. Surely this is just an oversight?

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:882
> +GRefPtr<GdkPixbuf> getStockSymbolicIconForWidgetType(GType widgetType, const char* symbolicIconName,
> +                                                     const char *fallbackStockIconName, gint direction, gint state,
> +                                                     gint iconSize)

This should be one line per WebKit style.

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:896
> +    GtkIconInfo* info = gtk_icon_theme_lookup_icon (gtk_icon_theme_get_default(),

You have an extra space after the function name.

> configure.ac:916
> +PKG_CHECK_MODULES([ICONS], [gnome-icon-theme gnome-icon-theme-symbolic])

Please remove this line.

[Xabier Rodríguez Calvar]

Created attachment 183573 [details]
Patch

Fixed Martin's comments, including changing all sliders and the inflateToSize, that I thought that could be useful for other ports. expand() does not work because I want the thumb to remain centered in the same place. Anyway, I changed the way thumbs are painted and now they use the CSS style to be painted, though they are painted in C++. I also reactivated the fullscreen button and changed the font to Cantarell.

[Xabier Rodríguez Calvar]

Created attachment 183575 [details]
Screenshot

[Xabier Rodríguez Calvar]

Created attachment 184253 [details]
Patch

I reworked a bit the margins to make them more similar, even when some buttons disappear.

I created rebaselines for some tests and marked some others in the TestExpectations because they do not work in Chromium. I would create a bug and put it in the patch if the rest of the review is ok.

Martin, would you be so kind of reviewing the patch? If it is ok, we could create the bug for the new expectations and land it manually tomorrow as we need some actions done in the bots.

[Xabier Rodríguez Calvar]

Created attachment 184255 [details]
Screenshot

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk-wk2/fast/hidpi/video-controls-in-hidpi-expected.txt', u'LayoutTests/platform/gtk-wk2/fast/layers/video-layer-expected.txt', u'LayoutTests/platform/gtk-wk2/media/audio-controls-rendering-expected.txt', u'LayoutTests/platform/gtk-wk2/media/audio-repaint-expected.txt', u'LayoutTests/platform/gtk-wk2/media/media-controls-clone-expected.txt', u'LayoutTests/platform/gtk-wk2/media/video-empty-source-expected.txt', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/gtk/accessibility/media-element-expected.txt', u'LayoutTests/platform/gtk/fast/hidpi/video-controls-in-hidpi-expected.txt', u'LayoutTests/platform/gtk/fast/layers/video-layer-expected.txt', u'LayoutTests/platform/gtk/media/audio-controls-rendering-expected.txt', u'LayoutTests/platform/gtk/media/audio-repaint-expected.txt', u'LayoutTests/platform/gtk/media/controls-after-reload-expected.txt', u'LayoutTests/platform/gtk/media/controls-strict-expected.txt', u'LayoutTests/platform/gtk/media/controls-styling-expected.txt', u'LayoutTests/platform/gtk/media/controls-styling-strict-expected.txt', u'LayoutTests/platform/gtk/media/controls-without-preload-expected.txt', u'LayoutTests/platform/gtk/media/media-controls-clone-expected.txt', u'LayoutTests/platform/gtk/media/video-controls-rendering-expected.txt', u'LayoutTests/platform/gtk/media/video-display-toggle-expected.txt', u'LayoutTests/platform/gtk/media/video-empty-source-expected.txt', u'LayoutTests/platform/gtk/media/video-no-audio-expected.txt', u'LayoutTests/platform/gtk/media/video-volume-slider-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/css/mediaControlsGtk.css', u'Source/WebCore/html/shadow/MediaControlsGtk.cpp', u'Source/WebCore/html/shadow/MediaControlsGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk3.cpp', u'Tools/ChangeLog', u'Tools/gtk/jhbuild.modules']" exit_code: 1
LayoutTests/platform/gtk/TestExpectations:446:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:447:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:448:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:449:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:450:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:451:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 6 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 184253 [details]
Patch

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.

[Martin Robinson]

I think I know why you are having so many issues with the CSS. In the Chromium controls (which these controls are derived from) the volume slider is horizontal, like the timeline slider. This means that many of the CSS properties for the timeline are also applicable to the volume slider. In the case of our controls, the volume slider is vertical, so the right tactic in the CSS file is to ensure that properties useful for horizontal sliders do not apply to the volume slider to begin with. I'm exploring this locally.

[Xabier Rodríguez Calvar]

Created attachment 184558 [details]
Patch

(In reply to comment #27)
> > 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.

This is needed because the component is used and if it is not created, the engine will crash.

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

I couldn't make the rectangle to be less than 20x16. By playing with the box-sizing: content-box I could make it bigger, but not smaller, so there must be something missing. In this version of the patch no changes are done to the rect in C++ so that it can be tested.

The rest of comments are already changed in this version of the patch. I didn't mark the review because the padding stuff needs to be finished.

[Philippe Normand]

(In reply to comment #27)
> (From update of attachment 184253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184253&action=review
> 
> 
> > 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.
> 

The VIDEO_TRACK feature is, AFAIK, cross-platform. The basic feature should work out of the box. Then there are indeed issues to fix like embedded track advertizing support but it's totally unrelated with this bug.

[Xabier Rodríguez Calvar]

Created attachment 185276 [details]
Patch

Done some changes with Martin's help, filled the volume slider track up to the volume level and reworked the tests (new baselines...). If the patch is ok, I can ask for the c? tomorrow in order to be able to watch the bots.

[Martin Robinson]

Comment on attachment 185276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185276&action=review

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:621
> +        volumeRect.setHeight(ceil(trackHeight));

If I'm not mistaken casting a float to an int does an implicit ceil.

[Xabier Rodríguez Calvar]

Created attachment 185278 [details]
Screenshot

[Xabier Rodríguez Calvar]

(In reply to comment #32)
> (From update of attachment 185276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185276&action=review
> 
> > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:621
> > +        volumeRect.setHeight(ceil(trackHeight));
> 
> If I'm not mistaken casting a float to an int does an implicit ceil.

It does an implicit floor, AFAIK and it was confirmed by the code I had before without the ceil because I had a little flickering at the bottom of the volume track because it was not reaching the end.

[Xabier Rodríguez Calvar]

Created attachment 187534 [details]
Patch

This version has pixel tests rebaslines, gnome is set as icon theme for the tests and fixes the regular sliders problem that were broken with the former versions of the patch. Phil, you are the one to review, I think, though Martin can also have a look even being co-author. I'll request the cq? when I have a moment to care about the bots.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/gtk/accessibility/media-element-expected.txt', u'LayoutTests/platform/gtk/fast/hidpi/video-controls-in-hidpi-expected.txt', u'LayoutTests/platform/gtk/fast/layers/video-layer-expected.png', u'LayoutTests/platform/gtk/fast/layers/video-layer-expected.txt', u'LayoutTests/platform/gtk/http/tests/media/video-buffered-range-contains-currentTime-expected.png', u'LayoutTests/platform/gtk/media/audio-controls-rendering-expected.txt', u'LayoutTests/platform/gtk/media/audio-repaint-expected.png', u'LayoutTests/platform/gtk/media/audio-repaint-expected.txt', u'LayoutTests/platform/gtk/media/controls-after-reload-expected.txt', u'LayoutTests/platform/gtk/media/controls-strict-expected.txt', u'LayoutTests/platform/gtk/media/controls-styling-strict-expected.png', u'LayoutTests/platform/gtk/media/controls-styling-strict-expected.txt', u'LayoutTests/platform/gtk/media/controls-without-preload-expected.txt', u'LayoutTests/platform/gtk/media/video-controls-rendering-expected.png', u'LayoutTests/platform/gtk/media/video-controls-rendering-expected.txt', u'LayoutTests/platform/gtk/media/video-display-toggle-expected.txt', u'LayoutTests/platform/gtk/media/video-empty-source-expected.txt', u'LayoutTests/platform/gtk/media/video-no-audio-expected.txt', u'LayoutTests/platform/gtk/media/video-volume-slider-expected.txt', u'LayoutTests/platform/gtk/media/video-zoom-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/css/mediaControlsGtk.css', u'Source/WebCore/html/shadow/MediaControlsGtk.cpp', u'Source/WebCore/html/shadow/MediaControlsGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk2.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk3.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/gtk/DumpRenderTree.cpp', u'Tools/gtk/jhbuild.modules']" exit_code: 1
Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:176:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xabier Rodríguez Calvar]

(In reply to comment #36)
> Attachment 187534 [details] did not pass style-queue:
> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:176:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 1 in 29 files

I kept this for coherence with the rest of the file.

[Xabier Rodríguez Calvar]

Created attachment 187540 [details]
Patch

Changed wrong changelog.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/gtk/accessibility/media-element-expected.txt', u'LayoutTests/platform/gtk/fast/hidpi/video-controls-in-hidpi-expected.txt', u'LayoutTests/platform/gtk/fast/layers/video-layer-expected.png', u'LayoutTests/platform/gtk/fast/layers/video-layer-expected.txt', u'LayoutTests/platform/gtk/http/tests/media/video-buffered-range-contains-currentTime-expected.png', u'LayoutTests/platform/gtk/media/audio-controls-rendering-expected.txt', u'LayoutTests/platform/gtk/media/audio-repaint-expected.png', u'LayoutTests/platform/gtk/media/audio-repaint-expected.txt', u'LayoutTests/platform/gtk/media/controls-after-reload-expected.txt', u'LayoutTests/platform/gtk/media/controls-strict-expected.txt', u'LayoutTests/platform/gtk/media/controls-styling-strict-expected.png', u'LayoutTests/platform/gtk/media/controls-styling-strict-expected.txt', u'LayoutTests/platform/gtk/media/controls-without-preload-expected.txt', u'LayoutTests/platform/gtk/media/video-controls-rendering-expected.png', u'LayoutTests/platform/gtk/media/video-controls-rendering-expected.txt', u'LayoutTests/platform/gtk/media/video-display-toggle-expected.txt', u'LayoutTests/platform/gtk/media/video-empty-source-expected.txt', u'LayoutTests/platform/gtk/media/video-no-audio-expected.txt', u'LayoutTests/platform/gtk/media/video-volume-slider-expected.txt', u'LayoutTests/platform/gtk/media/video-zoom-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/css/mediaControlsGtk.css', u'Source/WebCore/html/shadow/MediaControlsGtk.cpp', u'Source/WebCore/html/shadow/MediaControlsGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk2.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk3.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/gtk/DumpRenderTree.cpp', u'Tools/gtk/jhbuild.modules']" exit_code: 1
Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:176:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 187540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187540&action=review

Looks good to me. A couple of nits, but I think this is fine.

> Source/WebCore/ChangeLog:1
> [phone]  Xabier Rodriguez Calvar  <[e-mail]>  Martin Robinson  <[e-mail]>

I think maybe we need an 'and' here.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:611
> +    float volume = mediaElement->volume();
> +    if (volume) {

This should probably be an early return.

[Xabier Rodríguez Calvar]

Created attachment 188404 [details]
Patch

Changes following Martin's comments.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/gtk/accessibility/media-element-expected.txt', u'LayoutTests/platform/gtk/fast/hidpi/video-controls-in-hidpi-expected.txt', u'LayoutTests/platform/gtk/fast/layers/video-layer-expected.png', u'LayoutTests/platform/gtk/fast/layers/video-layer-expected.txt', u'LayoutTests/platform/gtk/http/tests/media/video-buffered-range-contains-currentTime-expected.png', u'LayoutTests/platform/gtk/media/audio-controls-rendering-expected.txt', u'LayoutTests/platform/gtk/media/audio-repaint-expected.png', u'LayoutTests/platform/gtk/media/audio-repaint-expected.txt', u'LayoutTests/platform/gtk/media/controls-after-reload-expected.txt', u'LayoutTests/platform/gtk/media/controls-strict-expected.txt', u'LayoutTests/platform/gtk/media/controls-styling-strict-expected.png', u'LayoutTests/platform/gtk/media/controls-styling-strict-expected.txt', u'LayoutTests/platform/gtk/media/controls-without-preload-expected.txt', u'LayoutTests/platform/gtk/media/video-controls-rendering-expected.png', u'LayoutTests/platform/gtk/media/video-controls-rendering-expected.txt', u'LayoutTests/platform/gtk/media/video-display-toggle-expected.txt', u'LayoutTests/platform/gtk/media/video-empty-source-expected.txt', u'LayoutTests/platform/gtk/media/video-no-audio-expected.txt', u'LayoutTests/platform/gtk/media/video-volume-slider-expected.txt', u'LayoutTests/platform/gtk/media/video-zoom-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/css/mediaControlsGtk.css', u'Source/WebCore/html/shadow/MediaControlsGtk.cpp', u'Source/WebCore/html/shadow/MediaControlsGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk2.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk3.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/gtk/DumpRenderTree.cpp', u'Tools/gtk/jhbuild.modules']" exit_code: 1
Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:176:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 188404 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188404&action=review

> Source/WebCore/css/mediaControlsGtk.css:53
> +    -webkit-box-orient: vertical;
> +    -webkit-box-pack: end;
> +    -webkit-box-align: center;

Have you seen this thread on webkit-dev?
[webkit-dev] Ports: Converting media controls to -webkit-flex
http://trac.webkit.org/changeset/142947

[Xabier Rodríguez Calvar]

Comment on attachment 188404 [details]
Patch

(In reply to comment #43)
> Have you seen this thread on webkit-dev?
> [webkit-dev] Ports: Converting media controls to -webkit-flex
> http://trac.webkit.org/changeset/142947

Thanks for the review, Phil. I need to care about this anyway because the tests have a new rebaseline in trunk and I need to check against that and work out the css too.

[Xabier Rodríguez Calvar]

Created attachment 189185 [details]
Patch

Rebased against the trunk and new baselines for the changing tests.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/gtk/accessibility/media-element-expected.txt', u'LayoutTests/platform/gtk/fast/hidpi/video-controls-in-hidpi-expected.txt', u'LayoutTests/platform/gtk/fast/layers/video-layer-expected.png', u'LayoutTests/platform/gtk/fast/layers/video-layer-expected.txt', u'LayoutTests/platform/gtk/http/tests/media/video-buffered-range-contains-currentTime-expected.png', u'LayoutTests/platform/gtk/media/audio-repaint-expected.png', u'LayoutTests/platform/gtk/media/audio-repaint-expected.txt', u'LayoutTests/platform/gtk/media/controls-styling-strict-expected.png', u'LayoutTests/platform/gtk/media/video-controls-rendering-expected.png', u'LayoutTests/platform/gtk/media/video-empty-source-expected.txt', u'LayoutTests/platform/gtk/media/video-no-audio-expected.txt', u'LayoutTests/platform/gtk/media/video-volume-slider-expected.txt', u'LayoutTests/platform/gtk/media/video-zoom-controls-expected.txt', u'LayoutTests/platform/gtk/media/video-zoom-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/css/mediaControlsGtk.css', u'Source/WebCore/html/shadow/MediaControlsGtk.cpp', u'Source/WebCore/html/shadow/MediaControlsGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk2.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk3.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/gtk/DumpRenderTree.cpp', u'Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp', u'Tools/gtk/jhbuild.modules']" exit_code: 1
LayoutTests/platform/gtk/media/video-zoom-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:176:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 189185 [details]
Patch

Clearing flags on attachment: 189185

Committed r143463: <http://trac.webkit.org/changeset/143463>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.