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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
This patch is looking good. I'm excited for this to land.

View in context:
https://bugs.webkit.org/attachment.cgi?id=66190&action=prettypatch

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:138
> +	   m_soupRequest.clear();
m_soupRequest.clear(); is unecessary here, I believe. The destructor will take
care of decrementing the smart pointer reference count. If this is needed
otherwise there should really be a comment explaining why.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:196
> +    String location = String(uri.get());
Again, I believe this should be ::fromUTF8(...).

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:305
> +#define BUFFER_SIZE 8192
If this is a constant, why do we need to store m_bufferSize? Let's just get rid
of that member if possible. Again, I think this should be called
READ_BUFFER_SIZE and be at the top of the file too.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:689
> +			       sendRequestCallback, 0);
This newline seems unecessary. I usually split the line when it goes over 120
characters.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:846
> +	   client->didReceiveData(handle.get(), reinterpret_cast<const
char*>(unicodeStr.get()), g_utf8_strlen(d->m_buffer, bytesRead) *
sizeof(UChar), 0);
Would it be possible to simply use WTFString here?

String data = String::fromUTF8(d->m_buffer, bytesRead);
client->didReceiveData(handle.get(), reinterpret_cast<const
char*>(data.characters()), data.length() * sizeof(UChar), 0);

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:867
> +    GOwnPtr<char> urlStr;
Please use CString here, if possible. It was made just for this purpose.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:901
> +
Please remove the newline.