Bug 176043

Summary: Begin transition to modern IPC decoding
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dbates, jfbastien, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Alex Christensen 2017-08-28 18:12:19 PDT
Begin transition to modern IPC decoding
Comment 1 Alex Christensen 2017-08-28 18:19:25 PDT
Created attachment 319225 [details]
Patch
Comment 2 Alex Christensen 2017-08-29 11:56:39 PDT
Created attachment 319269 [details]
Patch
Comment 3 Sam Weinig 2017-08-29 13:22:52 PDT
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...
Comment 4 Sam Weinig 2017-08-29 13:23:49 PDT
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.
Comment 5 Alex Christensen 2017-08-29 13:36:56 PDT
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<<
Comment 6 Alex Christensen 2017-08-29 14:05:22 PDT
(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.
Comment 7 Sam Weinig 2017-08-29 14:12:38 PDT
(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.
Comment 8 Alex Christensen 2017-08-29 15:04:50 PDT
Created attachment 319288 [details]
Patch
Comment 9 Build Bot 2017-08-29 15:06:18 PDT
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.
Comment 10 JF Bastien 2017-08-29 15:43:39 PDT
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.
Comment 11 Alex Christensen 2017-08-29 15:49:32 PDT
Created attachment 319295 [details]
Patch
Comment 12 Alex Christensen 2017-08-29 15:50:04 PDT
Comment on attachment 319295 [details]
Patch

http://trac.webkit.org/r221319
Comment 13 Daniel Bates 2017-09-12 20:59:27 PDT
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.
Comment 14 Radar WebKit Bug Importer 2017-09-27 12:52:19 PDT
<rdar://problem/34694179>