Bug 158299

Summary: Introduce ResourceErrorBase::type
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, berto, cdumez, cgarcia, commit-queue, danw, darin, ddkilzer, galpeter, gustavo, japhet, mcatanzaro, mrobinson, ossy
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Trying to fix win build
none
Patch
none
Win build fix
none
Win build fix again
none
Removing whitespace changes
none
Patch for landing none

youenn fablet
Reported 2016-06-02 02:50:49 PDT
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.
Attachments
Patch (18.01 KB, patch)
2016-06-02 03:31 PDT, youenn fablet
no flags
Trying to fix win build (19.32 KB, patch)
2016-06-02 06:17 PDT, youenn fablet
no flags
Patch (65.99 KB, patch)
2016-06-07 02:21 PDT, youenn fablet
no flags
Win build fix (66.03 KB, patch)
2016-06-07 03:17 PDT, youenn fablet
no flags
Win build fix again (65.91 KB, patch)
2016-06-07 05:16 PDT, youenn fablet
no flags
Removing whitespace changes (24.25 KB, patch)
2016-06-08 06:20 PDT, youenn fablet
no flags
Patch for landing (24.32 KB, patch)
2016-06-08 23:23 PDT, youenn fablet
no flags
youenn fablet
Comment 1 2016-06-02 03:31:41 PDT
youenn fablet
Comment 2 2016-06-02 06:17:18 PDT
Created attachment 280329 [details] Trying to fix win build
youenn fablet
Comment 3 2016-06-03 08:16:04 PDT
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 4 2016-06-06 01:16:34 PDT
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
Comment 5 2016-06-06 10:33:16 PDT
(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
Comment 6 2016-06-07 02:21:27 PDT
WebKit Commit Bot
Comment 7 2016-06-07 02:24:04 PDT
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
Comment 8 2016-06-07 03:17:57 PDT
Created attachment 280687 [details] Win build fix
WebKit Commit Bot
Comment 9 2016-06-07 03:18:46 PDT
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
Comment 10 2016-06-07 05:16:14 PDT
Created attachment 280698 [details] Win build fix again
WebKit Commit Bot
Comment 11 2016-06-07 05:17:26 PDT
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
Comment 12 2016-06-07 08:37:50 PDT
nit: Please remove unrelated whitespace-only changes.
youenn fablet
Comment 13 2016-06-08 06:20:10 PDT
Created attachment 280800 [details] Removing whitespace changes
youenn fablet
Comment 14 2016-06-08 06:20:44 PDT
(In reply to comment #12) > nit: Please remove unrelated whitespace-only changes. Done
WebKit Commit Bot
Comment 15 2016-06-08 06:22:33 PDT
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 16 2016-06-08 09:51:09 PDT
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
Comment 17 2016-06-08 12:13:01 PDT
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
Comment 18 2016-06-08 23:23:13 PDT
Created attachment 280885 [details] Patch for landing
WebKit Commit Bot
Comment 19 2016-06-08 23:53:57 PDT
Comment on attachment 280885 [details] Patch for landing Clearing flags on attachment: 280885 Committed r201856: <http://trac.webkit.org/changeset/201856>
WebKit Commit Bot
Comment 20 2016-06-08 23:54:03 PDT
All reviewed patches have been landed. Closing bug.
David Kilzer (:ddkilzer)
Comment 21 2016-06-10 11:35:32 PDT
Build fix for Apple internal iOS build: <https://trac.webkit.org/r201870>
youenn fablet
Comment 22 2016-06-10 11:41:12 PDT
Thanks!
Note You need to log in before you can comment on or make changes to this bug.