Bug 158299 – Introduce ResourceErrorBase::type

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

ResourceErrorBase has three booleans to store whether being null, timeout, cancellation or a combination of all three.
It would be better to introduce a ResourceError type.

[youenn fablet]

Created attachment 280326 [details]
Patch

[youenn fablet]

Created attachment 280329 [details]
Trying to fix win build

[youenn fablet]

In addition to remove the possibility of having errors that can be timeout and null, this patch may allow adding easily new error types.

A "CrossOrigin" error type may be useful to:
- Remove ThreadableLoaderClient::didFailAccessControl/didFailRedirectCheck
- Move response cross-origin checks in SubresourceLoader in lieu of various loaders (DocumentThreadableLoader, ImageLoader...)

[Alex Christensen]

Comment on attachment 280329 [details]
Trying to fix win build

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

This looks like an improvement.  There's another patch I've seen recently changing the way we do errors with the FrameLoader.

> Source/WebCore/loader/EmptyClients.h:334
> +    ResourceError blockedError(const ResourceRequest&) override { return ResourceError(); }

Could we use { } ?

> Source/WebCore/platform/network/ResourceErrorBase.h:58
> +    void setAsCancellation() { m_type = Type::Cancellation; }

Could this have an assertion that type wasn't Timeout?  Could the assertion be even stricter?
Also, I think it would be nice to have a Type parameter required in the constructor.

> Source/WebCore/platform/network/ResourceErrorBase.h:93
> +    Type m_type { Type::Null };

It's confusing that this defaults to Null when there is a Default type.

[youenn fablet]

(In reply to comment #4)
> Comment on attachment 280329 [details]
> Trying to fix win build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280329&action=review
> 
> This looks like an improvement.  There's another patch I've seen recently
> changing the way we do errors with the FrameLoader.
> 
> > Source/WebCore/loader/EmptyClients.h:334
> > +    ResourceError blockedError(const ResourceRequest&) override { return ResourceError(); }
> 
> Could we use { } ?

OK

> > Source/WebCore/platform/network/ResourceErrorBase.h:58
> > +    void setAsCancellation() { m_type = Type::Cancellation; }
> 
> Could this have an assertion that type wasn't Timeout?  Could the assertion
> be even stricter?

Yes, m_type should probably set at construct time once for all.

> Also, I think it would be nice to have a Type parameter required in the
> constructor.

I was tempted to doing so but resisted to get a smaller patch.
I can handle it now or as a follow-up patch.

> > Source/WebCore/platform/network/ResourceErrorBase.h:93
> > +    Type m_type { Type::Null };
> 
> It's confusing that this defaults to Null when there is a Default type.

Right, let's change the name to Type::General and make it the default here.

[youenn fablet]

Created attachment 280684 [details]
Patch

[WebKit Commit Bot]

Attachment 280684 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ResourceErrorBase.h:64:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 280687 [details]
Win build fix

[WebKit Commit Bot]

Attachment 280687 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ResourceErrorBase.h:64:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 280698 [details]
Win build fix again

[WebKit Commit Bot]

Attachment 280698 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ResourceErrorBase.h:64:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Csaba Osztrogonác]

nit: Please remove unrelated whitespace-only changes.

[youenn fablet]

Created attachment 280800 [details]
Removing whitespace changes

[youenn fablet]

(In reply to comment #12)
> nit: Please remove unrelated whitespace-only changes.

Done

[WebKit Commit Bot]

Attachment 280800 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ResourceErrorBase.h:64:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 280800 [details]
Removing whitespace changes

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

> Source/WebCore/ChangeLog:3
> +        Introduce a ResourceErrorBase type

::Type

> Source/WebCore/platform/network/ResourceErrorBase.h:64
> +    ResourceErrorBase(Type type) : m_type(type) { }

: m_type should be on the next line according to the style bot.

> Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp:207
>          resourceError = ResourceError();

{ }

[youenn fablet]

Thanks for the review.

(In reply to comment #16)
> Comment on attachment 280800 [details]
> Removing whitespace changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280800&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Introduce a ResourceErrorBase type
> 
> ::Type


OK

> > Source/WebCore/platform/network/ResourceErrorBase.h:64
> > +    ResourceErrorBase(Type type) : m_type(type) { }
> 
> : m_type should be on the next line according to the style bot.

I got other reviews that this was more readable this way for single assignment constructors. This style is consistent with other patches I made.
But I am fine changing to the bot style.

> > Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp:207
> >          resourceError = ResourceError();
> 
> { }

OK

[youenn fablet]

Created attachment 280885 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 280885 [details]
Patch for landing

Clearing flags on attachment: 280885

Committed r201856: <http://trac.webkit.org/changeset/201856>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

Build fix for Apple internal iOS build:

<https://trac.webkit.org/r201870>

[youenn fablet]

Thanks!