Bug 180689 – Remove ColorSpaceDeviceRGB and most users of the obsolete deviceRGB colorspace

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Remove ColorSpaceDeviceRGB and most users of the obsolete deviceRGB colorspace

[Simon Fraser (smfr)]

Created attachment 329072 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 329113 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 329121 [details]
Patch

[EWS Watchlist]

Attachment 329121 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/DragImageCGWin.cpp:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/win/DragImageCGWin.cpp:77:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 329121 [details]
Patch

Clearing flags on attachment: 329121

Committed r225797: <https://trac.webkit.org/changeset/225797>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36002058>

[Darin Adler]

Comment on attachment 329121 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:94
> +    return CGColorSpaceCreateDeviceRGB();

This isn’t quite right. These functions are not supposed to create a new CGColorSpace every time the function is called. Instead they are supposed to cache one only the first time the function is called and keep returning it. Please do that.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:98
> +    static CGColorSpaceRef linearRGBSpace;
> +    linearRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceLinearSRGB);
> +    return linearRGBSpace;

This is incorrect; we are now calling CGColorSpaceCreateWithName every time this function is called instead of caching its result the first time this function is called!

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:112
>      // If there is no support for exteneded sRGB, fall back to sRGB.
>      if (!extendedSRGBSpace)
> -        extendedSRGBSpace = sRGBColorSpaceRef();
> +        extendedSRGBSpace = CGColorSpaceCreateDeviceRGB();
>      return extendedSRGBSpace;

I don’t understand the rationale behind this change. Why would we use device RGB here? Also, why leave the comment saying we fall back to sRGB when changing the code to no longer do that?

Also, I see another mistake above in the earlier part of the function that we did not modify. We call CGColorSpaceCreateWithName every time this function is called instead of caching its result the first time this function is called. That’s the same bug we introduced in other functions, but I guess it was already present in this function.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:122
> +    static CGColorSpaceRef displayP3Space;
>  #if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED > 101100)
> -    static CGColorSpaceRef displayP3Space = CGColorSpaceCreateWithName(kCGColorSpaceDisplayP3);
> +    displayP3Space = CGColorSpaceCreateWithName(kCGColorSpaceDisplayP3);
>  #else
> -    static CGColorSpaceRef displayP3Space = sRGBColorSpaceRef();
> +    displayP3Space = sRGBColorSpaceRef();
>  #endif

This is incorrect; we are now calling CGColorSpaceCreateWithName every time this function is called instead of caching its result the first time this function is called!

[Simon Fraser (smfr)]

Looks like I shouldn't do patches while under the influence of a cold!

[Simon Fraser (smfr)]

Doing to use dispatch_once to fix this mess.

[Simon Fraser (smfr)]

Created attachment 329354 [details]
Patch

[Tim Horton]

Your reviewer didn’t have a cold :(

[Simon Fraser (smfr)]

Created attachment 329356 [details]
Patch

[Darin Adler]

Comment on attachment 329356 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Existing and new code mistakenly allocated colorspaces on every call, because
> +        they didn't initialize the static variable on the first call. Avoid this mistake
> +        by using dispatch_once() in these functions.

Seems like overkill to me, but I guess this does make them thread-safe whereas before they could only be safely called on the main thread.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:102
> +        linearRGBColorSpace sRGBColorSpaceRef();

Looks like this is missing an "="

[Darin Adler]

Comment on attachment 329356 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:120
> +        // If there is no support for exteneded sRGB, fall back to sRGB.

Oops, forgot to point this out when I spotted it earlier, there was a typo in this old comment: "exteneded"

[Simon Fraser (smfr)]

https://trac.webkit.org/r225915