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

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro 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 319788: Patch

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




--- Comment #19 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 319788
  --> https://bugs.webkit.org/attachment.cgi?id=319788
Patch

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

Hi!

I have a lot of comments, but don't be discouraged: that's normal. Getting
patches into WebKit isn't easy.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:28
> +#include
"WebKit/WebProcess/InjectedBundle/API/gtk/DOM/ConvertToUTF8String.h"

There's a reason that the WebKit subproject directories are not in the include
path: WebCore is a lower layer. You can't use the WebKit subproject here. (In
fact, WebCore/platform is supposed to be a lower layer than the rest of
WebCore, too, though that's not enforced yet.) It looks like you don't use the
functions from this header anymore, since you found WTF::String::utf8, so you
can just remove it.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:153
> +#if ENABLE(VIDEO) && USE(GSTREAMER)

What if WebKit is built with USE(GSTREAMER) and ENABLE(WEB_AUDIO) but not
ENABLE(VIDEO)? Now GStreamer won't be initialized. I think you need to drop the
ENABLE(VIDEO) check here.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:163
> +    nArgv = g_new0(char*, argcSizeSplit+1);
> +
> +    for (guint i = 0; i < parameters.size(); i++)
> +	   nArgv[i] = g_strdup(parameters[i].utf8().data());

You need to free nArgv with g_strfreev().

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

You don't need argc here. Just pass &size to gst_init_check().

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

This would be nicer on one long line.

Anyway, this is a preexisting problem, but it doesn't make sense to use
ASSERT_WITH_MESSAGE() here because we really want this assert in release mode,
not just debug mode. We could use RELEASE_ASSERT(), but it would be easier to
just use gst_init() instead of gst_init_check(). The point of gst_init_check()
is to allow the application to gracefully handle a situation where GStreamer is
broken, but that's fatal for WebKit: we really just want to crash always. If
you make this change, then this function will need to be changed to return void
instead of a bool.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:177
>      return gstInitialized;
> +#endif

So now the function has no return value in the !ENABLE(VIDEO) ||
!USE(GSTREAMER) case. Easiest solution is to change the function to return void
like I suggest above, but alternatively you could declare gstInitialized
outside the conditional.

> Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:96
> +    gboolean isGStreamerParameters = false;

Use normal bool.

If you need to use gboolean, which is necessary when interacting with GNOME's C
APIs, then beware that it's an int rather than one byte, and use GLib's
TRUE/FALSE instead of C++'s true/false. But otherwise, it's best to use C++
bool.

> Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:99
> +    enum {
> +	   BufferSize = 2048
> +    };

Don't create an enum with only one member, just declare a constant:

const unsigned BufferSize = 2048;

> Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:105
> +    while (fgets(buffer, BufferSize, file)) {

I see you've tried to account for the command line being longer than 2048
characters, but it doesn't work: it misses any GStreamer options that might be
split over the boundary. So this is going to need a rethink. The standard
library fgets() makes this very difficult because it forces you to guess the
size of the buffer in advance. I'm going to suggest using g_file_get_contents()
instead of fopen(), that way you get the result in one string of the right size
allocated by Gio (remember to free it with g_free()) and everything will become
much simpler.

> Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:108
> +	       isGStreamerParameters = g_str_has_prefix(buffer + n, "--gst");

There's actually no reason to filter out parameters here. Just pass everything
on to GStreamer. Let GStreamer figure out for itself what to do with the
options.

> Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:111
> +		   char* var = &buffer[n];
> +		   parameters.gstreamerOptions.append(String::fromUTF8(var));

In WebKit we prefer to avoid unnecessary local variables unless it
significantly improves readability. In this case it's better to do it in one
line: parameters.gstreamerOptions.append(String::fromUTF8(&buffer[n]).

> Source/WebKit/WebProcess/soup/WebProcessSoup.cpp:30
> +#include "WebCore/platform/graphics/gstreamer/GStreamerUtilities.h"

We don't use full paths in WebKit, see
https://webkit.org/code-style-guidelines/#include-statements. Just #include
<GStreamerUtilities.h>. Add WebCore/platform/graphics/gstreamer to the include
path in PlatformGTK.cmake and PlatformWPE.cmake if you need to.