Bug 238993

Created attachment 457040 [details]
WIP patch

Created attachment 457325 [details]
WIP patch

This one is still just a prototype, and some feedback on the general idea would be appreciated.


When dispatching synchronous messages, right now the expectation is to also provide a tuple of references to variables where the reply arguments should be stored. This normally requires initializing each such variable either through default initialization or assigning it some default value.

Reply is processed by essentially decoding whatever is sent back and then moving the resulted values into the prepared variables that were passed through the tuple of references.


Proposed change here changes the way the reply values are made available to the caller. Decoding is now done into a single tuple object, and the caller can access those values after validating the message (and reply) went through successfully.

When valid, the reply values stored in the tuple can be referenced or moved out. Through takeReplyArgumentsOr() it's also possible to either move the tuple out of the result struct or construct a tuple of the given default values for each reply argument, depending if the message is valid or not.

It's also possible to combine this with the C++17 structured bindings and std::tie(), establishing references to values inside the reply tuple or simply moving the values out into local variables that can then be moved into further operations. This can clear out the code a bit, avoiding e.g. defining variables that then aren't ever read because of an IPC failure. It could also possibly avoid some extra use of std::optional<> in reply arguments that's nowadays used because the default constructor for a given reply type isn't viable.


One downside is the loss of type info that's currently there for every local reply argument variable. Structured binding leverages auto, so there's no good way of immediately knowing what the specific type is if you're not a compiler. And as with other similar cases in C++, it can be too easy to forget an ampersand and perform a copy instead of establishing a reference for each reply argument.

Created attachment 457326 [details]
WIP patch

Comment on attachment 457326 [details]
WIP patch

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

looks good by me.
maybe we need to still run it by other developers.
marking it as r? to get feedback

> Source/WebKit/ChangeLog:3
> +        [WK2] Use structured bindings to access synchronous-message reply arguments

I think it should highlight that the benefit is perhaps not to use structured bindings per se, rather being able to receive non-default constructible objects and avoid the default construction.

> Source/WebKit/Platform/IPC/Connection.h:270
> +    struct SendSyncReply {

If possible, I think this should be just called SendSyncResult
and as a implementation detail it'd hold the std::unique_ptr<Decoder> m_decoder;
and then the above sendSync should be removed, once all call sites are changed to the other form.

Comment on attachment 457326 [details]
WIP patch

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

>> Source/WebKit/Platform/IPC/Connection.h:270
>> +    struct SendSyncReply {
> 
> If possible, I think this should be just called SendSyncResult
> and as a implementation detail it'd hold the std::unique_ptr<Decoder> m_decoder;
> and then the above sendSync should be removed, once all call sites are changed to the other form.

Should be possible, parallel structure and methods are used right now to keep changes limited to only a couple of places in WebKit.

<rdar://problem/91802564>

Created attachment 458497 [details]
Patch

Created attachment 458503 [details]
Patch

With the WinCairo fix.

I like this direction.

Pull request: https://github.com/WebKit/WebKit/pull/4523

Committed 254783@main (c3ae481a1d05): <https://commits.webkit.org/254783@main>

Reviewed commits have been landed. Closing PR #4523 and removing active labels.