[Webkit-unassigned] [Bug 39474] [GStreamer] GTK XOverlay support in GStreamerGWorld

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39474


Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #62047|review?                     |review-
               Flag|                            |




--- Comment #28 from Gustavo Noronha (kov) <gns at gnome.org>  2010-08-02 15:44:04 PST ---
(From update of attachment 62047)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.