We need a way to serialize the DestinationColorSpace when structured cloning.
<rdar://problem/83293533>
Created attachment 438634 [details] Patch for EWS
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Created attachment 438635 [details] Patch for EWS
Created attachment 438637 [details] Patch
I'm including the fix for the flaky WPT in https://github.com/web-platform-tests/wpt/pull/30844, which is the export from bug 229022, since without the fix that PR is blocked from merging.
Comment on attachment 438637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438637&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:36 > +#if PLATFORM(COCOA) > +#include <CoreFoundation/CoreFoundation.h> > +#endif > +#if USE(CG) > +#include <CoreGraphics/CoreGraphics.h> > +#endif I would put these under the rest of the #includes (they sort down their due to the "<"). > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1111 > - PixelBufferFormat format { AlphaPremultiplication::Premultiplied, PixelFormat::RGBA8, DestinationColorSpace::SRGB() }; > + PixelBufferFormat format { AlphaPremultiplication::Premultiplied, PixelFormat::RGBA8, buffer->colorSpace() }; Probably worth putting a comment about adding support for pixel format preservation (or adding that now if it is easy enough, not sure what the canonical representations are, perhaps the CoreVideo 4 character codes?) > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1624 > + auto colorSpace = destinationColorSpace.platformColorSpace(); > + if (!colorSpace) { > + write(DestinationColorSpaceCGColorSpaceNullTag); > + return; > + } I tried to make it so DestinationColorSpace never had a null CGColorSpaceRef (and people would use std::optional<DestinationColorSpace> for a nullable one). Do you know of any cases where this is happening? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1667 > +#endif > +#else > + switch (destinationColorSpace.platformColorSpace()) { > + case PlatformColorSpace::Name::SRGB: > + write(DestinationColorSpaceSRGBTag); > + break; > +#if ENABLE(DESTINATION_COLOR_SPACE_LINEAR_SRGB) > + case PlatformColorSpace::Name::LinearSRGB: > + write(DestinationColorSpaceLinearSRGBTag); > + break; > +#endif > +#if ENABLE(DESTINATION_COLOR_SPACE_DISPLAY_P3) > + case PlatformColorSpace::Name::DisplayP3: > + write(DestinationColorSpaceDisplayP3Tag); > + break; > +#endif One option to make the common cases faster would be to have these special tags for both Cocoa and non-Cocoa (but run before the color space code above). That way, we only encode the tag in the common case, and we have better code coverage for these on all platforms.
(In reply to Sam Weinig from comment #7) > I tried to make it so DestinationColorSpace never had a null CGColorSpaceRef > (and people would use std::optional<DestinationColorSpace> for a nullable > one). Do you know of any cases where this is happening? I don't, but I saw some code somewhere that was checking for it explicitly. I'll consider it more of an error case then, and add some assertions.
Created attachment 439426 [details] Patch
Committed r283193 (242240@main): <https://commits.webkit.org/242240@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439426 [details].
(In reply to Cameron McCormack (:heycam) from comment #8) > (In reply to Sam Weinig from comment #7) > > I tried to make it so DestinationColorSpace never had a null CGColorSpaceRef > > (and people would use std::optional<DestinationColorSpace> for a nullable > > one). Do you know of any cases where this is happening? > > I don't, but I saw some code somewhere that was checking for it explicitly. Where was that?
(In reply to Sam Weinig from comment #11) > (In reply to Cameron McCormack (:heycam) from comment #8) > > (In reply to Sam Weinig from comment #7) > > > I tried to make it so DestinationColorSpace never had a null CGColorSpaceRef > > > (and people would use std::optional<DestinationColorSpace> for a nullable > > > one). Do you know of any cases where this is happening? > > > > I don't, but I saw some code somewhere that was checking for it explicitly. > > Where was that? I think it was my misreading of DestinationColorSpace::decode's `if (!platformColorSpace)` as being a check for the PlatformColorSpace being null, rather than whether the decoder succeeded. In the end I didn't add any new assertions, as I saw you already had one in the DestinationColorSpace constructor.
Great. Thanks.