[Webkit-unassigned] [Bug 103151] [GStreamer] verify if GStreamer was previously initialized

[bugzilla-daemon at webkit.org]

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


Philippe Normand <pnormand at igalia.com> changed:

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




--- Comment #6 from Philippe Normand <pnormand at igalia.com>  2012-11-24 00:30:54 PST ---
(From update of attachment 175824)
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.

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