[Webkit-unassigned] [Bug 44261] [GTK] Add HTTP caching support
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 1 09:55:21 PDT 2010
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.
More information about the webkit-unassigned
mailing list