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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=177489

Wenson Hsieh <wenson_hsieh at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #322744|review+                     |review?
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171005/444c9c5c/attachment.html>