SSIA
Created attachment 124090 [details] 0.11 webkitwebsrc
Comment on attachment 124090 [details] 0.11 webkitwebsrc View in context: https://bugs.webkit.org/attachment.cgi?id=124090&action=review > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:590 > + g_set_error(error, GST_URI_ERROR, GST_URI_ERROR_BAD_URI, "Invalid URI '%s'", uri); error can be NULL
Looks good other than that
Comment on attachment 124090 [details] 0.11 webkitwebsrc View in context: https://bugs.webkit.org/attachment.cgi?id=124090&action=review Thanks Sebastian for the review! A couple nits + the error fix and this is good to land. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:509 > case GST_QUERY_DURATION: > - { > - GstFormat format; > + { > + GstFormat format; It's usual for these braces to be in the same line as the case statement, and close on the same column as the c in case. case BLA: { codegoeshere; } > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:558 > + static gchar* protocols[] = {(gchar*) "http", (gchar*) "https", 0 }; static_cast<>, I'd say let's not use the gchar defines unless it's necessary =)
Comment on attachment 124090 [details] 0.11 webkitwebsrc View in context: https://bugs.webkit.org/attachment.cgi?id=124090&action=review > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:127 > +static void webKitWebSrcNeedDataCb(GstAppSrc*, guint, gpointer); > +static void webKitWebSrcEnoughDataCb(GstAppSrc*, gpointer); > +static gboolean webKitWebSrcSeekDataCb(GstAppSrc*, guint64, gpointer); > > -static void webKitWebSrcStop(WebKitWebSrc* src, bool seeking); > -static gboolean webKitWebSrcSetUri(GstURIHandler*, const gchar*); > -static const gchar* webKitWebSrcGetUri(GstURIHandler*); > +static void webKitWebSrcStop(WebKitWebSrc*, bool); You should actually continue include the variable name here for generic types like guint64 and gpointer. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:556 > +const gchar * const * webKitWebSrcGetProtocols(GType) The spacing of the asterisks is off here. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:558 >> + static gchar* protocols[] = {(gchar*) "http", (gchar*) "https", 0 }; > > static_cast<>, I'd say let's not use the gchar defines unless it's necessary =) I think you can avoid the casts entirely if you make this something like static const char* protocols[] = {"http", "https", 0}; > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:568 > + WebKitWebSrc* src = WEBKIT_WEB_SRC(handler); > + WebKitWebSrcPrivate* priv = src->priv; > + > + return g_strdup(priv->uri); Just make this one line: return g_strdrup(WEBKIT_WEB_SRC(handler)->priv->uri));
(In reply to comment #2) > (From update of attachment 124090 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124090&action=review > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:590 > > + g_set_error(error, GST_URI_ERROR, GST_URI_ERROR_BAD_URI, "Invalid URI '%s'", uri); > > error can be NULL Hum but g_set_error accepts NULL error, not sure I understand the issue here.
(In reply to comment #6) > (In reply to comment #2) > > (From update of attachment 124090 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=124090&action=review > > > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:590 > > > + g_set_error(error, GST_URI_ERROR, GST_URI_ERROR_BAD_URI, "Invalid URI '%s'", uri); > > > > error can be NULL > > Hum but g_set_error accepts NULL error, not sure I understand the issue here. Yes, I forgot that g_set_error() accepts NULL. Sorry
Committed r106446: <http://trac.webkit.org/changeset/106446>