[Webkit-unassigned] [Bug 187794] [Datalist][macOS] Display suggestions for input[type=color]

[bugzilla-daemon at webkit.org]

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

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

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1518
> +std::optional<Color> ArgumentCoder<Color>::decode(Decoder& decoder)

I don’t think it’s ever right to have two decode functions for the same type. You should check with e.g. Alex and see what the current direction is here. Might be that you just get rid of the non-optional one? I don’t recall.

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:51
> + at interface NSPopover (AppKitSecretsIKnow)

No need to name the category. Also these should go in the AppKitSPI header, no?

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:174
> ++ (NSPopover *)_colorPopoverCreateIfNecessary:(BOOL)forceCreation {

I think there’s been a lot of debate over this and people have settled on a particular naming scheme for these things. But in general:

- should probably be a static C function.
- should probably not have “create if necessary”. I think the word we use is “ensure” but you should check. Yours is a little odd in that only sometimes do you want to ensure it...
- might want a NeverDestroyed<RetainPtr<>>? But again, not sure there’s a reason for it, just seems to happen.

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:177
> +        NSPopover *popover = [[NSPopover alloc] init];

RetainPtr!

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:181
> +        NSColorPopoverController *controller = [[NSClassFromString(@"NSColorPopoverController") alloc] init];

Why NSClassFromString instead of putting it in an SPI header?

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:226
> +    [_suggestedColors release];

Why not a RetainPtr!

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:258
> +        suggestedColors =  [[[NSColorList alloc] init] autorelease];

extra space before [

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:260
> +            [suggestedColors insertColor:nsColor(suggestions.at(i)) key:[NSString stringWithFormat:@"%zu", i] atIndex:i];

There are much more efficient ways of making an NSString from a number, I believe. I don’t know what is best, but I bet @(i).stringValue is better than this.

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:323
> +- (void)setAndShowPicker:(WebColorPickerMac*)picker withColor:(NSColor *)color andSuggestions:(Vector<WebCore::Color>&&)suggestions

I think you can leave the “and” out of “andSuggestions”, don’t think that’s Cocoa convention.

-- 
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/bd40f5d8/attachment.html>