[Webkit-unassigned] [Bug 186151] [Datalist] Allow TextFieldInputType to show and hide suggestions

[bugzilla-daemon at webkit.org]

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

--- Comment #3 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 341690
  --> https://bugs.webkit.org/attachment.cgi?id=341690
Patch

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

> Source/WebCore/html/DataListSuggestionInformation.h:31
> +enum DataListSuggestionActivationType {

Nit - we generally prefer to use `enum class` for these, instead of just plain enums.

> Source/WebCore/html/TextFieldInputType.cpp:168
> +void TextFieldInputType::handleClickEvent(__unused MouseEvent& event)

Nit - instead of __unused, we prefer omitting the argument name (in this case, just MouseEvent&).

> Source/WebCore/html/TextFieldInputType.cpp:544
> +#if ENABLE(DATALIST_ELEMENT)

Hm...I find this change a bit odd. It seems like whether we respect the list attribute should depend on whether we support rendering data list UI, rather than always returning true iff the compile-time flag is enabled.

Could we tweak the implementation of RenderTheme::supportsDataListUI() instead? I think it would make the code a bit easier to follow, though it would entail checking against a palette of input type names. But this doesn't seem so costly, especially since AtomicString comparisons are cheap.

> Source/WebCore/html/TextFieldInputType.cpp:768
> +        for (unsigned i = 0; RefPtr<HTMLOptionElement> option = downcast<HTMLOptionElement>(options->item(i)); ++i) {

Nit - We generally prefer to use `auto` for types that can be easily deduced from the RHS. For instance, this could be `auto option = downcast<HTMLOptionElement>(options->item(i))`.

> Source/WebCore/html/TextFieldInputType.h:57
> +    void handleClickEvent(MouseEvent&) override;

Nit - if subclasses don't need to override this, we generally prefer `final` instead of `override`.

> Source/WebCore/html/TextFieldInputType.h:127
> +    IntRect elementRectRelativeToRootView() const final;

Nit - It's great that you specified the coordinate space in the function name! But in most of the places where we have a function like this, it's usually (…)InRootViewCoordinates().

> Source/WebCore/platform/DataListSuggestionPicker.h:42
> +    virtual void handleKeydownWithIdentifier(const WTF::String&) { }

Nit - we probably don't need an explicit WTF:: here.

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