Bug 180689 - Remove ColorSpaceDeviceRGB and most users of the obsolete deviceRGB colorspace
Summary: Remove ColorSpaceDeviceRGB and most users of the obsolete deviceRGB colorspace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-11 18:19 PST by Simon Fraser (smfr)
Modified: 2017-12-14 11:35 PST (History)
7 users (show)

See Also:


Attachments
Patch (16.43 KB, patch)
2017-12-11 18:32 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (17.84 KB, patch)
2017-12-12 08:09 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (25.40 KB, patch)
2017-12-12 10:10 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (7.47 KB, patch)
2017-12-14 09:01 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (7.47 KB, patch)
2017-12-14 09:27 PST, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2017-12-11 18:19:32 PST
Remove ColorSpaceDeviceRGB and most users of the obsolete deviceRGB colorspace
Comment 1 Simon Fraser (smfr) 2017-12-11 18:32:43 PST
Created attachment 329072 [details]
Patch
Comment 2 Simon Fraser (smfr) 2017-12-12 08:09:37 PST
Created attachment 329113 [details]
Patch
Comment 3 Simon Fraser (smfr) 2017-12-12 10:10:44 PST
Created attachment 329121 [details]
Patch
Comment 4 EWS Watchlist 2017-12-12 10:14:56 PST
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.
Comment 5 WebKit Commit Bot 2017-12-12 13:00:42 PST
Comment on attachment 329121 [details]
Patch

Clearing flags on attachment: 329121

Committed r225797: <https://trac.webkit.org/changeset/225797>
Comment 6 WebKit Commit Bot 2017-12-12 13:00:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2017-12-12 13:01:32 PST
<rdar://problem/36002058>
Comment 8 Darin Adler 2017-12-13 21:34:36 PST
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!
Comment 9 Simon Fraser (smfr) 2017-12-14 08:15:46 PST
Looks like I shouldn't do patches while under the influence of a cold!
Comment 10 Simon Fraser (smfr) 2017-12-14 08:31:06 PST
Doing to use dispatch_once to fix this mess.
Comment 11 Simon Fraser (smfr) 2017-12-14 09:01:52 PST
Created attachment 329354 [details]
Patch
Comment 12 Tim Horton 2017-12-14 09:13:10 PST
Your reviewer didn’t have a cold :(
Comment 13 Simon Fraser (smfr) 2017-12-14 09:27:52 PST
Created attachment 329356 [details]
Patch
Comment 14 Darin Adler 2017-12-14 09:35:28 PST
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 "="
Comment 15 Darin Adler 2017-12-14 09:37:23 PST
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"
Comment 16 Simon Fraser (smfr) 2017-12-14 11:35:29 PST
https://trac.webkit.org/r225915