[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 #3 from Wenson Hsieh <wenson_hsieh 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/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:36
> +const CGFloat DropdownShadowSize = 5;

Nit - I would call this DropdownShadowHeight (since it's just the height of the CGSize, not the size itself)

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:70
> +    WebKit::WebDataListSuggestionsDropdownMac *_dropdown;

Nit - I don't think we need explicit WebKit:: namespacing, since we already have `using namespace WebKit` above. Also, the * is usually on the LHS for C++ pointers (as opposed to Objective-C ids)

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:76
> +- (id)initWithInformation:(const WebCore::DataListSuggestionInformation &)information inView:(NSView *)view;

Nit - WebCore::DataListSuggestionInformation& (no space)

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:77
> +- (void)showSuggestionsDropdown:(WebKit::WebDataListSuggestionsDropdownMac *)dropdown;

Ditto.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:78
> +- (void)updateWithInformation:(const WebCore::DataListSuggestionInformation &)information;

Ditto.

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

Nit - the space is usually on the RHS for Objective-C ids.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:227
> +- (void)dealloc

Nit - If we change _textField to a RetainPtr, we won't need to override -dealloc and -release it here.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:237
> +- (id)initWithElementRect:(const WebCore::IntRect &)rect

Nit - const WebCore::IntRect& (no space)

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:242
> +    [self setIntercellSpacing:CGSizeMake(0, 0)];

Nit - CGSizeZero

>> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:250
>> +    _enclosingScrollView = [[NSScrollView alloc] init];
> 
> I think you forgot your adoptNS? _enclosingScrollView should be a RetainPtr

Nit: this looks correct since you -release below, in -dealloc, but I think we generally prefer to just use RetainPtr instead (so we just need to adoptNS() here, and it will be automatically released when the RetainPtr goes away in -dealloc).

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:427
> +    if (result == nil)

Nit - we prefer `!result` instead (rather than checking against nil).

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:428
> +        result = [[WKDataListSuggestionCellMac alloc] initWithFrame:NSMakeRect(0, 0, tableView.frame.size.width, DropdownRowHeight)];

Let's double check to make sure this is expected to return a +1 object.

> LayoutTests/fast/forms/datalist/datalist-show-hide.html:25
> +        shouldBe("showingList", '"true"');

shouldBeTrue is a thing, I think.

> LayoutTests/fast/forms/datalist/datalist-show-hide.html:31
> +            shouldBe("showingList", '"false"');

Also, shouldBeFalse

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