.
Created attachment 440670 [details] For EWS
Comment on attachment 440670 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=440670&action=review Thanks for the review! > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1165 > std::optional<FloatRect> globalBounds() const { return std::nullopt; } (It just occurred to me that I forgot to remove the now-unnecessary `isValid()` method in DrawPattern, so I'll do that before landing)
Created attachment 440674 [details] For EWS
Created attachment 440696 [details] Fix failing tests
Comment on attachment 440696 [details] Fix failing tests View in context: https://bugs.webkit.org/attachment.cgi?id=440696&action=review > Source/WebCore/ChangeLog:8 > + These three display list items all contain ImagePaintingOptions, which (in turn) contains strongly typed enums. nit: ImagePaintingOptions is plural, so it should use "contain" instead of "contains." Or maybe "Each of which contain" > Source/WebCore/ChangeLog:13 > + we should just make them non-inline and implement encoding/decoding logic as static methods on the items This is unclear. If they're just enums, why should they be out-of-line items? Enums are tiny.
Comment on attachment 440696 [details] Fix failing tests View in context: https://bugs.webkit.org/attachment.cgi?id=440696&action=review >> Source/WebCore/ChangeLog:8 >> + These three display list items all contain ImagePaintingOptions, which (in turn) contains strongly typed enums. > > nit: ImagePaintingOptions is plural, so it should use "contain" instead of "contains." Or maybe "Each of which contain" Fixed! >> Source/WebCore/ChangeLog:13 >> + we should just make them non-inline and implement encoding/decoding logic as static methods on the items > > This is unclear. If they're just enums, why should they be out-of-line items? Enums are tiny. The issue is that we don't have a mechanism for validating the underlying types of these strongly typed enums when `memcpy`-ing these items cross-process, which triggers UB. It's technically possible to address this by storing all of these as the underlying type directly (e.g. `uint8_t`) when sending them to the GPUP so we can validate the values, but in the streamable IPC world, this won't really make a difference.
Created attachment 440713 [details] Fix ChangeLog typo
Comment on attachment 440713 [details] Fix ChangeLog typo View in context: https://bugs.webkit.org/attachment.cgi?id=440713&action=review > Source/WebCore/ChangeLog:14 > + Instead of adding SimpleArgumentCoder support for these display list item types when implementing streaming IPC, > + we should just make them non-inline and implement encoding/decoding logic as static methods on the items > + themselves. Why is it better to make them out of line than add the enum traits / argument coder support for these enums?
Comment on attachment 440713 [details] Fix ChangeLog typo View in context: https://bugs.webkit.org/attachment.cgi?id=440713&action=review >> Source/WebCore/ChangeLog:14 >> + themselves. > > Why is it better to make them out of line than add the enum traits / argument coder support for these enums? Hm.. not sure what you mean by this — we already have enum traits / argument coder support for these enums (and the options struct). Making these items "out of line" means making these display list items work with argument coders, as opposed to being memcpy'd directly in and out of shared memory. Currently, we only do the latter, which bypasses validation for these enum types.
I think I'm going to adjust my approach here, such that I won't require this patch.