Bug 54628 - [GStreamer] Add 'location' property in webkitwebsrc
Summary: [GStreamer] Add 'location' property in webkitwebsrc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 54627
  Show dependency treegraph
 
Reported: 2011-02-17 02:01 PST by Andoni
Modified: 2011-02-21 01:34 PST (History)
4 users (show)

See Also:


Attachments
add 'location' property (103 bytes, text/plain)
2011-02-17 02:01 PST, Andoni
no flags Details
Patch (3.46 KB, patch)
2011-02-17 03:16 PST, Andoni
no flags Details | Formatted Diff | Diff
Patch (3.49 KB, patch)
2011-02-18 08:13 PST, Andoni
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andoni 2011-02-17 02:01:58 PST
Created attachment 82771 [details]
add 'location' property

Add the 'location' property to the webkitwebsrc so that get_element_make_from_uri() can set the uri on the element.
Comment 1 Philippe Normand 2011-02-17 02:18:41 PST
Could you post a patch with a ChangeLog please? See also http://www.webkit.org/coding/contributing.html :)
Comment 2 Andoni 2011-02-17 03:16:59 PST
Created attachment 82776 [details]
Patch
Comment 3 WebKit Review Bot 2011-02-17 03:18:51 PST
Attachment 82776 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:122:  The parameter name "handler" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:123:  The parameter name "handler" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Philippe Normand 2011-02-17 04:08:34 PST
LGTM apart from the style issue :)
Comment 5 Martin Robinson 2011-02-17 07:41:29 PST
Comment on attachment 82776 [details]
Patch

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@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.
Comment 6 Andoni 2011-02-18 08:13:36 PST
Created attachment 82959 [details]
Patch
Comment 7 Andoni 2011-02-18 08:27:23 PST
I have updated the patch with all your suggestions :)
Comment 8 Martin Robinson 2011-02-18 08:39:18 PST
Comment on attachment 82959 [details]
Patch

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

Great!

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

Generally you shoud explain here why tests are not needed. We can do that whil landing this though.
Comment 9 Martin Robinson 2011-02-18 08:39:38 PST
Philippe, do you mind landing this one?
Comment 10 Philippe Normand 2011-02-21 01:34:08 PST
Committed r79197: <http://trac.webkit.org/changeset/79197>