[webkit-reviews] review denied: [Bug 24758] [Gtk] Implement a WebDataSource for the gtk port : [Attachment 33014] updated patch

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied Jan Alonzo <jmalonzo at gmail.com>'s
request for review:
Bug 24758: [Gtk] Implement a WebDataSource for the gtk port
https://bugs.webkit.org/show_bug.cgi?id=24758

Attachment 33014: updated patch
https://bugs.webkit.org/attachment.cgi?id=33014&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
> +static void test_webkit_web_resource_get_url(WebResourceFixture* fixture,
gconstpointer data)
> +{
> +    gchar* url;
> +    g_object_get(G_OBJECT(fixture->webResource), "url", &url, NULL);
> +    g_assert_cmpstr(url, ==, "http://example.com/");
> +    g_assert_cmpstr(webkit_web_resource_get_url(fixture->webResource)
,==,"http://example.com/");
> +}

You are leaking 'url' here, g_object_get will dup the string.

> +static void test_webkit_web_resource_get_mime_type(WebResourceFixture*
fixture, gconstpointer data)
> +{
> +    gchar* mime_type;
> +    g_object_get(G_OBJECT(fixture->webResource), "mime-type", &mime_type,
NULL);
> +    g_assert_cmpstr(mime_type, ==, "text/html");
> +   
g_assert_cmpstr(webkit_web_resource_get_mime_type(fixture->webResource),==,"tex
t/html");
> +}

Leaking 'mime_type'; same issue for all remaining tests.

> +static void webkit_web_resource_dispose(GObject* object)
> +{
> +    WebKitWebResource* webResource = WEBKIT_WEB_RESOURCE(object);
> +    WebKitWebResourcePrivate* priv = webResource->priv;
> +
> +    if (priv->resource)
> +	   priv->resource->deref();

Don't you need a priv->resource = 0; here?

> +
> +    G_OBJECT_CLASS(webkit_web_resource_parent_class)->dispose(object);
> +}
> +
> +static void webkit_web_resource_finalize(GObject* object)
> +{
> +    WebKitWebResource* webResource = WEBKIT_WEB_RESOURCE(object);
> +    WebKitWebResourcePrivate* priv = webResource->priv;
> +
> +    priv->url = CString();
> +    priv->mimeType = CString();
> +    priv->textEncoding = CString();
> +    priv->frameName = CString();

IIRC we had a discussion about this and we decided it was not a good method to
free memory? Is the mac port doing something like this? I'd say you'll be still
leaking a full CString object this way...

> +
> +    if (priv->data)
> +	   g_string_free(priv->data, FALSE);

I think you want to pass TRUE, not FALSE.

> +
> +/**
> + * webkit_web_resource_new:
> + * @data: the data to initialize the #WebKitWebResource
> + * @url: the url of the #WebKitWebResource
> + * @mime_type: the MIME type of the #WebKitWebResource
> + * @text_encoding_name: the text encoding name of the #WebKitWebResource
> + * @frame_name: the frame name of the #WebKitWebResource
> + *
> + * Returns a new #WebKitWebResource. The @text_encoding_name can be %NULL.
The
> + * @frame_name argument can be used if the resource represents contents of
an
> + * entire HTML frame, otherwise pass %NULL.
> + *
> + * Return value: a new #WebKitWebResource
> + *
> + * Since: 1.1.12
> + */
> +WebKitWebResource* webkit_web_resource_new(const gchar* data,
> +					      const gchar* url,
> +					      const gchar* mimeType,
> +					      const gchar* textEncodingName,
> +					      const gchar* frameName)
> +{
> +    ASSERT(data);
> +    ASSERT(url);
> +    ASSERT(mime_type);
> +
> +    RefPtr<SharedBuffer> buffer = SharedBuffer::create(data, strlen(data));

Mmm, data can't have explicit '0'? If it can strlen is wrong here (and I guess
you'd need an extra 'length' parameter).

> +    WebKitWebResource* webResource =
webkit_web_resource_new_with_core_resource(ArchiveResource::create(buffer,
KURL(KURL(),String::fromUTF8(url)), String::fromUTF8(mimeType),
String::fromUTF8(textEncodingName), String::fromUTF8(frameName)));
> +
> +    return webResource;
> +}
> +
> +/**
> + * webkit_web_resource_get_data:
> + * @web_resource: a #WebKitWebResource
> + *
> + * Returns the data of the @webResource.
> + *
> + * Return value: a #GString containing the character data of the
@webResource.
> + * The string is owned by WebKit and should not be freed or destroyed.
> + *
> + * Since: 1.1.12
> + */
> +G_CONST_RETURN GString* webkit_web_resource_get_data(WebKitWebResource*
webResource)

I believe in general 'const pointer object' makes little sense in C APIs, so
I'd just return GString* with the note.

> +/**
> + * webkit_web_resource_get_url:
> + * @web_resource: a #WebKitWebResource
> + *
> + * Returns the URL of the resource
> + *
> + * Return value: the URL of the resource
> + *
> + * Since: 1.1.12
> + */
> +G_CONST_RETURN gchar* webkit_web_resource_get_url(WebKitWebResource*
webResource)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_RESOURCE(webResource), NULL);
> +
> +    WebKitWebResourcePrivate* priv = webResource->priv;
> +    if (!priv->resource)
> +	   return NULL;
> +
> +    priv->url = priv->resource->url().string().utf8();
> +
> +    return priv->url.data();

Shouldn't you put the URL in priv->url once and then return always that? You
are doing the same for the other properties.


> + * webkit_web_resource_get_text_encoding_name:
> + * @web_resource: a #WebKitWebResource
> + *
> + * Returns the text encoding name of the resource
> + *
> + * Return value: the text encoding name of the resource
> + *
> + * Since: 1.1.12
> + */
> +G_CONST_RETURN gchar*
webkit_web_resource_get_text_encoding_name(WebKitWebResource* webResource)

We have other APIs to get the encoding name and we call them simply
"get_encoding", so I think we should be consistent here probably.

Going to r- while we work on the issues.