Bug 207963 – Expose caret color for clients

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Expose text insertion color for clients

[Megan Gardner]

Created attachment 391200 [details]
Patch

[Megan Gardner]

<rdar://problem/57383170>

[Megan Gardner]

Created attachment 391211 [details]
Patch

[Megan Gardner]

Created attachment 391212 [details]
Patch

[Wenson Hsieh]

Comment on attachment 391212 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:1624
> +    WebCore::Frame *frame = core(self);

Nit - * on the other side (I would also just use auto*)

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:1633
> +    auto& renderer = *focusedElement->renderer();

What guarantees that the renderer exists?

[Daniel Bates]

Comment on attachment 391212 [details]
Patch

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

This patch looks good.

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:1622
> +- (CGColorRef)textInsertionColor

This is ok as-is. A better solution would be to name this caretColor and move this code nearer to -setCaretColor because:

1. This will now be the getter to the setter.
2. Because of (1) this enables use of muscle memory.

Actually as I am writing this I made the assumption that computeCaretColor could return the -setCaretColor. If this is true, ^^^ is true. If not, why not? If not, then I think the name should be underscore prefixed. If not, what would the return value mean in its most descriptive form? Could that be used as a more descriptive name for this function?

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:1625
> +    auto* document = frame->document();

FYI frame can be nil

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:1634
> +    return cachedCGColor(WebCore::CaretBase::computeCaretColor(renderer.style(), renderer.element()));

If computeCaretColor() returns an invalid color then what does cachedCGColor() return? Does it return nil? If so, then the optimal solution is to add a comment to explain this subtlety and do what you do now. If it does not then the optimal solution is to check if it's INVALID and return nil and otherwise do what you do now.

[Megan Gardner]

Created attachment 391297 [details]
Patch

[Daniel Bates]

Comment on attachment 391297 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebFramePrivate.h:146
> +- (CGColorRef)getCaretColor;

This is ok as-is. The optimal solution would be to define @property caretColor because:

1. Can remove -setCaretColor.
2. No need to add -getCaretColor
3. Because of (2) no need to rename -getCaretColor to -caretColor to conform to WebKit Code Style guide.

[Daniel Bates]

Comment on attachment 391297 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:1449
> +    auto color = WebCore::CaretBase::computeCaretColor(renderer->style(), renderer->element());

Is it OK if renderer->element() != focusedElement? If so, why? Contrived example: image maps.

[Megan Gardner]

Created attachment 391312 [details]
Patch for landing

[Simon Fraser (smfr)]

Comment on attachment 391312 [details]
Patch for landing

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

> Source/WebKitLegacy/mac/WebView/WebFramePrivate.h:145
> +@property (nonatomic, readwrite) CGColorRef caretColor;

readwrite? Is it supposed to be settable?

[Megan Gardner]

Comment on attachment 391312 [details]
Patch for landing

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

>> Source/WebKitLegacy/mac/WebView/WebFramePrivate.h:145
>> +@property (nonatomic, readwrite) CGColorRef caretColor;
> 
> readwrite? Is it supposed to be settable?

yep, this replaces the setter above and adds a getter.

[Simon Fraser (smfr)]

Comment on attachment 391312 [details]
Patch for landing

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

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:1435
> +- (CGColorRef)caretColor

This doesn't return a retained color (and there's no autorelease for CF objects), so I'm a bit concerned about the lifetime of the CGColorRef. If WebCore decided to purge it from the cache, the client's color would be deleted.

>> Source/WebKitLegacy/mac/WebView/WebFramePrivate.h:145
>> +@property (nonatomic, readwrite) CGColorRef caretColor;
> 
> readwrite? Is it supposed to be settable?

Oh I see that's old behavior.

[Megan Gardner]

Created attachment 391336 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 391336 [details]
Patch for landing

Clearing flags on attachment: 391336

Committed r257097: <https://trac.webkit.org/changeset/257097>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.