241407 2022-06-08 01:28:54 -0700 SharedBuffer IPC encoders have duplicated and redundant code 2022-06-13 05:13:22 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=241416 InRadar P2 Normal --- 1 kkinnunen jean-yves.avenard jean-yves.avenard kkinnunen webkit-bug-importer oldest_to_newest 1874667 0 kkinnunen 2022-06-08 01:28:54 -0700 SharedBuffer IPC encoders have duplicated and redundant code There are multiple redundant top-level argument coders specialised for Ref,RefPtr: template<> struct ArgumentCoder<RefPtr<WebCore::FragmentedSharedBuffer>> { static void encode(Encoder&, const RefPtr<WebCore::FragmentedSharedBuffer>&); static std::optional<RefPtr<WebCore::FragmentedSharedBuffer>> decode(Decoder&); }; template<> struct ArgumentCoder<Ref<WebCore::FragmentedSharedBuffer>> { static void encode(Encoder&, const Ref<WebCore::FragmentedSharedBuffer>&); static std::optional<Ref<WebCore::FragmentedSharedBuffer>> decode(Decoder&); }; template<> struct ArgumentCoder<RefPtr<WebCore::SharedBuffer>> { static void encode(Encoder&, const RefPtr<WebCore::SharedBuffer>&); static std::optional<RefPtr<WebCore::SharedBuffer>> decode(Decoder&); }; Instead, there should be top-level argument coders only for the root types: template<> struct ArgumentCoder<WebCore::FragmentedSharedBuffer> { static void encode(Encoder&, const WebCore::FragmentedSharedBuffer&); static std::optional<Ref<WebCore::FragmentedSharedBuffer>> decode(Decoder&); }; template<> struct ArgumentCoder<WebCore::SharedBuffer> { static void encode(Encoder&, const WebCore::SharedBuffer&); static std::optional<Ref<WebCore::SharedBuffer>> decode(Decoder&); }; The encoder implementations use multiple redundant functions and duplicated logic for encoding and decoding static void encodeSharedBuffer(Encoder& encoder, const FragmentedSharedBuffer* buffer) static void encodeTypesAndData(Encoder& encoder, const Vector<String>& types, const Vector<RefPtr<SharedBuffer>>& data) Instead, the implementations should be of form: ArgumentCoder<WebCore::FragmentedSharedBuffer>::encode(Encoder&, const WebCore::FragmentedSharedBuffer& buffer) { encode the buffer } Generic Vector<T> encoders should be used to encode vectors of buffers, avoiding duplication of "encode size, for each buffer encode the buffer". Generic RefPtr<T> encoders should be used to encode the refptrs of buffers, avoiding duplication of "if exists then encode true and buffer else encode false". The decoder implementations use multiple redundant functions and duplicated logic static WARN_UNUSED_RETURN bool decodeSharedBuffer(Decoder& decoder, RefPtr<SharedBuffer>& buffer) if (!decodeSharedBuffer(decoder, content.dataInWebArchiveFormat)) return false; instead, the decoding should be of form: std::optional<PasteboardWebContent> ArgumentCoder<PasteboardWebContent>::decode(Decoder& decoder) { auto dataInWebArchievFormat = decoder.decode<RefPtr<SharedBuffer>>(); ..... if (UNLIKELY(!decoder.isValid()) return std::nullopt; return PasteboardWebContent { ...., WTFMove(*dataInWebArchiveFormat), ... }; } 1874690 1 webkit-bug-importer 2022-06-08 04:28:05 -0700 <rdar://problem/94621576> 1874709 2 jean-yves.avenard 2022-06-08 06:43:47 -0700 Pull request: https://github.com/WebKit/WebKit/pull/1377 1875506 3 ews-feeder 2022-06-13 05:13:21 -0700 Committed r295487 (251492@main): <https://commits.webkit.org/251492@main> Reviewed commits have been landed. Closing PR #1377 and removing active labels.