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




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2010-08-19 11:28:10 PST ---
(From update of attachment 64849)
This patch is huge! I'd really like to see it split up into at least
two parts.

1. The code drop from Soup, which should probably be separated somehow from
the rest of our source, so we can treat it specially (it will go away eventually, right?).
2. The bits that actually implement the cache in WebCore/WebKit.

That being said, I have a few comments on what I presume to be the second part.

> +        GRefPtr<WebKitSoupRequest> m_req;
> +	GRefPtr<WebKitSoupRequester> m_requester;

Please do not abbreviate member names. This could be m_request or, heck,
even m_soupRequest. Someone new will look at the code and know exactly what it is.
The indentation is a little funky here as well.

>  #include "SharedBuffer.h"
>  #include "TextEncoding.h"
>  
> +#include "webkitsouprequesthttp.h"
> +
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <gio/gio.h>

Just remove all spaces in the list of includes per the style guidelines. There is
no need to separate them by include style.

> +    if (m_req) {
> +        g_object_set_data(G_OBJECT(m_req.get()), "webkit-resource", 0);
> +        m_req.clear();

There's no chance that this may be the last reference to m_req here?

> +    GOwnPtr<char> uri(soup_uri_to_string(soup_message_get_uri(msg), false));
> +    String location = String(uri.get());

Even though URL are generally URL-encoded, I don't know if we can rely on that
here. You should use String::fromUTF8.

> +#define BUFFER_SIZE 8192

Why not move this to the top and make the name more descriptive?
INPUT_STREAM_BUFFER_SIZE or something like that.

> +    GInputStream* in = webkitSoupRequestSend(d->m_req.get(), 0, &error.outPtr());

This should be a full-blown variable name like inputStream.

> -    d->m_idleHandler = g_timeout_add(0, parseDataUrl, handle);
> +    d->m_idleHandler = g_idle_add(parseDataUrl, handle);

Why did you switch this to an idle? I think Gustavo is trying to move
away from that because they are often starved. Maybe ping him about this.

>      g_object_set(session,
>                   SOUP_SESSION_MAX_CONNS, MAX_CONNECTIONS,
>                   SOUP_SESSION_MAX_CONNS_PER_HOST, MAX_CONNECTIONS_PER_HOST,
> -                 NULL);
> +                 0);

The style bot should not complain about this, because the missing NULL causes
a compiler warning. Please just use NULL here.

> +
> +    if (d->m_cancellable) {
> +        g_object_unref(d->m_cancellable);
> +        d->m_cancellable = 0;
> +    }

m_cancellable can probably be a GRefPtr in another patch.

> +    if (d->m_inputStream) {
> +        g_object_set_data(G_OBJECT(d->m_inputStream), "webkit-resource", 0);
> +        g_object_unref(d->m_inputStream);
> +        d->m_inputStream = 0;
> +    }

Ditto for m_inputStream.

> +
> +    if (d->m_buffer) {
> +        g_free(d->m_buffer);
> +        d->m_buffer = 0;
> +    }

Ditto for m_buffer, except GOwnPtr.

> +    GInputStream* in = webkitSoupRequestSendFinish(d->m_req.get(), res, &error.outPtr());

inputStream again. :)

> +
> +    if (error) {
> +        if (d->m_msg && SOUP_STATUS_IS_TRANSPORT_ERROR(d->m_msg->status_code)) {
> +            SoupURI* uri = webkitSoupRequestGetUri(d->m_req.get());
> +            GOwnPtr<char> uriStr(soup_uri_to_string(uri, false));

No need to abbreviate the variable name here. uriString is fine.

>  
> -    // Used to set the authentication dialog toplevel; may be NULL
> +    // Used to set the authentication dialog toplevel; may be 0

I think "null" here is clearer and probably won't upset the style gods.

> +        // We have to convert it to UTF-16 due to limitations in KURL
> +        GOwnPtr<gunichar2> unicodeStr(g_utf8_to_utf16(d->m_buffer, bytesRead, 0, 0, 0));
> +        client->didReceiveData(handle.get(), reinterpret_cast<const char*>(unicodeStr.get()), g_utf8_strlen(d->m_buffer, bytesRead) * sizeof(UChar), 0);

Okay, again this is kind of a nit, but unicodeStr is not entirely precise. It should
probably be utf16String.  g_utf8_strlen(d->m_buffer, bytesRead) * sizeof(UChar) seems
unsafe against encoding errors and is a little clunky. I think it's better to do something
like this:

    long wordsWritten;
    GOwnPtr<gunichar2> utf16String(g_utf8_to_utf16(d->m_buffer, bytesRead, 0, &wordsWritten, 0));
    client->didReceiveData(handle.get(), reinterpret_cast<const char*>(utf16String()), wordsWritten * sizeof(UChar), 0);

> +    g_object_set(soupMessage, SOUP_MESSAGE_METHOD, httpMethod().utf8().data(), 0);

NULL here instead of 0.

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