[webkit-reviews] review granted: [Bug 184343] WebCore::screenColorSpace is retrieving CGColorSpace from NSScreen directly : [Attachment 337369] Patch

[bugzilla-daemon at webkit.org]

Per Arne Vollan <pvollan at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 184343: WebCore::screenColorSpace is retrieving CGColorSpace from NSScreen
directly
https://bugs.webkit.org/show_bug.cgi?id=184343

Attachment 337369: Patch

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




--- Comment #5 from Per Arne Vollan <pvollan at apple.com> ---
Comment on attachment 337369
  --> https://bugs.webkit.org/attachment.cgi?id=337369
Patch

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

Looks good!

> Source/WebCore/platform/ScreenProperties.h:126
> +	   cgColorSpace = nullptr;

This line is probably not strictly needed, since cgColorSpace should already be
null.

> Source/WebCore/platform/ScreenProperties.h:132
> +	   if (!colorSpaceName)
> +	       return std::nullopt;

It seems we should always have a color space name here, perhaps add an ASSERT?

> Source/WebCore/platform/ScreenProperties.h:141
> +	   if (!iccData)
> +	       return std::nullopt;

Ditto.