Summary: | Add a DOMPromiseDeferred method to handle ExceptionOr<> results | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | beidson, buildbot, cdumez, commit-queue, darin, sam, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
youenn fablet
2017-08-15 15:55:23 PDT
Created attachment 318191 [details]
Patch
Attachment 318191 [details] did not pass style-queue:
ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:67: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 318269 [details]
Fixing Windows
Attachment 318269 [details] did not pass style-queue:
ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:67: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Still red bubbles. Created attachment 318274 [details]
Patch
Created attachment 318275 [details]
Patch
Attachment 318275 [details] did not pass style-queue:
ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:67: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 318275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318275&action=review > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:36 > +std::optional<Exception> CacheStorageConnection::convertErrorIntoException(Error error) Do we really needed an Error type? Cannot we just use Exception type everywhere? This would avoid having to convert. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:44 > + result = std::nullopt; Not needed. Comment on attachment 318275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318275&action=review > Source/WebCore/Modules/cache/Cache.cpp:275 > +void Cache::batchDeleteOperation(const FetchRequest& request, CacheQueryOptions&& options, WTF::Function<void(ExceptionOr<bool>&&)>&& callback) I like that we're using ExceptionOr<> here as this avoids returning a result parameter unnecessarily in case of error.... > Source/WebCore/Modules/cache/Cache.cpp:280 > + callback(CacheStorageConnection::exceptionOrResult(!records.isEmpty(), error)); .. However, this only moved the issue since we still have to provide both a result and an error here. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:43 > + case Error::None: I really think we should drop this enum value in a follow-up and use std::optional<>. Then the call sites could boolean check the std::optional directly. > Source/WebCore/Modules/cache/CacheStorageConnection.h:50 > + template<typename T> static ExceptionOr<T> exceptionOrResult(T&& value, Error error) This is not very scalable as you force the call site to construct a value/result it likely does not have in the case an error happened. Works OK now because you're only dealing with booleans. It is also confusing to have to pass both a result and an error while in practice they are mutually exclusive. Comment on attachment 318275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318275&action=review > Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:216 > + void settle(ExceptionOr<typename IDLType::ParameterType>&& result) I also like the part of the change. (In reply to Chris Dumez from comment #11) > Comment on attachment 318275 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318275&action=review > > > Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:216 > > + void settle(ExceptionOr<typename IDLType::ParameterType>&& result) > > I also like the part of the change. *this* part of the change. (In reply to Chris Dumez from comment #9) > Comment on attachment 318275 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318275&action=review > > > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:36 > > +std::optional<Exception> CacheStorageConnection::convertErrorIntoException(Error error) > > Do we really needed an Error type? Cannot we just use Exception type > everywhere? This would avoid having to convert. An Exception requires passing a String through IPC or through workers while it might not be needed usually. That is the main reason. Maybe this is not worth optimizing though. The other reason is that I plan to use Error codes in the storage engine and it might not make sense to use ExceptionCode there. That said, it might be better for the storage engine to have its own error codes in which case WK2 CacheStorageConnection would translate these into an Exception. This patch is an improvement and allows already to isolate the main functions from CacheStorageConnection::Error. Comment on attachment 318275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318275&action=review The "settle" idea is great. I like Chris’s suggestion of using std::optional instead of a error called "none" and the idea to reduce or remove the Error enumeration entirely if possible. > Source/WebCore/Modules/cache/CacheStorageConnection.h:49 > + static std::optional<Exception> convertErrorIntoException(Error); std::optional<Exception> and ExceptionOr<void> are nearly identical. We should not use both. I suggest merging exceptionIfAny and convertErrorIntoException into a single function; and call it convertErrorIntoException, convertToException, or exception. Then your exceptionOrResult function, if you choose to keep it, can just call that. It should be easy and clear to make an ExceptionOr<T> out of an ExceptionOr<void> rather than also using std::optional<Exception>. (In reply to Darin Adler from comment #14) > Comment on attachment 318275 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318275&action=review > > The "settle" idea is great. I like Chris’s suggestion of using std::optional > instead of a error called "none" and the idea to reduce or remove the Error > enumeration entirely if possible. Yes, that is the plan. This would mean updating CacheStorageConnection to either pass an ExceptionOr<> or something like CacheStorageConnection::ErrorOr<>. WK2 specific implementation of CacheStorageConnection is following that model in bug 175640. Created attachment 318378 [details]
Patch for landing
Attachment 318378 [details] did not pass style-queue:
ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:59: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 318378 [details] Patch for landing Clearing flags on attachment: 318378 Committed r220863: <http://trac.webkit.org/changeset/220863> All reviewed patches have been landed. Closing bug. |