Bug 176043 – Begin transition to modern IPC decoding

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Begin transition to modern IPC decoding

[Alex Christensen]

Created attachment 319225 [details]
Patch

[Alex Christensen]

Created attachment 319269 [details]
Patch

[Sam Weinig]

Comment on attachment 319269 [details]
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...

[Sam Weinig]

Comment on attachment 319269 [details]
Patch

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

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

You might be able to avoid this by checking if decode has a bool return type.

[Alex Christensen]

Comment on attachment 319269 [details]
Patch

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

>> Source/WebKit/Shared/WebPageCreationParameters.cpp:123
>> +    decoder >> pageGroupData;
> 
> This would be nicer if it read
> 
> std::optional<WebPageGroupData> pageGroupData << decoder.
> 
> Do you think that's doable?

That would require overloading operator<< on std::optional, which I don't think is a good thing to do, and I'm not sure we can.  Also, I think having operator>> on decoders mirroring operator<< on encoders is symmetric like std::istream::operator>> and std::ostream::operator<<

[Alex Christensen]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 319269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319269&action=review
> 
> >> Source/WebKit/Shared/WebPageCreationParameters.cpp:123
> >> +    decoder >> pageGroupData;
> > 
> > This would be nicer if it read
> > 
> > std::optional<WebPageGroupData> pageGroupData << decoder.
> > 
> > Do you think that's doable?
> 
> That would require overloading operator<< on std::optional, which I don't
> think is a good thing to do, and I'm not sure we can.  Also, I think having
> operator>> on decoders mirroring operator<< on encoders is symmetric like
> std::istream::operator>> and std::ostream::operator<<

This is impossible even if we had such an operator.  We would need to do this by overloading the std::optional constructor, and we definitely shouldn't do that.

[Sam Weinig]

(In reply to Alex Christensen from comment #6)
> (In reply to Alex Christensen from comment #5)
> > Comment on attachment 319269 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=319269&action=review
> > 
> > >> Source/WebKit/Shared/WebPageCreationParameters.cpp:123
> > >> +    decoder >> pageGroupData;
> > > 
> > > This would be nicer if it read
> > > 
> > > std::optional<WebPageGroupData> pageGroupData << decoder.
> > > 
> > > Do you think that's doable?
> > 
> > That would require overloading operator<< on std::optional, which I don't
> > think is a good thing to do, and I'm not sure we can.  Also, I think having
> > operator>> on decoders mirroring operator<< on encoders is symmetric like
> > std::istream::operator>> and std::ostream::operator<<
> 
> This is impossible even if we had such an operator.  We would need to do
> this by overloading the std::optional constructor, and we definitely
> shouldn't do that.

Yeah, you're right.

[Alex Christensen]

Created attachment 319288 [details]
Patch

[Build Bot]

Attachment 319288 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/IPC/ArgumentCoders.h:146:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit/Platform/IPC/ArgumentCoder.h:47:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit/Platform/IPC/Decoder.h:134:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 319288 [details]
Patch

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

r=me

> Source/WebKit/Platform/IPC/ArgumentCoder.h:46
> +    static auto decode(Decoder& decoder, U& u) -> std::enable_if_t<!UsesModernDecoder<U>::value, bool>

I'd make enable_if a default template parameter instead. De-clutters the return type.

> Source/WebKit/Shared/WebPageCreationParameters.cpp:264
> +    return { WTFMove(parameters) };

Do you need the curls here? Or can you implicitly get optional(U&&) ?

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

I'm not a fan of multi-bools but separate change... I like enum class here.

[Alex Christensen]

Created attachment 319295 [details]
Patch

[Alex Christensen]

Comment on attachment 319295 [details]
Patch

http://trac.webkit.org/r221319

[Daniel Bates]

Marking this bug as Resolved Fixed per comment #12. If this bug is not fixed then please either re-open this bug or file a new bug.

[Radar WebKit Bug Importer]

<rdar://problem/34694179>