Bug 187794

Summary: [Datalist][macOS] Display suggestions for input[type=color]
Product: WebKit Reporter: Aditya Keerthi <pxlcoder>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, ews-watchlist, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews201 for win-future
none
Patch
none
Patch
none
Patch none

Description Aditya Keerthi 2018-07-18 17:44:05 PDT
This bug tracks the macOS work required to show suggestions for color input fields with a list attribute.
Comment 1 Aditya Keerthi 2018-07-18 18:03:03 PDT
Created attachment 345314 [details]
Patch
Comment 2 EWS Watchlist 2018-07-19 05:45:04 PDT
Comment on attachment 345314 [details]
Patch

Attachment 345314 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8585589

New failing tests:
http/wpt/crypto/unwrap-ec-key-crash.any.worker.html
Comment 3 EWS Watchlist 2018-07-19 05:45:16 PDT
Created attachment 345343 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 4 Tim Horton 2018-07-23 15:18:59 PDT
Comment on attachment 345314 [details]
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
> +@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.
Comment 5 Aditya Keerthi 2018-07-23 17:30:26 PDT
Created attachment 345628 [details]
Patch
Comment 6 Aditya Keerthi 2018-07-24 12:01:19 PDT
Created attachment 345697 [details]
Patch
Comment 7 Tim Horton 2018-07-24 12:56:17 PDT
Comment on attachment 345697 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/mac/NSColorWellSPI.h:34
> +#endif // PLATFORM(MAC)

Is it USE(APPKIT), or PLATFORM(MAC)???

> Source/WebCore/PAL/pal/spi/mac/NSPopoverColorWellSPI.h:52
> +#endif // PLATFORM(MAC)

Ditto

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1520
> +    bool isExtended;

It has got to be possible to implement the other one in terms of this one, no? Just don’t love all the duplication, and also can see myself fixing one and not the other.

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

Brace in the wrong place

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:166
> +- (void)_showPopover {

Brace in the wrong place

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:174
> +    if ([owner isKindOfClass:[NSPopoverColorWell class]])
> +        [owner deactivate];

What is this fragile-looking code for? Comments, maybe?
Comment 8 Aditya Keerthi 2018-07-24 13:57:13 PDT
Created attachment 345711 [details]
Patch
Comment 9 Aditya Keerthi 2018-07-24 13:59:41 PDT
(In reply to Tim Horton from comment #7)
> Comment on attachment 345697 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345697&action=review
> 
> It has got to be possible to implement the other one in terms of this one,
> no? Just don’t love all the duplication, and also can see myself fixing one
> and not the other.

Removed the duplicated code. Also, Alex confirmed that we should keep both decode methods (one returning a bool and one returning an std::optional).
Comment 10 Tim Horton 2018-07-24 14:07:03 PDT
Comment on attachment 345711 [details]
Patch

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1521
> +    if (!decode(decoder, color))

OMG SO MUCH BETTER
Comment 11 WebKit Commit Bot 2018-07-24 14:46:42 PDT
Comment on attachment 345711 [details]
Patch

Clearing flags on attachment: 345711

Committed r234172: <https://trac.webkit.org/changeset/234172>
Comment 12 WebKit Commit Bot 2018-07-24 14:46:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-07-24 14:48:08 PDT
<rdar://problem/42557159>