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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 23 10:27:52 PDT 2018


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

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

> Source/WebCore/html/ColorInputType.cpp:173

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>

More information about the webkit-unassigned mailing list