[webkit-reviews] review requested: [Bug 177489] [iOS] Respect the "caret-color" CSS property when editing : [Attachment 322744] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 4 17:24:38 PDT 2017


Wenson Hsieh <wenson_hsieh at apple.com> has asked  for review:
Bug 177489: [iOS] Respect the "caret-color" CSS property when editing
https://bugs.webkit.org/show_bug.cgi?id=177489

Attachment 322744: Patch

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




--- Comment #3 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 322744
  --> https://bugs.webkit.org/attachment.cgi?id=322744
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


More information about the webkit-reviews mailing list