[Webkit-unassigned] [Bug 44261] [GTK] Add HTTP caching support
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 30 15:19:34 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=44261
Martin Robinson <mrobinson at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #69356|review? |review-
Flag| |
--- Comment #98 from Martin Robinson <mrobinson at webkit.org> 2010-09-30 15:19:34 PST ---
(From update of attachment 69356)
View in context: https://bugs.webkit.org/attachment.cgi?id=69356&action=review
This patch is so very close, but I've found some issues with memory leaks. When constructing a platformRefPtr, you should use adoptPlatformRef, unless you want the reference count to increase (the default constructor calls g_object_ref_sink).
> WebCore/platform/network/ResourceHandleInternal.h:118
> + , m_soupRequest(0)
> + , m_requester(0)
> , m_inputStream(0)
> , m_cancellable(0)
Since these are now PlatformRefPtrs you don't need to initialize them. They are automatically 0.
> WebCore/platform/network/ResourceHandleInternal.h:140
> + m_requester = webkit_soup_requester_new();
This should use adoptPlatformRef, see below for an extended note about this.
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:357
> + d->m_soupRequest = webkit_soup_requester_request(d->m_requester.get(), handle->firstRequest().url().string().utf8().data(), session, &error.outPtr());
Ditto.
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:365
> + if (error)
> + return false;
> +
> + d->m_inputStream = in;
Careful here. From the soup code it looks like it is the callers responsibility to unref the value that webkit_soup_request_send returns. Constructing a PlatformRefPtr without using adoptPlatformRef has the effect of calling g_object_ref another time on the GInputStream.
d->m_inputStream = adoptPlatformRef(webkit_soup_request_send(d->m_soupRequest.get(), 0, &error.outPtr()));
if (error) {
d->m_inputStream = 0;
return false;
}
and dispense with the intermediate 'in' variable entirely.
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:373
> + d->m_cancellable = g_cancellable_new();
This should be d->m_cancellable = adoptPlatformRef(g_cancellable_new()); as well.
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:436
> +static void cleanupSoupRequestOperation(ResourceHandle* handle, bool isDestroying = false)
> +{
> + ResourceHandleInternal* d = handle->getInternal();
> +
> + if (d->m_soupRequest) {
> + g_object_set_data(G_OBJECT(d->m_soupRequest.get()), "webkit-resource", 0);
> + d->m_soupRequest.clear();
> + }
We still need to clear the m_cancellable here to ensure that the unref happens, right?
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:519
> + d->m_inputStream = in;
The same goes for this section. After this the reference count on d->m_inputStream will be 2.
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:564
> + d->m_soupRequest = webkit_soup_requester_request(d->m_requester.get(), url.string().utf8().data(), session, &error.outPtr());
Ditto.
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:652
> + d->m_cancellable = g_cancellable_new();
Same issue here.
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:833
> // GIO doesn't know how to handle refs and queries, so remove them
Shouldn't this comment be changed to reflect the fact that we're using soup now?
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:839
> + urlStr = url.string().utf8().data();
This might as well be:
CString urlStr = url.string().utf8().data();
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:842
> + d->m_soupRequest = webkit_soup_requester_request(d->m_requester.get(), urlStr.data(), session, &error.outPtr());
I think this should use adoptPlatformRef as well.
> WebCore/platform/network/soup/ResourceHandleSoup.cpp:851
> d->m_cancellable = g_cancellable_new();
Same issue here.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list