Bug 98521 – [Soup] Clean up ResourceError creation

• Component: Platform
• Status: RESOLVED FIXED

[Martin Robinson]

ResourceError creation is scattered throughout ResourceHandleSoup.cpp. This bug tracks moving it all to ResourceErrorSoup.cpp.

[Martin Robinson]

Created attachment 167337 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 167337 [details]
Patch

Attachment 167337 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14178537

[Martin Robinson]

Created attachment 167344 [details]
Try to fix the EFL build

[Carlos Garcia Campos]

Comment on attachment 167337 [details]
Patch

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

It seems to me you are adding a lot of more code that what we currently have. Instead of adding more ResourceError constructors maybe we add ErrorsSoup with a similar pattern that ErrorsGtk and add specific errors like transportError and sslError for example. For generic errors we could probably add a constructor that receives a GError and a failing uri.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:409
> +    if (!response.soupMessageTLSErrors())
> +        return false;
>  
> -static inline bool hasUnignoredTLSErrors(ResourceHandle* handle)
> -{
> -    return handle->getInternal()->m_response.soupMessageTLSErrors()
> -        && !gIgnoreSSLErrors
> -        && !allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower());
> +    String lowercaseHostURL = handle->firstRequest().url().host().lower();
> +    if (gIgnoreSSLErrors || allowsAnyHTTPSCertificateHosts().contains(lowercaseHostURL))
> +        return false;

I prefer a single if, and if gIgnoreSSLErrors or response.soupMessageTLSErrors() is tru you don't need to convert the host to lowercase:

if (!response.soupMessageTLSErrors() || gIgnoreSSLErrors || allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower())
    return;

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:413
> +    if (i != clientCertificates().end() || i->second.contains(response.soupMessageCertificate()))

I think this should be && not || it means, if we find a certificate map and it contains the response certificate.

[Martin Robinson]

