WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Trying to fix win build
(19.32 KB, patch)
2016-06-02 06:17 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(65.99 KB, patch)
2016-06-07 02:21 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Win build fix
(66.03 KB, patch)
2016-06-07 03:17 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Win build fix again
(65.91 KB, patch)
2016-06-07 05:16 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Removing whitespace changes
(24.25 KB, patch)
2016-06-08 06:20 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(24.32 KB, patch)
2016-06-08 23:23 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2016-06-02 03:31:41 PDT
Created
attachment 280326
[details]
Patch
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
Created
attachment 280684
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug