[webkit-reviews] review granted: [Bug 191189] [GStreamer] Move elements registration to GStreamerCommon : [Attachment 353690] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 191189: [GStreamer] Move elements registration to GStreamerCommon
https://bugs.webkit.org/show_bug.cgi?id=191189

Attachment 353690: Patch

https://bugs.webkit.org/attachment.cgi?id=353690&action=review




--- Comment #2 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 353690
  --> https://bugs.webkit.org/attachment.cgi?id=353690
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:53
> +#if ENABLE(VIDEO)

Shouldn't this enclose more of the includes above?

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
-152
> -GST_DEBUG_CATEGORY(webkit_media_player_debug);
> +GST_DEBUG_CATEGORY_EXTERN(webkit_media_player_debug);
>  #define GST_CAT_DEFAULT webkit_media_player_debug
>  
>  
>  namespace WebCore {
>  using namespace std;
>  
> -bool
MediaPlayerPrivateGStreamerBase::initializeGStreamerAndRegisterWebKitElements()
> -{
> -    if (!initializeGStreamer())
> -	   return false;
> -
> -    static std::once_flag onceFlag;
> -    std::call_once(onceFlag, [] {
> -	   GST_DEBUG_CATEGORY_INIT(webkit_media_player_debug,
"webkitmediaplayer", 0, "WebKit media player");

This is conceptually weird for me. The category variable should "live" in
Base.cpp and be initialized here since it's the superclass. I understand you
really want to get rid of this method but IMHO, I think we should keep a very
small version of it because of the initialization of this variable, keep the
_CATEGORY( here and have the _CATEGORY_EXTERN( in the subclass.