[webkit-reviews] review denied: [Bug 26304] [GTK] Add controls for playing html5 video. : [Attachment 40178] Media controls, the simple approach

[bugzilla-daemon at webkit.org]

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 40178: Media controls, the simple approach
https://bugs.webkit.org/attachment.cgi?id=40178&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
>@@ -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.