Bug 207963

Created attachment 391200 [details]
Patch

<rdar://problem/57383170>

Created attachment 391211 [details]
Patch

Created attachment 391212 [details]
Patch

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?

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.

Created attachment 391297 [details]
Patch

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.

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.

Created attachment 391312 [details]
Patch for landing

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?

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.

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.

Created attachment 391336 [details]
Patch for landing

Comment on attachment 391336 [details]
Patch for landing

Clearing flags on attachment: 391336

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

All reviewed patches have been landed.  Closing bug.