Bug 231909 – cachedCGColor() and nsColor() should return smart pointers

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

[Chris Dumez]

cachedCGColor() and nsColor() should return smart pointers to be truly thread-safe.

[Chris Dumez]

Created attachment 441645 [details]
Patch

[Chris Dumez]

Created attachment 441651 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/84414751>

[David Kilzer (:ddkilzer)]

<rdar://problem/84348887>

[Darin Adler]

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.

[Darin Adler]

I’ll tackle these changes I am suggesting in another bug.

[Chris Dumez]

(In reply to Darin Adler from comment #7)
> I’ll tackle these changes I am suggesting in another bug.

Thanks.