[webkit-reviews] review granted: [Bug 98521] [Soup] Clean up ResourceError creation : [Attachment 167469] Same patch, but using the factory approach

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 98521: [Soup] Clean up ResourceError creation
https://bugs.webkit.org/show_bug.cgi?id=98521

Attachment 167469: Same patch, but using the factory approach
https://bugs.webkit.org/attachment.cgi?id=167469&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=167469&action=review


This is a lot simpler, thanks! Please check my comments before landing, most of
them are just suggestions or questions.

> Source/WebCore/ChangeLog:23
> +	   * platform/network/soup/ResourceErrorSoup.cpp: Added.
> +	   (WebCore::isTransportError): Added helper to be used in the
constructor.
> +	   (WebCore::errorDomain): Ditto.
> +	   (WebCore::errorCodeFromErrorAndMessage): Ditto.
> +	   (WebCore::failingURI): Ditto.
> +	   (WebCore::failureReason): Ditto.
> +	   (WebCore::ResourceError::timeoutError): Added this factory.

This needs to be updated to the new factory approach.

> Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:45
> +ResourceError ResourceError::soupMessageError(SoupMessage* message, GError*
error, SoupRequest* request)

soupHTTPError()? it's consistent with the error domain SOUP_HTTP_ERROR and
doesn't seem like there's an error in the message itself.

> Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:50
> +    if (!message || !SOUP_STATUS_IS_TRANSPORT_ERROR(message->status_code))
> +	   return gerrorError(error, request);
> +    return ResourceError(g_quark_to_string(SOUP_HTTP_ERROR),
message->status_code,
> +	   failingURI(request), String::fromUTF8(message->reason_phrase));

Same happens to me here, at a first glance the last line seems to me like a bad
indented line instead of the second line of a multiline function call, because
it's lined up with the previous return in the if. Maybe it's a matter of
getting used to it, as you said, but I find it very confusing. Anyway, I'm not
suggesting you to change anything, because in the end it's just my personal
feeling, so feel free to leave it this way or use a single line.

> Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:57
> +ResourceError ResourceError::gerrorError(GError* error, SoupRequest*
request)
> +{
> +    return ResourceError(g_quark_to_string(G_IO_ERROR), error->code,
> +	   failingURI(request), String::fromUTF8(error->message));
> +}

gIOError() maybe? it's a GIO error in the end not a generic GError. Or you can
take the domain from the GError instead of using G_IO_ERROR unconditionally.

> Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:72
> +    static const char* const  errorDomain = "WebKitnetworkError";

Why do you need to cache this? why not just using "WebKitnetworkError"
directly?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:402
> +    static const ResourceResponse& response = d->m_response;

I haven't noticed this before, why is this static? what happens when the
ResourceHandleInternal is deleted? Why do you need to cache this instead of
simply use d->m_response directly?