Bug 60436 - [Soup] Clean up error handling in ResourceHandleSoup
Summary: [Soup] Clean up error handling in ResourceHandleSoup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Martin Robinson
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-05-07 08:50 PDT by Martin Robinson
Modified: 2011-05-09 10:47 PDT (History)
3 users (show)

See Also:


Attachments
Patch (7.20 KB, patch)
2011-05-07 09:01 PDT, Martin Robinson
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2011-05-07 08:50:06 PDT
Just what it says on the tin. This bug tracks a simple code cleanup.
Comment 1 Martin Robinson 2011-05-07 09:01:31 PDT
Created attachment 92690 [details]
Patch
Comment 2 Daniel Bates 2011-05-07 14:15:38 PDT
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.
Comment 3 Martin Robinson 2011-05-07 14:19:55 PDT
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." ?
Comment 4 Daniel Bates 2011-05-07 14:28:17 PDT
(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."?
Comment 5 Martin Robinson 2011-05-09 10:46:47 PDT
(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.
Comment 6 Martin Robinson 2011-05-09 10:47:23 PDT
Committed r86064: <http://trac.webkit.org/changeset/86064>