[webkit-reviews] review denied: [Bug 44261] [GTK] Add HTTP caching support : [Attachment 68350] WebKitGtk+ to use the new API from the imported SoupURILoader code

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 44261: [GTK] Add HTTP caching support
https://bugs.webkit.org/show_bug.cgi?id=44261

Attachment 68350: WebKitGtk+ to use the new API from the imported SoupURILoader
code
https://bugs.webkit.org/attachment.cgi?id=68350&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68350&action=review

Looks good! I think we should merge this soon. I just have a few style nits.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:128
> +static void cleanupSoupRequestOperation(ResourceHandle* handle, bool
isDestroying);
> +static void sendRequestCallback(GObject* source, GAsyncResult* res,
gpointer);
> +static void readCallback(GObject* source, GAsyncResult* res, gpointer);
> +static void closeCallback(GObject* source, GAsyncResult* res, gpointer);

I think all of these parameter names are unnecessary except for isDestroying.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:381
> +    handle->ref();

This really needs a comment describing where the balancing deref() is.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:386
> +    g_input_stream_read_async(d->m_inputStream, d->m_buffer,
READ_BUFFER_SIZE,
> +				 G_PRIORITY_DEFAULT, d->m_cancellable,
> +				 readCallback, (isBase64) ? 0 :
GINT_TO_POINTER(1));

Perhaps smoosh this down to two lines in the interest of vertical space?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:403
> +

This blank line seems accidental.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:501
> +	       ResourceError resourceError(g_quark_to_string(SOUP_HTTP_ERROR),
> +					   d->m_msg->status_code,
> +					   uriStr.get(),
> +					  
String::fromUTF8(d->m_msg->reason_phrase));

Once again, smoosh this down to two lines.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:513
> +	       ResourceError resourceError(g_quark_to_string(G_IO_ERROR),
> +					   error->code,
> +					   uriStr.get(),
> +					   error ?
String::fromUTF8(error->message) : String());

Ditto.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:574
> +    g_input_stream_read_async(d->m_inputStream, d->m_buffer,
READ_BUFFER_SIZE,
> +				 G_PRIORITY_DEFAULT, d->m_cancellable,
> +				 readCallback, 0);

Ditto.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:824
>	   ResourceError resourceError(g_quark_to_string(G_IO_ERROR),
>				       error->code,
> -				       uri,
> +				       uriStr.get(),
>				       error ? String::fromUTF8(error->message)
: String());

Ditto.

> WebCore/platform/network/soup/ResourceRequest.h:70
> +	   void updateSoupMessage(SoupMessage* soupMessage) const;

Please remove the parameter name.