[webkit-reviews] review denied: [Bug 176043] Begin transition to modern IPC decoding : [Attachment 319269] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 176043: Begin transition to modern IPC decoding
https://bugs.webkit.org/show_bug.cgi?id=176043

Attachment 319269: Patch

https://bugs.webkit.org/attachment.cgi?id=319269&action=review




--- Comment #3 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 319269
  --> https://bugs.webkit.org/attachment.cgi?id=319269
Patch

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

This looks like a good start.  The syntax I was hoping for eventually was
something like:


std::optional<double> aDouble << decoder;
std::optional<int> anInteger << decoder;
std::optional<WebPageGroupData> aPageGroupData << decoder;

if (!decoder.isCool())
    return std::nullopt;

return { *aDouble, *anInteger, WTFMove(*aPageGroupData) };

Note how no explicit if checks are needed.  The decoder would simply have an "I
have error" bit and do nothing if it was set.

This can probably be improved upon as well, but it would be a nice mirror to
the encoder, which looks like:

encoder << aDouble;
encoder << anInteger;
encoder << aPageGroupData;

> Source/WebKit/Platform/IPC/ArgumentCoder.h:40
> +#define USE_MODERN_DECODER(classname) template<> struct
UsesLegacyDecoder<classname> : public std::false_type { };
> +USE_MODERN_DECODER(WebKit::WebPageCreationParameters);
> +USE_MODERN_DECODER(WebKit::WebPageGroupData);
> +USE_MODERN_DECODER(WebKit::WebsitePolicies);

Why don't you do this in the header of the class? And drop the macro.

> Source/WebKit/Platform/IPC/ArgumentCoder.h:62
> +    static auto decode(Decoder& decoder) ->
std::enable_if_t<!UsesLegacyDecoder<U>::value, std::optional<U>> {
> +	   return U::decode(decoder);
>      }

{
should be on the next line.

> Source/WebKit/Shared/WebPageCreationParameters.cpp:115
> +	   return { };

These should be std::nullopt,

> Source/WebKit/Shared/WebPageCreationParameters.cpp:123
> +    std::optional<WebPageGroupData> pageGroupData;
> +    decoder >> pageGroupData;

This would be nicer if it read

std::optional<WebPageGroupData> pageGroupData << decoder.

Do you think that's doable?

> Source/WebKit/Shared/WebPageGroupData.cpp:59
> +    return {{id, pageGroupID, visibleToInjectedBundle,
visibleToHistoryClient, userContentControllerIdentifier}};

Please put spaces around the parenthesis.  e.g. return { { id, pageGroupID...