Bug 60436 – [Soup] Clean up error handling in ResourceHandleSoup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

Just what it says on the tin. This bug tracks a simple code cleanup.

[Martin Robinson]

Created attachment 92690 [details]
Patch

[Daniel Bates]

Comment on attachment 92690 [details]
Patch

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.

[Martin Robinson]

Comment on attachment 92690 [details]
Patch

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

>> 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.

How about: "We want to inform WebCore of load completion as soon as possible, so we do not wait for the stream to close." ?

[Daniel Bates]

(In reply to comment #3)
> How about: "We want to inform WebCore of load completion as soon as possible, so we do not wait for the stream to close." ?

This doesn't seem to add much of a reason compared to what you had previously. Maybe "We inform WebCore of load completion now instead of waiting for the input stream to close because the input stream is closed asynchronously."?

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 92690 [details])
> 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

Fixed.

> > 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?

I've added:
ASSERT(error);
ASSERT(request);

> > 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.

Made this change.

> > 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.

I changed the comment to the one you suggested in your latest reply.

[Martin Robinson]

Committed r86064: <http://trac.webkit.org/changeset/86064>