Bug 175603

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 Flags
Patch
none
Fixing Windows
none
Patch
none
Patch
none
Patch for landing none

Description youenn fablet 2017-08-15 15:55:23 PDT
Given an ExceptionOr<T>, a promise could do:
- reject if there is an exception
- resolve with t otherwise
Comment 1 youenn fablet 2017-08-15 16:12:32 PDT
Created attachment 318191 [details]
Patch
Comment 2 Build Bot 2017-08-15 16:14:18 PDT
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.
Comment 3 youenn fablet 2017-08-16 10:13:56 PDT
Created attachment 318269 [details]
Fixing Windows
Comment 4 Build Bot 2017-08-16 10:14:53 PDT
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.
Comment 5 Chris Dumez 2017-08-16 10:29:42 PDT
Still red bubbles.
Comment 6 youenn fablet 2017-08-16 10:55:12 PDT
Created attachment 318274 [details]
Patch
Comment 7 youenn fablet 2017-08-16 10:58:13 PDT
Created attachment 318275 [details]
Patch
Comment 8 Build Bot 2017-08-16 11:01:24 PDT
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 9 Chris Dumez 2017-08-16 14:31:20 PDT
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 10 Chris Dumez 2017-08-16 15:01:12 PDT
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 11 Chris Dumez 2017-08-16 15:03:59 PDT
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.
Comment 12 Chris Dumez 2017-08-16 15:04:18 PDT
(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.
Comment 13 youenn fablet 2017-08-16 16:17:55 PDT
(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 14 Darin Adler 2017-08-17 09:07:37 PDT
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>.
Comment 15 youenn fablet 2017-08-17 09:29:44 PDT
(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.
Comment 16 youenn fablet 2017-08-17 10:29:23 PDT
Created attachment 318378 [details]
Patch for landing
Comment 17 Build Bot 2017-08-17 10:31:05 PDT
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 18 WebKit Commit Bot 2017-08-17 11:07:30 PDT
Comment on attachment 318378 [details]
Patch for landing

Clearing flags on attachment: 318378

Committed r220863: <http://trac.webkit.org/changeset/220863>
Comment 19 WebKit Commit Bot 2017-08-17 11:07:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2017-08-17 11:08:21 PDT
<rdar://problem/33944908>