[webkit-reviews] review granted: [Bug 186151] [Datalist] Allow TextFieldInputType to show and hide suggestions : [Attachment 341977] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 7 10:14:54 PDT 2018


Wenson Hsieh <wenson_hsieh at apple.com> has granted Aditya Keerthi
<akeerthi at apple.com>'s request for review:
Bug 186151: [Datalist] Allow TextFieldInputType to show and hide suggestions
https://bugs.webkit.org/show_bug.cgi?id=186151

Attachment 341977: Patch

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




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

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

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

Ah, I think in general we prefer auto* instead of auto for pointers like this.

> Source/WebCore/html/TextFieldInputType.cpp:822
> +    m_suggestionPicker = nullptr;

This seems to be the only place we null out m_suggestionPicker, and it looks
like the plan is for this to be invoked after an IPC message from the UI
process? Should we be clearing this out from more places, e.g. when the element
loses focus? Though, it looks like the next patch in the sequence will answer
my questions :)

Just keep this in mind!

> Source/WebKit/WebProcess/WebCoreSupport/WebDataListSuggestionPicker.cpp:42
> +    , m_page(page) { }

Nit - in most constructors like this, we'd put the braces on new lines like:

{
}

> Source/WebKit/WebProcess/WebCoreSupport/WebDataListSuggestionPicker.h:47
> +    void didSelectOption(const WTF::String&);

Nit - I don't think the WTF:: in WTF::String should be needed here.

> Source/WebKit/WebProcess/WebCoreSupport/WebDataListSuggestionPicker.h:52
> +    __unused WebCore::DataListSuggestionsClient*
m_dataListSuggestionsClient;

I assume these __unused are going to be short-lived? (i.e. the next patch will
use m_page and m_dataListSuggestionsClient).

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3566
> +    m_activeDataListSuggestionPicker = dataListSuggestionPicker;

Also here — make sure we clean up m_activeDataListSuggestionPicker in the
various ways that might cause the picker to disappear! (e.g. element is blurred
programmatically).


More information about the webkit-reviews mailing list