[webkit-reviews] review granted: [Bug 226143] Convert DestinationColorSpace from an enum to class wrapping a platform color space (CGColorSpaceRef for CG ports, etc.) : [Attachment 429470] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 226143: Convert DestinationColorSpace from an enum to class wrapping a
platform color space (CGColorSpaceRef for CG ports, etc.)
https://bugs.webkit.org/show_bug.cgi?id=226143

Attachment 429470: Patch

https://bugs.webkit.org/attachment.cgi?id=429470&action=review




--- Comment #16 from Darin Adler <darin at apple.com> ---
Comment on attachment 429470
  --> https://bugs.webkit.org/attachment.cgi?id=429470
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429470&action=review

Is it OK to make this code less optimal? Would love to stay super-efficient for
cases where these are the most common color spaces.

> Source/WebCore/Headers.cmake:1297
> +    platform/graphics/PlatformColorSpace.h

Not a huge fan of the word "platform". Always confused about what exactly it
means.

> Source/WebCore/platform/graphics/DestinationColorSpace.h:36
> +    WEBCORE_EXPORT static DestinationColorSpace SRGB();

Sure would be nice if functional ike this could be super-efficient, like
constexpr. But instead it’s a function that calls another function to fetch the
value of a global variable. Still not too bad, but for an idiom where we are
constantly calling this, seems like it would be nice if this was something that
could compile down to nothing.

> Source/WebCore/platform/graphics/DestinationColorSpace.h:44
> +    WEBCORE_EXPORT explicit DestinationColorSpace(PlatformColorSpace);

Doesn’t seem like this needs to be explicit. It’s a lossless conversion.

But maybe the agenda is to make this class contain more over time, and
eventually the explicit will make sense?

> Source/WebCore/platform/graphics/DestinationColorSpace.h:45
> +    PlatformColorSpaceValue platformColorSpace() const { return
m_platformColorSpace.get(); }

Seems unfortunate that this function name has to repeat the words "color
space". But we don’t have a great naming scheme for this sort of thing:
"underlying platform-specific representation of this platform-independent
wrapper".

> Source/WebCore/platform/graphics/PlatformColorSpace.h:41
> +using PlatformColorSpace = RetainPtr<CGColorSpaceRef>;
> +using PlatformColorSpaceValue = CGColorSpaceRef;

Wish we had a better idiom to relate these pairs of types. The "main type" and
the "peek type", to use our hash table value parlance.

> Source/WebCore/platform/graphics/PlatformColorSpace.h:47
> +    enum class Name {

Maybe ": uint8_t"?

> Source/WebCore/platform/graphics/PlatformColorSpace.h:85
> +    return PlatformColorSpace { *name };

Could also use:

    return { { *name } };

> Source/WebCore/platform/graphics/RemoteVideoSample.cpp:59
> +    auto ioSurface =  IOSurface::create(size, DestinationColorSpace::SRGB(),
IOSurface::Format::BGRA);

Extra space here after "="?

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:890
> +    BeginClipToDrawingCommands(const FloatRect& destination, const
DestinationColorSpace& colorSpace)
>	   : m_destination(destination)
>	   , m_colorSpace(colorSpace)
>      {
>      }

Do we need this? it’s just convenience because the caller can copy and then
call the rvalue reference version of the constructor. In fact, can also write
this:

    BeginClipToDrawingCommands(const FloatRect& destination, const
DestinationColorSpace& colorSpace)
	: BeginClipToDrawingCommands { destination, DestinationColorSpace {
colorSpace } }
    {
    }

To avoid repeating anything at all. Might put the rvalue reference version
first and treat it as the "main" version.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1985
> +    GetPixelBuffer(const PixelBufferFormat& outputFormat, const IntRect&
srcRect)
>	   : m_srcRect(srcRect)
>	   , m_outputFormat(outputFormat)
>      {
>      }

Same here.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1996
> +    ~GetPixelBuffer() { }

= default instead? Or does that not force a non-trivial destructor? Surprised
that this qualifies as non-trivial.