RESOLVED FIXED 158299
Introduce ResourceErrorBase::type
https://bugs.webkit.org/show_bug.cgi?id=158299
Summary Introduce ResourceErrorBase::type
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.