[webkit-reviews] review granted: [Bug 188669] [Datalist][iOS] Display suggestions for input[type=color] : [Attachment 347281] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Aditya Keerthi
<akeerthi at apple.com>'s request for review:
Bug 188669: [Datalist][iOS] Display suggestions for input[type=color]
https://bugs.webkit.org/show_bug.cgi?id=188669

Attachment 347281: Patch

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




--- Comment #2 from Tim Horton <thorton at apple.com> ---
Comment on attachment 347281
  --> https://bugs.webkit.org/attachment.cgi?id=347281
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:48
> +#if ENABLE(DATALIST_ELEMENT)

Newline before the #if usually

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:168
> +    return @[ @[ [UIColor redColor], [UIColor orangeColor], [UIColor
yellowColor], [UIColor greenColor], [UIColor cyanColor], [UIColor blueColor],
[UIColor magentaColor], [UIColor purpleColor], [UIColor brownColor], [UIColor
whiteColor], [UIColor grayColor], [UIColor blackColor] ] ];

Ideally I think these would have all been UIColor.redColor (dots!), but
whatever.

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:202
> +	       WebCore::Color color =
view.assistedNodeInformation.suggestedColors.at(i);

Why at() instead of subscripting? (maybe there's a reason?)

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:203
> +	       [colors addObject: [UIColor
colorWithCGColor:cachedCGColor(color)]];

There's some weird spaces here (after the colon).
Do we not have a UIColor cache? (maybe not)

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:208
> +    } else
> +#endif
> +	   topColorMatrix = [[self class] defaultTopColorMatrix];

I do not love flow-control-crossing-ifdef-boundaries, but maybe it's fine.