Introduce SerializingTuple to autogenerate IPC code
Created attachment 383809 [details] Patch
Comment on attachment 383809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383809&action=review > Source/WebKit/Shared/FrameInfoData.cpp:33 > void FrameInfoData::encode(IPC::Encoder& encoder) const SO CLOSE now we just need to figure out how to get rid of these. > Source/WebKit/UIProcess/API/APIFrameInfo.cpp:43 > + WebKit::FrameInfoData frameInfoData {{{ Let's just throw some more curly braces in here for good luck? > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:192 > + return FrameInfoData {{{ > + isMainFrame(), I don't looooove the positional-ness of this. Names are nice.
Comment on attachment 383809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383809&action=review > Source/WebKit/Shared/FrameInfoData.cpp:-56 > - Optional<WebCore::SecurityOriginData> securityOrigin; > - decoder >> securityOrigin; > - if (!securityOrigin) > - return WTF::nullopt; Before if any subtype failed to decode we'd fail the whole thing immediately without continuing. Is that still the case?
Comment on attachment 383809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383809&action=review >> Source/WebKit/Shared/FrameInfoData.cpp:33 >> void FrameInfoData::encode(IPC::Encoder& encoder) const > > SO CLOSE now we just need to figure out how to get rid of these. You can also inherit from SerializingTuple then add getters and setters, but then you need all the member serializers to be visible when instantiating the template in the header. That wasn't possible without further modification to how FrameInfoData was used in generated messages code, so I made FrameInfoData have a SerializingTuple member. >> Source/WebKit/Shared/FrameInfoData.cpp:-56 >> - return WTF::nullopt; > > Before if any subtype failed to decode we'd fail the whole thing immediately without continuing. Is that still the case? That is still the case. >> Source/WebKit/UIProcess/API/APIFrameInfo.cpp:43 >> + WebKit::FrameInfoData frameInfoData {{{ > > Let's just throw some more curly braces in here for good luck? It's needed to call the std::tuple constructor, then call the SerializingTuple constructor with that std::tuple, then call the FrameInfoData constructor with that SerializingTuple. It can slightly be improved, but triple braces aren't the worst thing ever.
Comment on attachment 383809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383809&action=review > Source/WebKit/Shared/FrameInfoData.cpp:44 > + return {{{ WTFMove(*data) }}}; FrameInfoData is a lightweight struct and SerializingTuple is even lighter struct. So having this deep nesting architecture does not look a right design to me. Also I think you still need two {{ }} only here. The inner one creates the FrameInfoData from *data. The outer one creates the Optional<FrameInfoData>. > Source/WebKit/Shared/SerializingTuple.h:52 > +}; Some comments about this. struct: 1. I do not think it is worthy having the std::tuple be encapsulated in a SerializingTuple. The only thing that forces this is the function encode(). So I would suggest having this struct non construct-able. And having both encode() and decode() be static methods. The clients of this struct will store the std::tuple directly without having it inside a SerializingTuple. 2. This template struct could be more generic by having the types of the encoder and the decoder be passed as template typenames. In this case it can be moved to WTF. > Source/WebKit/Shared/SerializingTuple.h:55 > +template<size_t I, typename... Types> > +const typename std::tuple_element<I, std::tuple<Types...> >::type& get(const SerializingTuple<Types...>& tuple) { return std::get<I>(tuple.data); } If you do not like the suggestion above, I think this code is a little bit hard to read. Instead it can be a member of SerializingTuple: template<std::size_t I> const auto& get() const { return std::get<I>(data); } And the caller can call it like this: bool isMainFrame() const { return data.get<0>(); }