Bug 184343 – WebCore::screenColorSpace is retrieving CGColorSpace from NSScreen directly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

We should be brokering the screen's CGColorSpace from the UIProcess to the WebContent process.

[Radar WebKit Bug Importer]

<rdar://problem/39224881>

[Brent Fulgham]

Created attachment 337366 [details]
Patch

[Brent Fulgham]

Created attachment 337369 [details]
Patch

[Per Arne Vollan]

Comment on attachment 337369 [details]
Patch

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

> Source/WebCore/platform/ScreenProperties.h:51
> +    enum EncodedDataType {
> +        Null,
> +        Name,
> +        Data,
> +    };

Perhaps we could indicate that this type is related to color space, for example:

enum EncodedColorSpaceDataType {
    Null,
    ColorSpaceName,
    ColorSpaceData,
};

[Per Arne Vollan]

Comment on attachment 337369 [details]
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.

[Brent Fulgham]

Comment on attachment 337369 [details]
Patch

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

>> Source/WebCore/platform/ScreenProperties.h:51
>> +    };
> 
> Perhaps we could indicate that this type is related to color space, for example:
> 
> enum EncodedColorSpaceDataType {
>     Null,
>     ColorSpaceName,
>     ColorSpaceData,
> };

Sure!

>> Source/WebCore/platform/ScreenProperties.h:126
>> +        cgColorSpace = nullptr;
> 
> This line is probably not strictly needed, since cgColorSpace should already be null.

OK.

>> Source/WebCore/platform/ScreenProperties.h:132
>> +            return std::nullopt;
> 
> It seems we should always have a color space name here, perhaps add an ASSERT?

OK.

>> Source/WebCore/platform/ScreenProperties.h:141
>> +            return std::nullopt;
> 
> Ditto.

Ditto! :-)

[Brent Fulgham]

Committed r230351: <https://trac.webkit.org/changeset/230351>