Bug 231992 - [Cocoa] Merge and simplify the nsColor family of functions
Summary: [Cocoa] Merge and simplify the nsColor family of functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-19 15:27 PDT by Darin Adler
Modified: 2021-10-21 11:26 PDT (History)
19 users (show)

See Also:


Attachments
Patch (76.15 KB, patch)
2021-10-20 09:04 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (76.13 KB, patch)
2021-10-20 10:52 PDT, Darin Adler
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (76.13 KB, patch)
2021-10-20 11:24 PDT, Darin Adler
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2021-10-19 15:27:50 PDT
Merge and simplify the nsColor family of functions
Comment 1 Darin Adler 2021-10-20 09:04:52 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2021-10-20 10:52:46 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2021-10-20 11:24:44 PDT
Created attachment 441906 [details]
Patch
Comment 4 Darin Adler 2021-10-20 14:52:47 PDT
Passing all the EWS tests now so a good time for someone to review. Style feedback welcome.
Comment 5 Anders Carlsson 2021-10-21 08:12:38 PDT
Comment on attachment 441906 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/ColorCocoa.mm:38
> +RetainPtr<UIColor> cocoaColor(const Color& color)

Can/should this assert if the passed in color is invalid?
Comment 6 Darin Adler 2021-10-21 11:08:01 PDT
Comment on attachment 441906 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/ColorCocoa.mm:38
>> +RetainPtr<UIColor> cocoaColor(const Color& color)
> 
> Can/should this assert if the passed in color is invalid?

Currently the behavior is to treat invalid colors as transparent. Removing this feature and instead requiring that no one pass any invalid colors would probably be OK, but I’d want to review that change one call site at a time.

If I was *sure* that no one needed the "treat invalid as transparent" behavior, I would be tempted to just merge the cocoaColorOrNil behavior in to the cocoaColor function and not have two.

This makes me re-discover why I like std::optional better than types like RefPtr, WTF::String, and WTF::Color that have the null value built-in. It’s not easy to see if nulls are expected or an error.
Comment 7 Darin Adler 2021-10-21 11:25:16 PDT
Committed r284630 (243351@main): <https://commits.webkit.org/243351@main>
Comment 8 Radar WebKit Bug Importer 2021-10-21 11:26:17 PDT
<rdar://problem/84516075>