WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
60436
[Soup] Clean up error handling in ResourceHandleSoup
https://bugs.webkit.org/show_bug.cgi?id=60436
Summary
[Soup] Clean up error handling in ResourceHandleSoup
Martin Robinson
Reported
2011-05-07 08:50:06 PDT
Just what it says on the tin. This bug tracks a simple code cleanup.
Attachments
Patch
(7.20 KB, patch)
2011-05-07 09:01 PDT
,
Martin Robinson
dbates
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Martin Robinson
Comment 1
2011-05-07 09:01:31 PDT
Created
attachment 92690
[details]
Patch
Daniel Bates
Comment 2
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.
Martin Robinson
Comment 3
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." ?
Daniel Bates
Comment 4
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."?
Martin Robinson
Comment 5
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.
Martin Robinson
Comment 6
2011-05-09 10:47:23 PDT
Committed
r86064
: <
http://trac.webkit.org/changeset/86064
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug