[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 #68350|review?                     |review-
               Flag|                            |




--- Comment #72 from Martin Robinson <mrobinson at webkit.org>  2010-09-23 16:43:47 PST ---
(From update of attachment 68350)
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.

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