[Webkit-unassigned] [Bug 34317] [GStreamer] Add a WebKit HTTP/HTTPS source that uses WebKit's network infrastructure

[bugzilla-daemon at webkit.org]

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





--- Comment #25 from Gustavo Noronha (kov) <gns at gnome.org>  2010-02-05 05:22:38 PST ---
(From update of attachment 48220)
 142         Frame* frame = mp->m_player->frameView() ?
 143 mp->m_player->frameView()->frame() : 0;

Keep these in a single line, or indent the second one appropriately =).

 567 void webkit_web_src_set_frame(WebkitWebSrc* src, WebCore::Frame* frame)
 568 {
 569     WebkitWebSrcPrivate* priv = src->priv;
 570 
 571     frame->ref();
 572     priv->frame = frame;
 573 }

Instead of using manual refing/unrefing, I think you should make the object's
private frame a RefPtr<Frame>, then just assigning 0 to it will make it go
away, when it's no longer needed.

 22 #include "CString.h"
[...]
 29 #include "ResourceRequest.h"
 30 #include "ResourceResponse.h"
 31 
 32 #include <gst/app/gstappsrc.h>
 33 #include <gst/pbutils/missing-plugins.h>

I know this pattern is used in a bunch of places, but the right thing to do
here is to add the system includes in the same list as the webkit ones. See,
for instance, WebCore/platform/mac/GeolocationServiceMac.mm.

 104 static void webkit_web_src_finalize(GObject* object);
 105 static void webkit_web_src_set_property(GObject* object, guint propID,
 106     const GValue* value, GParamSpec* pspec);
 107 static void webkit_web_src_get_property(GObject* object, guint propID,
 108     GValue* value, GParamSpec* pspec);
 109 static GstStateChangeReturn webkit_web_src_change_state(GstElement*
element,
 110                                                         GstStateChange
transition);

Indentation is weird here. I would prefer a single line for each, but just
being consistent works for me =D. I have noticed indentation issues like these
also in other functions, like base_init, and src_init, but I won't clutter the
review with them, just give everything a pass, I'd say.

223     gst_app_src_set_max_bytes(priv->appsrc, 512 * 1024);

Why is this the maximum? Does this depend on the amount of data soup will get
before each got-chunk, for instance?

 234     g_free(priv->uri);

 339 static gboolean webkit_web_src_start(WebkitWebSrc* src)

You can use bool here instead of gboolean, because this is internal.

 368         gchar* val = g_strdup_printf("bytes=%" G_GUINT64_FORMAT "-",
priv->requestedOffset);

Use GOwnPtr<gchar>, and throw the g_free away =).

 432 /*** GSTURIHANDLER INTERFACE
*************************************************/

This is very alien for WebKit code =D.

// GstUriHandler Interface.

looks more like it =)

 441     static gchar* protocols[] = {(gchar*) "http", (gchar*) "https", 0 };

You can use char*, and lose the casts.

 464     if (uri && !g_ascii_strncasecmp("http", uri, 4) &&
!g_ascii_strncasecmp("http", uri, 5)) {

Looks like one of them is missing an s. You may consider storing priv->uri as a
SoupURI, btw, and using SOUP_URI_VALID_FOR_HTTP() here instead. This has the
advantage of only storing URIs that soup will actually accept, and saving us
the trouble of finding bugs later for using invalid-for-soup URIs.

Let me push what I already have (since you'll likely work on this soonish =P),
and I'll add another comment with the rest.

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