Bug 177489 – [iOS] Respect the "caret-color" CSS property when editing

• Component: CSS
• Status: RESOLVED FIXED

[Aishwarya Nirmal]

<rdar://problem/34600419>

[Aishwarya Nirmal]

Created attachment 322744 [details]
Patch

[Andy Estes]

Comment on attachment 322744 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        This change adds support for the caret-color property on iOS. 

This line should contain the title of the bug ("[iOS] Respect the "caret-color" CSS property when editing").

You can put a description of your change below the "Reviewed by" line.

[Wenson Hsieh]

Comment on attachment 322744 [details]
Patch

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

Looks good overall! Just some minor nits here and there.

> Source/WebKit/ChangeLog:3
> +        This change adds support for the caret-color property on iOS. 

This should be the title of the bug ([iOS] Respect the "caret-color" CSS property when editing)

> Source/WebKit/Shared/EditorState.h:105
> +        WebCore::Color caretColor { WebCore::Color::black };

Hm...black seems like a strange default value to have here. This should probably be an invalid Color, so that we'll use the default [UIColor insertionPointColor]. Perhaps the default Color constructor fulfills this already, so we might not need an explicit initial value here!

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2028
> +    WebCore::Color caretColor = _page->editorState().postLayoutData().caretColor;

I don't think PostLayoutData is guaranteed here -- we should probably add an early return with the default insertionPointColor if the EditorState is missing post layout data (there's a flag on EditorState for this).

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2030
> +        return [[UIColor alloc] initWithCGColor:cachedCGColor(caretColor)];

We should return an autoreleased object UIColor here to avoid leaking.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:234
> +            Color caretColor = m_assistedNode->renderer()->style().caretColor();

Nit - I don't think the local variable for caretColor adds much -- `postLayoutData.caretColor = m_assistedNode->renderer()->style().caretColor();` would be a bit cleaner.

> Tools/ChangeLog:3
> +        Adds test for iOS caret color support.

Title of the bug :)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:311
> +    CGFloat ipcRed = 0, ipcGreen = 0, ipcBlue = 0, ipcAlpha = 0; // insertion point color components

We generally use non-abbreviated variable names in WebKit (so this would be insertionPointColorRed, etc.).. There's more info about it here: https://webkit.org/code-style-guidelines

[Wenson Hsieh]

Comment on attachment 322744 [details]
Patch

Whoops, my comment's collided with Andy's :P

Restoring Andy's r+, but adding cq- for some of the above comments.

[Wenson Hsieh]

Comment on attachment 322744 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:304
> +    TestWKWebView *webView = [[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)];

Looks like TestWKWebView leaks here. We can fix this in a few ways -- one is to just call -release at the end of this test, and another is to autorelease here right after initializing webView. But in WebKit, we often use RetainPtr and adoptNS instead to tie -retain/-release calls to the lifetime of C++ objects (RetainPtrs) so we don't have to explicitly call -retain or -release, so this line could look like this:

    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);

...and then RetainPtr automatically takes care of releasing when it gets destroyed. The `auto` keyword here expands to the return type of adoptNS, which in this case is RetainPtr<TestWKWebView>.

[Aishwarya Nirmal]

Created attachment 322878 [details]
Patch

[Tim Horton]

Comment on attachment 322878 [details]
Patch

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

> Source/WebKit/Shared/EditorState.h:105
> +        WebCore::Color caretColor { WebCore::Color::Color() };

You don't need the braces or anything inside, the default constructor will be invoked regardless.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2031
> +            return [[[UIColor alloc] initWithCGColor:cachedCGColor(caretColor)] autorelease];

UIColor has colorWithCGColor if you want to avoid the autorelease.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:314
> +    [redColor getRed:&redColorRed green:&redColorGreen blue:&redColorBlue alpha:&redColorAlpha];

I wonder, can we not just use UIColor equality instead? Like EXPECT_TRUE([insertionPointColor isEqual:redColor]);?

You're already going to fail if they're the same color in different color spaces, which is the usual gotcha with isEqual, so that's not a reason not to.

Thoughts?

[Aishwarya Nirmal]

Created attachment 323040 [details]
Patch

[Aishwarya Nirmal]

Created attachment 323044 [details]
Patch

[Tim Horton]

Comment on attachment 323044 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:310
> +    CGColorSpaceRef colorSpace = CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB);

This leaks.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:311
> +    CGColorRef cgInsertionPointColor = CGColorCreateCopyByMatchingToColorSpace(colorSpace, kCGRenderingIntentDefault, insertionPointColor.CGColor, NULL);

This leaks.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:312
> +    CGColorRef cgRedColor = CGColorCreateCopyByMatchingToColorSpace(colorSpace, kCGRenderingIntentDefault, redColor.CGColor, NULL);

This leaks.

[Aishwarya Nirmal]

Created attachment 323063 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323063 [details]
Patch

Clearing flags on attachment: 323063

Committed r223012: <http://trac.webkit.org/changeset/223012>

[Sam Sneddon [:gsnedders]]

Not sure why this didn't get closed when the patch was committed?