Bug 175603 – Add a DOMPromiseDeferred method to handle ExceptionOr<> results

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Given an ExceptionOr<T>, a promise could do:
- reject if there is an exception
- resolve with t otherwise

[youenn fablet]

Created attachment 318191 [details]
Patch

[Build Bot]

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.

[youenn fablet]

Created attachment 318269 [details]
Fixing Windows

[Build Bot]

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.

[Chris Dumez]

Still red bubbles.

[youenn fablet]

Created attachment 318274 [details]
Patch

[youenn fablet]

Created attachment 318275 [details]
Patch

[Build Bot]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

(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.

[youenn fablet]

(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.

[Darin Adler]

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>.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 318378 [details]
Patch for landing

[Build Bot]

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.

[WebKit Commit Bot]

Comment on attachment 318378 [details]
Patch for landing

Clearing flags on attachment: 318378

Committed r220863: <http://trac.webkit.org/changeset/220863>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33944908>