[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