[Webkit-unassigned] [Bug 44261] [GTK] Add HTTP caching support

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69438|review?                     |review-
               Flag|                            |




--- Comment #105 from Martin Robinson <mrobinson at webkit.org>  2010-10-01 09:55:20 PST ---
(From update of attachment 69438)
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.

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