[Webkit-unassigned] [Bug 186531] [Datalist][macOS] Add suggestions UI for TextFieldInputTypes

[bugzilla-daemon at webkit.org]

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

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

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1642
> +    decoder.decode(suggestions);

These should all check the return value and return false if it’s false.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1647
> +    info = { activationType, suggestions, elementRect };

Why not decode directly into `info`?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6432
> +    for (NSWindow *childWindow in [[self window] childWindows]) {

More dots! self.window.childWindows

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6433
> +        if ([childWindow isKindOfClass:NSClassFromString(@"WKDataListSuggestionWindow")])

This whole thing is not great. If this is really how you want to implement it, maybe hide its ugliness away in the test runners? Maybe Wenson has some thoughts.

> Source/WebKit/UIProcess/WebPageProxy.cpp:2068
> +    endDataListSuggestions();

This seems like a grand time to put hideValidationMessage, endDataListSuggestions, and endColorPicker in a function that all these clients call.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:40
> +class WebDataListSuggestionsDropdownMac;

We can have the Mac in the filenames to make things clear/avoid overlap, but do we need it in the class names? We don’t usually do that.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:94
> +WebDataListSuggestionsDropdownMac::WebDataListSuggestionsDropdownMac(WebDataListSuggestionsDropdown::Client* client, NSView* view)

NSView star’s on the wrong side

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:95
> +    : WebDataListSuggestionsDropdown(client), m_view(view)

two lines

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:202
> +        [[NSColor colorWithRed:238.0f / 255.0f green:238.0f / 255.0f blue:238.0f / 255.0f alpha:1.0f] setFill];
> +        [_textField setTextColor:[NSColor blackColor]];
> +    } else {
> +        [[NSColor whiteColor] setFill];
> +        [_textField setTextColor:[NSColor blackColor]];

Probably these need to all be semantic colors

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:246
> +    NSTableColumn *column = [[[NSTableColumn alloc] init] autorelease];

Even for locals we tend to prefer RetainPtr over autorelease

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:250
> +    _enclosingScrollView = [[NSScrollView alloc] init];

I think you forgot your adoptNS? _enclosingScrollView should be a RetainPtr

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:309
> +    [_enclosingScrollView release];

You won’t need this dealloc once _enclosingScrollView is a RetainPtr.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:326
> +    _enclosingWindow = [[WKDataListSuggestionWindow alloc] initWithContentRect:NSZeroRect styleMask:NSWindowStyleMaskBorderless backing:NSBackingStoreBuffered defer:NO];

Moar RetainPtr

-- 
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/20180611/8ed59a0e/attachment-0001.html>