[webkit-reviews] review denied: [Bug 54628] [GStreamer] Add 'location' property in webkitwebsrc : [Attachment 82776] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Feb 17 07:41:28 PST 2011
Martin Robinson <mrobinson at webkit.org> has denied Andoni <ylatuya at gmail.com>'s
request for review:
Bug 54628: [GStreamer] Add 'location' property in webkitwebsrc
https://bugs.webkit.org/show_bug.cgi?id=54628
Attachment 82776: Patch
https://bugs.webkit.org/attachment.cgi?id=82776&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82776&action=review
Looks good, but I have just a few small suggestions.
> WebCore/ChangeLog:14
> +2011-02-17 Andoni Morales Alastruey <amorales at flumotion.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Added 'location' property
> + https://bugs.webkit.org/show_bug.cgi?id=54628
> +
> + The 'location' property is used by gst_element_make_from_uri()
> + to set the uri in the source element
> +
> + * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
> + (webkit_web_src_class_init):
> + (webKitWebSrcSetProperty):
> + (webKitWebSrcGetProperty):
Please generate your ChangeLog with prepare-ChangeLog --bug <bugid>. Some of
the tools will unfotunately choke if the format is incorrect.
> WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:119
> +static gboolean webKitWebSrcSetUri(GstURIHandler* handler, const gchar*
uri);
> +static const gchar* webKitWebSrcGetUri(GstURIHandler* handler);
The argument names aren't necessary here.
> WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:206
> + /* location property for gst_element_make_from_uri */
Please use complete sentences for comments.
> WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:298
> + webKitWebSrcSetUri((GstURIHandler *)src, g_value_get_string(value));
Please use either static_cast or reinterpret_cast where appropriate.
> WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:328
> + g_value_set_string(value, webKitWebSrcGetUri((GstURIHandler *)
src));
Ditto.
More information about the webkit-reviews
mailing list