[webkit-reviews] review denied: [Bug 44261] [GTK] Add HTTP caching support : [Attachment 69438] 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 69438: WebKitGtk+ to use the new API from the imported SoupURILoader
code
https://bugs.webkit.org/attachment.cgi?id=69438&action=review

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

General comment: I'm not as picky about this as others, but some reviewers
would have you end all comments with a period. That's something to consider for
this patch.

> WebCore/platform/network/ResourceHandleInternal.h:189
> +	   PlatformRefPtr<SoupMessage> m_soupMsg;

This should probably be m_soupMessage, as the guideline is to avoid
abbreviations in new code.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:-132
> -    if (m_msg) {
> -	   g_object_unref(m_msg);
> -	   m_msg = 0;
> -    }

Nice change.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:145
> -    if (d->m_msg)
> -	   g_signal_handlers_disconnect_matched(d->m_msg, G_SIGNAL_MATCH_DATA,
> +    if (d->m_soupMsg)
> +	   g_signal_handlers_disconnect_matched(d->m_soupMsg.get(),
G_SIGNAL_MATCH_DATA,

I could be wrong about this, but shouldn't this be in
cleanupSoupRequestOperation? Otherwise, we might get responses to messages for
a no-longer-valid SoupMessage.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:364
> +    d->m_buffer = static_cast<char*>(g_malloc(READ_BUFFER_SIZE));

My intuition is that this allocation should either be done with the GLib slice
allocator or with fastAlloc/fastFree. Perhaps someone else can chime in here.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:445
> +    if (d->m_cancellable)
> +	   d->m_cancellable.clear();
> +
> +    if (d->m_soupMsg)
> +	   d->m_soupMsg.clear();

No need to check these before clearing them. Just clear them.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:506
> +	       // WebCore might have cancelled the job in the while
> +	       if (d->m_cancelled)
> +		   return;
> +
> +	       if (soupMsg->response_body->data)
> +		   client->didReceiveData(handle.get(),
soupMsg->response_body->data, soupMsg->response_body->length, true);

In the case that the request was cancelled this should be calling
cleanupSoupRequestOperation(). So you might change 
    if (soupMsg->response_body->data)
to
    if (!d->m_cancelled && soupMsg->response_body->data)
and remove the early return.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:513
> +	   if (d->m_cancelled || !client) {
> +	     cleanupSoupRequestOperation(handle.get());
> +	     return;
> +	   }

The tab width here should be 4.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:525
> +    d->m_buffer = static_cast<char*>(g_malloc(READ_BUFFER_SIZE));

Same issue with the allocation.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:578
> +    SoupMessage* soupMsg = d->m_soupMsg.get();

Avoid abbreviations.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:582
> +    request.updateSoupMessage(soupMsg);
> +
> +    if (!d->m_soupMsg)
>	   return false;

It doesn't look like you want to call updateSoupMessage with a null message, so
you should probably move this check up to line 578.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:775
> +static void readCallback(GObject* source, GAsyncResult* res, gpointer data)

Please change res to asyncResult.

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

This can be two lines.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:845
> +    CString urlStr = url.string().utf8().data();

This can just be: CString urlStr = url.string().utf8(); I think I suggested
.data() originally, sorry.