[webkit-reviews] review granted: [Bug 201768] Datalist option's label not used : [Attachment 395090] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 201768: Datalist option's label not used
https://bugs.webkit.org/show_bug.cgi?id=201768

Attachment 395090: Patch

https://bugs.webkit.org/attachment.cgi?id=395090&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 395090
  --> https://bugs.webkit.org/attachment.cgi?id=395090
Patch

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

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:41
> +static const CGFloat dropdownRowHeightWithoutLabel = 20;
> +static const CGFloat dropdownRowHeightWithLabel = 40;
> +static const size_t maximumTotalHeightForDropdownCells = 120;

As we update code to be more modern, we could use "constexpr" instead of
"static const".

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:189
> +    [_bottomDivider setWantsLayer:YES];
> +    [_bottomDivider setHidden:YES];
> +    [_bottomDivider layer].backgroundColor = NSColor.separatorColor.CGColor;

If we are going to use some "." syntax then I suggest using it even more:

    _bottomDivider.wantsLayer = YES;
    _bottomDivider.hidden = YES;
    _bottomDivider.layer.backgroundColor = NSColor.separatorColor.CGColor;

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:231
> +    [_valueField setStringValue:value];
> +    [_labelField setStringValue:label];
> +    [_labelField setHidden:!label.length];

Same thought here. If we start using "." syntax in some code it would be good
to go all in:

    _valueField.stringValue = value;
    _labelField.stringValue = label;
    _labelField.hidden = !label.length;

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:254
> +    [_valueField setTextColor:backgroundStyle == NSBackgroundStyleLight ?
NSColor.textColor : NSColor.alternateSelectedControlTextColor];
> +    [_labelField setTextColor:NSColor.secondaryLabelColor];

And here we are actively switching some things *away* from "." syntax, but
other things *to* "." syntax in the same line of code. Seems peculiar.