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.
Created attachment 280326 [details] Patch
Created attachment 280329 [details] Trying to fix win build
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...)
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.
(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.
Created attachment 280684 [details] Patch
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.
Created attachment 280687 [details] Win build fix
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.
Created attachment 280698 [details] Win build fix again
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.
nit: Please remove unrelated whitespace-only changes.
Created attachment 280800 [details] Removing whitespace changes
(In reply to comment #12) > nit: Please remove unrelated whitespace-only changes. Done
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.
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(); { }
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
Created attachment 280885 [details] Patch for landing
Comment on attachment 280885 [details] Patch for landing Clearing flags on attachment: 280885 Committed r201856: <http://trac.webkit.org/changeset/201856>
All reviewed patches have been landed. Closing bug.
Build fix for Apple internal iOS build: <https://trac.webkit.org/r201870>
Thanks!