(In reply to comment #4)
> (From update of attachment 167337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167337&action=review
> 
> It seems to me you are adding a lot of more code that what we currently have. Instead of adding more ResourceError constructors maybe we add ErrorsSoup with a similar pattern that ErrorsGtk and add specific errors like transportError and sslError for example. For generic errors we could probably add a constructor that receives a GError and a failing uri.

This patch adds a little bit more code, but the code is out of ResourceHandleSoup and separated into methods that fit onto one screen of text, so I think it's an overall improvement. As to the suggestion to use a pattern like ErrorsGtk, I want to actually move the networking related errors to ResourceErrorsSoup in a future patch, but not this one.  The code is moving in the direction you imagine, but more gradually. Hopefully it's okay to do things more slowly.
> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:409
> > +    if (!response.soupMessageTLSErrors())
> > +        return false;
> >  
> > -static inline bool hasUnignoredTLSErrors(ResourceHandle* handle)
> > -{
> > -    return handle->getInternal()->m_response.soupMessageTLSErrors()
> > -        && !gIgnoreSSLErrors
> > -        && !allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower());
> > +    String lowercaseHostURL = handle->firstRequest().url().host().lower();
> > +    if (gIgnoreSSLErrors || allowsAnyHTTPSCertificateHosts().contains(lowercaseHostURL))
> > +        return false;
> 
> I prefer a single if, and if gIgnoreSSLErrors or response.soupMessageTLSErrors() is tru you don't need to convert the host to lowercase:

What I'll do is to move the check for gIgnoreSSLErrors to the first conditional in this function. That's a good balance between what you are suggesting and my original intention, which was to avoid the duplicate call to host().lower().

> if (!response.soupMessageTLSErrors() || gIgnoreSSLErrors || allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower())
>     return;
> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:413
> > +    if (i != clientCertificates().end() || i->second.contains(response.soupMessageCertificate()))
> 
> I think this should be && not || it means, if we find a certificate map and it contains the response certificate.

Good catch, I definitely futzed that up.

[Martin Robinson]

Created attachment 167357 [details]
Respond to review comments

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 167337 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=167337&action=review
> > 
> > It seems to me you are adding a lot of more code that what we currently have. Instead of adding more ResourceError constructors maybe we add ErrorsSoup with a similar pattern that ErrorsGtk and add specific errors like transportError and sslError for example. For generic errors we could probably add a constructor that receives a GError and a failing uri.
> 
> This patch adds a little bit more code, but the code is out of ResourceHandleSoup and separated into methods that fit onto one screen of text, so I think it's an overall improvement. As to the suggestion to use a pattern like ErrorsGtk, I want to actually move the networking related errors to ResourceErrorsSoup in a future patch, but not this one.  The code is moving in the direction you imagine, but more gradually. Hopefully it's okay to do things more slowly.

Ah, ok. 

> > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:409
> > > +    if (!response.soupMessageTLSErrors())
> > > +        return false;
> > >  
> > > -static inline bool hasUnignoredTLSErrors(ResourceHandle* handle)
> > > -{
> > > -    return handle->getInternal()->m_response.soupMessageTLSErrors()
> > > -        && !gIgnoreSSLErrors
> > > -        && !allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower());
> > > +    String lowercaseHostURL = handle->firstRequest().url().host().lower();
> > > +    if (gIgnoreSSLErrors || allowsAnyHTTPSCertificateHosts().contains(lowercaseHostURL))
> > > +        return false;
> > 
> > I prefer a single if, and if gIgnoreSSLErrors or response.soupMessageTLSErrors() is tru you don't need to convert the host to lowercase:
> 
> What I'll do is to move the check for gIgnoreSSLErrors to the first conditional in this function. That's a good balance between what you are suggesting and my original intention, which was to avoid the duplicate call to host().lower().

Ah, I see, the lowercase host is used later.

> > if (!response.soupMessageTLSErrors() || gIgnoreSSLErrors || allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower())
> >     return;
> > 
> > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:413
> > > +    if (i != clientCertificates().end() || i->second.contains(response.soupMessageCertificate()))
> > 
> > I think this should be && not || it means, if we find a certificate map and it contains the response certificate.
> 
> Good catch, I definitely futzed that up.

[Carlos Garcia Campos]

Comment on attachment 167357 [details]
Respond to review comments

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

> Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:71
> +ResourceError::ResourceError(GError* error, SoupRequest* request, SoupMessage* message)
> +    : ResourceErrorBase(errorDomain(message), errorCodeFromErrorAndMessage(error, message), failingURI(request), failureReason(error, message))
> +    , m_tlsErrors(0)
> +{
> +}

Why not split this into two different constructors so that we don't need to check all the time whether it's a transport error or not? I find it a bit weird, because the constructor receives two arguments and only needs one of them depending on whether the error is a transport error or not. We could simply have 

ResourceError::ResourceError(SoupRequest* request, GError* error)
ResourceError::ResourceError(SoupRequest* request, SoupMessage* message)

moving the soupRequest as first parameter for consistency with the TLS error constructor that receives first the request and then the error information. With this approach the caller need to check which one to use, but only once, and we don't need all these small methods to decide what to use.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:409
> +    if (!response.soupMessageTLSErrors() || gIgnoreSSLErrors)
> +        return false;
>  
> -static inline bool hasUnignoredTLSErrors(ResourceHandle* handle)
> -{
> -    return handle->getInternal()->m_response.soupMessageTLSErrors()
> -        && !gIgnoreSSLErrors
> -        && !allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower());
> +    String lowercaseHostURL = handle->firstRequest().url().host().lower();
> +    if (allowsAnyHTTPSCertificateHosts().contains(lowercaseHostURL))
> +        return false;

Perfect, I think this method is a lot easier to read and follow than the current code.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:417
> +    handle->client()->didFail(handle, ResourceError(d->m_soupRequest.get(),
> +        response.soupMessageTLSErrors(), response.soupMessageCertificate()));

Maybe we could use a single line here and avoid the new style rule that makes the code more difficult to read? I'm not a fan of long lines, though.

[Martin Robinson]

Created attachment 167469 [details]
Same patch, but using the factory approach

[Martin Robinson]

(In reply to comment #8)

> Why not split this into two different constructors so that we don't need to check all the time whether it's a transport error or not? I find it a bit weird, because the constructor receives two arguments and only needs one of them depending on whether the error is a transport error or not. We could simply have 

I finally took the factory approach we discussed in the end. It's a bit simpler and we might as well do this now.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:417
> > +    handle->client()->didFail(handle, ResourceError(d->m_soupRequest.get(),
> > +        response.soupMessageTLSErrors(), response.soupMessageCertificate()));
> 
> Maybe we could use a single line here and avoid the new style rule that makes the code more difficult to read? I'm not a fan of long lines, though.

I went with the long line. I'm not sure that one style is harder to read than the other, but it definitely takes a bit of time to get used to a different style.

[Carlos Garcia Campos]

Comment on attachment 167469 [details]
Same patch, but using the factory approach

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?

[Martin Robinson]

(In reply to comment #11)

Thanks for the review.

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

Okay.

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

I chose httpError(...)
 
> > 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.

Yeah, having the style change on us is pretty annoying.

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

Here I chose genericIOError, to stress that it's just a generic scrape of the GIO GError information.

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

It's not really important to cache it, but putting it in a variable called errorDomain just makes it clear what it refers to.
 
> > 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?

This was an oversight on my part. I'll fix it before landing.

[Martin Robinson]

Committed r130613: <http://trac.webkit.org/changeset/130613>