Bug 231909 - cachedCGColor() and nsColor() should return smart pointers
Summary: cachedCGColor() and nsColor() should return smart pointers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-18 13:24 PDT by Chris Dumez
Modified: 2021-10-19 09:54 PDT (History)
29 users (show)

See Also:


Attachments
Patch (81.50 KB, patch)
2021-10-18 15:23 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (85.09 KB, patch)
2021-10-18 15:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-10-18 13:24:37 PDT
cachedCGColor() and nsColor() should return smart pointers to be truly thread-safe.
Comment 1 Chris Dumez 2021-10-18 15:23:05 PDT
Created attachment 441645 [details]
Patch
Comment 2 Chris Dumez 2021-10-18 15:58:44 PDT
Created attachment 441651 [details]
Patch
Comment 3 EWS 2021-10-19 07:51:31 PDT
Committed r284453 (243213@main): <https://commits.webkit.org/243213@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441651 [details].
Comment 4 Radar WebKit Bug Importer 2021-10-19 07:52:20 PDT
<rdar://problem/84414751>
Comment 5 David Kilzer (:ddkilzer) 2021-10-19 09:12:22 PDT
<rdar://problem/84348887>
Comment 6 Darin Adler 2021-10-19 09:45:42 PDT
Comment on attachment 441651 [details]
Patch

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

Looking at all these call sites makes me think about how to improve further:

It’s not great that we have both platformColor in ColorCocoa.h and nsColor in ColorMac.h. Maybe we can retire nsColor?

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:424
> +    UIColor *uiColor = [PAL::getUIColorClass() colorWithCGColor:cachedCGColor(color).get()];

Seems like this should use platformColor, not cachedCGColor.

> Source/WebCore/rendering/RenderThemeMac.mm:2354
> +        (__bridge id)kCTForegroundColorAttributeName: (__bridge NSColor *)cachedCGColor(titleTextColorForAttachment(attachment, style)).get()

Food for thought: Is this really a cast to NSColor? If so, then I am surprised that we aren’t using the nsColor function instead of the cachedCGColor function; and I will note that the nsColor function does *not* just use cachedCGColor with toll-free bridging to convert to NSColor. So I wonder why here we do that, but elsewhere we don’t.

One possibility is that maybe this is supposed to be a CGColor that just happens to be stored in a dictionary, and thus is not really an NSColor, and toll-free bridging is not relevant. In that case this really is more of an "id" cast, written as if it’s an NSColor * cast.

> Source/WebCore/rendering/RenderThemeMac.mm:2413
> +        (__bridge id)kCTForegroundColorAttributeName: (__bridge NSColor *)cachedCGColor(subtitleColor).get()

Same.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:294
> +    return { adoptNS([[UIColor alloc] initWithCGColor:cachedCGColor(color).get()]) };

Seems like this should use platformColor, not cachedCGColor.

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:547
> +        auto uiBackgroundColor = adoptNS([[UIColor alloc] initWithCGColor:cachedCGColor(newScrollViewBackgroundColor).get()]);

Seems like this should use platformColor, not cachedCGColor.

> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:229
> +    auto preview = createTargetedDragPreview(textIndicatorImage.get(), contentView, previewContainer, indicator.textBoundingRectInRootViewCoordinates, indicator.textRectsInBoundingRectCoordinates, [UIColor colorWithCGColor:cachedCGColor(indicator.estimatedBackgroundColor).get()], nil);

Seems like this should use platformColor, not cachedCGColor.

> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:316
> +        return createTargetedDragPreview(textIndicatorImage.get(), contentView, previewContainer, indicator.textBoundingRectInRootViewCoordinates, indicator.textRectsInBoundingRectCoordinates, [UIColor colorWithCGColor:cachedCGColor(indicator.estimatedBackgroundColor).get()], nil).autorelease();

Seems like this should use platformColor, not cachedCGColor.

> Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm:228
> +            backgroundColor = adoptNS([[UIColor alloc] initWithCGColor:WebCore::cachedCGColor(coreColor).get()]);

Seems like this should use platformColor, not cachedCGColor.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2015
> +    [_highlightView setColor:adoptNS([[UIColor alloc] initWithCGColor:cachedCGColor(_tapHighlightInformation.color).get()]).get()];

Seems like this should use platformColor, not cachedCGColor.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3694
> +            return [UIColor colorWithCGColor:cachedCGColor(caretColor).get()];

Seems like this should use platformColor, not cachedCGColor.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8600
> +        targetedPreview = createTargetedPreview(textIndicatorImage.get(), self, self.containerForContextMenuHintPreviews, indicator.textBoundingRectInRootViewCoordinates, indicator.textRectsInBoundingRectCoordinates, [UIColor colorWithCGColor:cachedCGColor(indicator.estimatedBackgroundColor).get()]);

Seems like this should use platformColor, not cachedCGColor.

> Source/WebKit/UIProcess/ios/forms/WKFormColorControl.mm:82
> +        [colors addObject:[UIColor colorWithCGColor:cachedCGColor(color).get()]];

Seems like this should use platformColor, not cachedCGColor.

> Source/WebKit/UIProcess/ios/forms/WKFormColorControl.mm:90
> +    [_colorPickerViewController setSelectedColor:[UIColor colorWithCGColor:cachedCGColor(_view.focusedElementInformation.colorValue).get()]];

Seems like this should use platformColor, not cachedCGColor.

> Source/WebKitLegacy/mac/WebView/WebView.mm:885
> +        _estimatedBackgroundColor = [PAL::allocUIColorInstance() initWithCGColor:cachedCGColor(indicatorData.estimatedBackgroundColor).get()];

Seems like this should use platformColor, not cachedCGColor.
Comment 7 Darin Adler 2021-10-19 09:46:02 PDT
I’ll tackle these changes I am suggesting in another bug.
Comment 8 Chris Dumez 2021-10-19 09:54:06 PDT
(In reply to Darin Adler from comment #7)
> I’ll tackle these changes I am suggesting in another bug.

Thanks.