[webkit-reviews] review denied: [Bug 103151] [GStreamer] verify if GStreamer was previously initialized : [Attachment 175824] Patch

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Víctor M. Jáquez L.
<vjaquez at igalia.com>'s request for review:
Bug 103151: [GStreamer] verify if GStreamer was previously initialized
https://bugs.webkit.org/show_bug.cgi?id=103151

Attachment 175824: Patch
https://bugs.webkit.org/attachment.cgi?id=175824&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=175824&action=review


Looks good overall, have a look at the other ChangeLog entries to see how
they're structured :)

> Source/WebCore/ChangeLog:5
> +	   Verify if GStreamer was previously initialized
> +
> +	   The patch also contains the proof of concept for GtkLaunch

Please move this down the Reviewed by line.
Also it's GtkLauncher. And maybe you can explain why the same is not done for
the WK2 MiniBrowser.

> Source/WebCore/ChangeLog:12
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

This one should be replaced by the 2 lines above.

> Source/WebCore/ChangeLog:14
> +	   No new tests (OOPS!).

Usually for a small patch like this without any impact on the tests we state
something like "No new tests, existing media tests cover this change."

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:32
> +    if (gst_is_initialized ())

We don't allow space between the function name and the parenthesis, usually.
Maybe the style bot can check that.
Also as this is an API introduced in 0.10.31 and we require 0.10.30 in
configure.ac it'd be nice to wrap this code in a #if GST_CHECK_VERSION(0, 10,
31) block.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:33
> +	   return true;

A blank line wouldn't hurt after this one :)

> Tools/ChangeLog:5
> +	   Verify if GStreamer was previously initialized
> +
> +	   The patch also contains the proof of concept for GtkLaunch

Well the same apply to this ChangeLog and please don't copy-paste from the
other one :) It'd be fine if this entry mentions only the change related to the
GtkLauncher.