This bug tracks the macOS work required to show suggestions for color input fields with a list attribute.
Created attachment 345314 [details] Patch
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
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 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.
Created attachment 345628 [details] Patch
Created attachment 345697 [details] Patch
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?
Created attachment 345711 [details] Patch
(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 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 on attachment 345711 [details] Patch Clearing flags on attachment: 345711 Committed r234172: <https://trac.webkit.org/changeset/234172>
All reviewed patches have been landed. Closing bug.
<rdar://problem/42557159>