Begin transition to modern IPC decoding
Created attachment 319225 [details] Patch
Created attachment 319269 [details] Patch
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 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 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<<
(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.
(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.
Created attachment 319288 [details] Patch
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 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.
Created attachment 319295 [details] Patch
Comment on attachment 319295 [details] Patch http://trac.webkit.org/r221319
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.
<rdar://problem/34694179>