[webkit-reviews] review granted: [Bug 60436] [Soup] Clean up error handling in ResourceHandleSoup : [Attachment 92690] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 60436: [Soup] Clean up error handling in ResourceHandleSoup
https://bugs.webkit.org/show_bug.cgi?id=60436

Attachment 92690: Patch
https://bugs.webkit.org/attachment.cgi?id=92690&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92690&action=review

> Source/WebCore/ChangeLog:8
> +	   Instead of repeating the ResourceErorr creation twice, abstract

ResourceErorr => ResourceError

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:414
> +    GOwnPtr<char> uri(soup_uri_to_string(soup_request_get_uri(request),
FALSE));

I am assuming this works out if request is a null pointer.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:424
> +    return ResourceError(g_quark_to_string(G_IO_ERROR),
> +			    error->code,

Maybe we should add an ASSERT(error) above this line to ensure that error is
non-null?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:781
> +	   client->didFail(handle.get(),
convertSoupErrorToResourceError(error.get(), d->m_soupRequest.get(), 0));

Maybe we should give the parameter message in convertSoupErrorToResourceError()
a default value of 0? Then we could remove the last argument in the call here.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:787
> +	   // Finish the load now instead of waiting for the input stream to
close.

The comment is sufficient as-is and I like how it's more concise that the old
one. But something about this comment makes me feel that I'm missing some kind
of reasoning into why we are calling didFinishLoading() now instead of calling
it in callback closeCallback(). The old comment quasi-satisfied this
"because"/or at least seemed to imply some kind of importance to this design
decision with the sense of urgency implied in the last sentence. Maybe we
should consider appending a "because" in the new comment.