[Webkit-unassigned] [Bug 187871] [iOS] Add support for input[type=color]

[bugzilla-daemon at webkit.org]

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

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

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

> Source/WebCore/html/ColorInputType.cpp:173
> +#if PLATFORM(MAC)

It feels like this ifdef is more appropriate in the platform-y Chrome code instead of here.

> Source/WebKit/UIProcess/ios/forms/WKFormColorControl.h:33
> +- (instancetype)initWithView:(WKContentView *)view;

Can this just be a UIView? It’s kind of weird to spread knowledge of WKContentView, and I don’t think it’s important in this case.

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:109
> +        [colorButtons addObject:[colorButtonsRow copy]];

This looks like a leak. But it’s not, because you release them in dealloc. But the array was retaining them, so the extra ref was for naught. If you did things the WebKit way this would be

auto row = adoptNS([colorButtonsRow copy]);
[colorButtons addObject:row.get()];

and then nothing in dealloc.

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:145
> +    [_colorButtons release];
> +    [_colorMatrix release];

Why not RetainPtr instead of this?

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:219
> +    const CGFloat *components = CGColorGetComponents(uiColor.CGColor);

Why are you doing all this yourself? I’m sure we have UIColor->Color code elsewhere.

-- 
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/20180723/2bb8986b/attachment.html>