[webkit-reviews] review denied: [Bug 173655] [GStreamer][MiniBrowser] Honor GStreamer command line parameters in MiniBrowser : [Attachment 313727] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Vanessa Chipirrás
Navalón <vchipirras at igalia.com>'s request for review:
Bug 173655: [GStreamer][MiniBrowser] Honor GStreamer command line parameters in
MiniBrowser
https://bugs.webkit.org/show_bug.cgi?id=173655

Attachment 313727: Patch

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




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

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

> Source/WebCore/ChangeLog:12
> +	   (WebCore::initializeGStreamer): Here gets the Gstreamer's option
from

GStreamer should be spelled like this.

> Source/WebCore/ChangeLog:13
> +	   the main() and create the char "argv" to Initialize the GStreamer
library.

"the main() function" or just "main()". Initialized does not need to be
capitalized.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:155
> +

Please remove this empty line.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:156
> +    const gchar* gstArgv = g_getenv("GST_ARGUMENTS");

const char* gstreamerArgumentsString

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:157
> +    bool gstInitialized = false;

bool isGStreamerInitialized

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:160
> +

Please remove this empty line.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:161
> +	   gchar ** argvSplit = g_strsplit(gstArgv, " ", -1);

char** splitGStreamerArguments

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:162
> +	   guint argcSizeSplit = g_strv_length(argvSplit);

unsigned gstreamerArgumentsSize

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:164
> +	   char ** nArgv = g_new0(char*, argcSizeSplit+1);

char** argv = g_new0(char*, gstreamerArgumentsSize + 1);

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:167
> +	   for (guint i = 0; i < (argcSizeSplit+1); i++)

unsigned i

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:173
> +	   argcSizeSplit++;
> +	   int* argc = (int*)(&argcSizeSplit);
> +
> +	   gstInitialized = gst_init_check(argc, &nArgv, &error.outPtr());

int argc = gstreamerArgumentsSize + 1;
isGStreamerInitialized = gst_init_check(&argc, &argv, &error.outPtr());

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:175
> +	   ASSERT_WITH_MESSAGE(gstInitialized, "GStreamer initialization
failed: %s", error ? error->message :
> +	   "unknown error occurred");

As we allow long lines, you can make this so.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:176
> +

Please remove this empty line.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:184
> +

Please remove this empty line.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:187
> +    ASSERT_WITH_MESSAGE(gstInitialized, "GStreamer initialization failed:
%s", error ? error->message :
> +    "unknown error occurred");

ditto

> Tools/ChangeLog:12
> +	   I need to use them in this part of the project to returns 

there's a typo "to returns" -> "to return".

> Tools/ChangeLog:15
> +	   (main): Detect the Gstreamer's input options to pass 

GStreamer is the proper capitalization.

> Tools/ChangeLog:16
> +	   them to the GStreamer initialization function.

ditto.

> Tools/MiniBrowser/gtk/main.c:471
> +    GString *strbuf = g_string_new(NULL);

Please use WTF::StringBuilder and name it stringBuilder (WK coding style: full
words as variable names)

> Tools/MiniBrowser/gtk/main.c:472
> +    gboolean gstParam = false;

Please use bool and name the variable as gstreamerParameters.

> Tools/MiniBrowser/gtk/main.c:482
> +	   gchar * arguments = g_string_free(strbuf, FALSE);

I suspect you won't need this line with WTF::StringBuilder but you should have
used char instead gchar and placing the * next to the type: char* arguments;

> Tools/MiniBrowser/gtk/main.c:491
> +    g_option_context_add_group(context, gst_init_get_option_group());

Doc says: If you use this function, you should make sure you initialise the
GLib threading system as one of the very first things in your program (see the
example at the beginning of this section). I guess you should call gst_init
here which is going to be fun as it will steal your GStreamer parameter and
you'll need to copy them first.