[webkit-reviews] review denied: [Bug 39474] [GStreamer] GTK XOverlay support in GStreamerGWorld : [Attachment 62047] fullscreen video support in WebKit/gtk

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 39474: [GStreamer] GTK XOverlay support in GStreamerGWorld
https://bugs.webkit.org/show_bug.cgi?id=39474

Attachment 62047: fullscreen video support in WebKit/gtk
https://bugs.webkit.org/attachment.cgi?id=62047&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
WebKit/gtk/WebCoreSupport/FullscreenVideoController.cpp:571
 +	g_object_set(m_exitFullscreenAction, "icon-name",
"view-restore-symbolic", NULL);
symbolic icons are quite new, are we going to have to shift our dependencies
for this?

 294	 if (!node->hasTagName(HTMLNames::videoTag))
 295	     return;
 296 
 297 #if ENABLE(VIDEO)

Might as well only do the check if video is enabled? Otherwise it's just
useless work.

311 void webkit_web_view_exit_fullscreen(WebKitWebView* webView)
 312 {
 313 #if ENABLE(VIDEO)
 314	 WebKitWebViewPrivate* priv = webView->priv;
 315	 if (priv->fullscreenVideoController)
 316	     priv->fullscreenVideoController->exitFullscreen();
 317 #endif

Certainly a good idea to NULLify the fullscreenVideoController here. Also, I
believe you need to exit fullscreen in case a fullscreenVideoController exists
during WebKitWebView's destruction? The callbacks receiving C++ objects as user
data still concern me, I'm pretty sure we'll get crashes from the object going
away and the callback being called right after that.

These are my last concerns, I think, the rest of the patch looks good, so let's
try to clear them up and land =D