Bug 230907 - Add support for sending Expected<void, E> in IPC messages
Summary: Add support for sending Expected<void, E> in IPC messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-28 11:53 PDT by BJ Burg
Modified: 2021-09-29 08:40 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.23 KB, patch)
2021-09-28 13:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (20.90 KB, patch)
2021-09-28 18:03 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2021-09-28 11:53:57 PDT
While developing WebInspectorUIExtensionController it became apparent that Expected<void, Inspector::ExtensionError> does not work as expected. The first of many compilation errors looks something like:

In file included from /Users/bburg/repos/webkit/OpenSource/WebKitBuild/Release/DerivedSources/WebKit2/WebInspectorUIExtensionControllerMessageReceiver.cpp:29:
/Users/bburg/repos/webkit/OpenSource/Source/WebKit/Platform/IPC/ArgumentCoders.h:658:38: note: in instantiation of template class 'std::optional<void>' requested here
            std::optional<ValueType> value;
In file included from /Users/bburg/repos/webkit/OpenSource/WebKitBuild/Release/DerivedSources/WebKit2/WebInspectorUIExtensionControllerMessageReceiver.cpp:27:
In file included from /Users/bburg/repos/webkit/OpenSource/Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.h:30:
In file included from /Users/bburg/repos/webkit/OpenSource/Source/WebKit/Platform/IPC/Connection.h:31:
/Users/bburg/repos/webkit/OpenSource/Source/WebKit/Platform/IPC/Decoder.h:114:23: note: in instantiation of member function 'IPC::ArgumentCoder<std::experimental::expected<void, Inspector::ExtensionError>>::decode' requested here
            t = Impl::decode(*this);
/Users/bburg/repos/webkit/OpenSource/WebKitBuild/Release/DerivedSources/WebKit2/WebInspectorUIExtensionControllerMessageReceiver.cpp:180:13: note: in instantiation of function template specialization 'IPC::Decoder::operator>><std::experimental::expected<void, Inspector::ExtensionError>>' requested here
    decoder >> result;


I believe this can be fixed with a special-cased implementation in wtf/persistence/PersistentCoders.h
Comment 1 Devin Rousso 2021-09-28 13:32:27 PDT
Created attachment 439513 [details]
Patch
Comment 2 Chris Dumez 2021-09-28 13:34:52 PDT
Comment on attachment 439513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439513&action=review

> Source/WTF/ChangeLog:3
> +        Add support for sending Expected<void, E> in IPC messages

This says IPC

> Source/WTF/ChangeLog:8
> +        * wtf/persistence/PersistentCoders.h:

But it updates persistent coders that are used for serializing to disk.

Which one is right? Hard to say given that it is unused in this patch.
Comment 3 Devin Rousso 2021-09-28 13:40:28 PDT
oh whoops I put this in the wrong spot

good catch @Chris Dumez!
Comment 4 Devin Rousso 2021-09-28 13:43:10 PDT
ill just wait for the related patch to land and then move this to the right spot :)
Comment 5 Chris Dumez 2021-09-28 13:49:35 PDT
Comment on attachment 439513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439513&action=review

> Source/WTF/wtf/persistence/PersistentCoders.h:121
> +            return std::optional<Expected<T, E>> { Unexpected<E> { WTFMove(*error) } };

Same comment as below.

> Source/WTF/wtf/persistence/PersistentCoders.h:128
> +        return std::optional<Expected<T, E>> { Expected<T, E> { WTFMove(*value) } };

Same comment as below.

> Source/WTF/wtf/persistence/PersistentCoders.h:156
> +            return std::optional<Expected<void, E>> { Unexpected<E> { WTFMove(*error) } };

I believe the following should work:
return Expected<void, E> { Unexpected<E> { WTFMove(*error) } };

> Source/WTF/wtf/persistence/PersistentCoders.h:159
> +        return std::optional<Expected<void, E>> { Expected<void, E> { } };

We probably don't see `std::optional<Expected<void, E>> { }`
Comment 6 Devin Rousso 2021-09-28 18:03:34 PDT
Created attachment 439551 [details]
Patch
Comment 7 BJ Burg 2021-09-28 22:52:21 PDT
Comment on attachment 439551 [details]
Patch

Web Inspector-related changes look good to me. r+ if Chris is OK with the ArgumentCoder changes.
Comment 8 EWS 2021-09-29 08:39:33 PDT
Committed r283220 (242262@main): <https://commits.webkit.org/242262@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439551 [details].
Comment 9 Radar WebKit Bug Importer 2021-09-29 08:40:16 PDT
<rdar://problem/83672326>