Bug 186531 – [Datalist][macOS] Add suggestions UI for TextFieldInputTypes

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

This bug tracks the macOS user-interface work required to show suggestions for TextFieldInputTypes with a list attribute.

[Aditya Keerthi]

Created attachment 342452 [details]
Patch

[Tim Horton]

Comment on attachment 342452 [details]
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

[Wenson Hsieh]

Comment on attachment 342452 [details]
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

[Wenson Hsieh]

Comment on attachment 342452 [details]
Patch

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

>> 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.

Yeah, I think we can do this in two ways:

1. If we want to try and inspect the window hierarchy to find the datalist suggestion window, let's just put that logic in the test runner as Tim suggested instead of as SPI in WKWebView (since the implementation of this wouldn't require any special knowledge internal to WebKit or WKWebView).

2. We can also add a bit of plumbing from WKWebView to WebPageProxy, and ask it for the existence of m_dataListSuggestionsDropdown, so we don't need to do the NSWindow crawl.

[Wenson Hsieh]

Comment on attachment 342452 [details]
Patch

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

>> LayoutTests/fast/forms/datalist/datalist-show-hide.html:25
>> +        shouldBe("showingList", '"true"');
> 
> shouldBeTrue is a thing, I think.

Ah, but runUIScript's callback returns stringified values! Never mind, let's keep this as shouldBe()

[Aditya Keerthi]

Created attachment 342477 [details]
Patch

[Tim Horton]

Comment on attachment 342477 [details]
Patch

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

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:226
> +- (BOOL)acceptsFirstResponder {

{ on the next line

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:387
> +    NSRect windowRect = [[_view window] convertRectToScreen:[_view convertRect:NSMakeRect(rect.x(), rect.y(), rect.width(), rect.height()) toView:nil]];

You should just be able to cast from IntRect->NSRect, no?

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:417
> +    WKDataListSuggestionCell *result = [tableView makeViewWithIdentifier:@"WKDataListSuggestionCell" owner:self];

I think we usually keep cell reuse identifiers statically (see Wenson’s recent watch form controls patches)

[Aditya Keerthi]

Created attachment 342707 [details]
Patch

[Aditya Keerthi]

Created attachment 342714 [details]
Patch

[EWS Watchlist]

Comment on attachment 342714 [details]
Patch

Attachment 342714 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/8173317

New failing tests:
fast/forms/datalist/datalist-show-hide.html
fast/forms/datalist/datalist-textinput-keydown.html

[EWS Watchlist]

Created attachment 342723 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 342714 [details]
Patch

Attachment 342714 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8188352

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 342783 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Aditya Keerthi]

Created attachment 342864 [details]
Patch

[EWS Watchlist]

Comment on attachment 342864 [details]
Patch

Attachment 342864 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8208999

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 342878 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Darin Adler]

Comment on attachment 342864 [details]
Patch

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

Looks like a good start. I have a few comments and questions.

> Source/WebKit/UIProcess/WebDataListSuggestionsDropdown.h:36
> +namespace WebCore {
> +class IntRect;
> +}

This is declared but not used in this header.

> Source/WebKit/UIProcess/WebDataListSuggestionsDropdown.h:40
> +class WebPageProxy;

This is declared but not used in this header.

> Source/WebKit/UIProcess/WebDataListSuggestionsDropdown.h:53
> +    static Ref<WebDataListSuggestionsDropdown> create(Client* client)

Client should be a reference rather than a pointer since this is new code.

> Source/WebKit/UIProcess/WebDataListSuggestionsDropdown.h:65
> +    virtual void show(const WebCore::DataListSuggestionInformation&) { };
> +    virtual void close() { };
> +    virtual void handleKeydownWithIdentifier(const String&) { };
> +
> +protected:
> +    explicit WebDataListSuggestionsDropdown(Client* client) : m_client(client) { };

All these lines have unnecessary semicolons. Also, some of these functions should probably be pure virtual instead of having empty bodies. Also, I suggest having the close function call didCloseSuggestions() and set m_client to nullptr rather than being an empty function.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4731
> +    if (!m_dataListSuggestionsDropdown)
> +        m_dataListSuggestionsDropdown = m_pageClient.createDataListSuggestionsDropdown(this);
> +
> +    m_dataListSuggestionsDropdown->show(info);

Under what circumstance is it OK for m_dataListSuggestionsDropdown to already be non-null here? I think we should be asserting it’s null and maybe returning early and doing nothing in that case. But maybe I am missing something. It seems to me that at least we would need to close the existing one before just calling show on it a second time.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4751
> +    if (m_dataListSuggestionsDropdown)
> +        m_dataListSuggestionsDropdown = nullptr;

The if statement here is not needed. We can null out a pointer without checking if it’s already null.

On the other hand, if this is called and m_dataListSuggestionsDropdown is already null, then we have a problem so I think we should probably assert that it’s not null, or maybe even do an early exit.

> Source/WebKit/UIProcess/WebPageProxy.h:1510
> +    void didSelectOption(String&) override;
> +    void didCloseSuggestions() override;

I think we should use final instead of override here, since it means both override and final.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:26
> +#pragma once

If this is included only from Objective-C, then we should use import on it and not add #pragma once to it.

If this is included from non-Objective-C then I think we need to use OBJC_CLASS rather than @class below.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:30
> +#if ENABLE(DATALIST_ELEMENT)
> +
> +#if USE(APPKIT)

This should be using && rather than nested  #if statements.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:37
> +namespace WebCore {
> +class IntRect;
> +}

This is declared but not used in this header.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:43
> +class WebDataListSuggestionsDropdownMac : public WebDataListSuggestionsDropdown {

I suggest marking this class final.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:45
> +    static Ref<WebDataListSuggestionsDropdownMac> create(WebDataListSuggestionsDropdown::Client*, NSView*);

Should take a reference to the client rather than a pointer. Should have a space after NSView before "*".

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:50
> +    void show(const WebCore::DataListSuggestionInformation&) override;
> +    void close() override;
> +    void handleKeydownWithIdentifier(const String&) override;

These should be private rather than public. And final rather than override.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:59
> +    NSView* m_view;

What guarantees we won’t use this pointer after the view has been deallocated?

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:33
> +#if ENABLE(DATALIST_ELEMENT)
> +
> +#if USE(APPKIT)

Should use && rather than two separate #if here.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:35
> +static const int dropdownRowHeight = 20;

This should be CGFloat.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:37
> +static const unsigned long dropdownMaxSuggestions = 6;

The type "unsigned long" here isn’t right. If you want to combine this with the result of size() then the type can be size_t, otherwise it would be unsigned. We don’t use "long" types in WebKit except in very unusual cases.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:40
> +using namespace WebKit;

We shouldn’t use these. They will interfere with our ability to do our Unified Sources optimization in the future. Instead we can just write WebKit:: in a few places.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:46
> +    RetainPtr<NSTextField> _textField;

What prevents this from creating a reference cycle that causes objects to leak?

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:52
> +- (void)activate:(BOOL)shouldActivate;

This should be named setActive, I think. A function named activate would not have a boolean NO that told it to do something else.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:53
> +- (BOOL)isActive;

Do we need this method? I don’t see any uses of it.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:57
> +    RetainPtr<NSScrollView> _enclosingScrollView;

What prevents this from creating a reference cycle that causes objects to leak?

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:70
> +    RetainPtr<WKDataListSuggestionTable> _table;

What prevents this from creating a reference cycle that causes objects to leak? Maybe we have a guarantee that invalidate will always be called?

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

The formatting here is not correct for a C++ object pointer. We put the * next to the type in cases like that.

What guarantees code won’t dereference this pointer after the dropdown is deleted?

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:73
> +    NSView *_view;

What guarantees code won’t dereference this pointer after the view is deallocated?

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:74
> +    RetainPtr<NSWindow> _enclosingWindow;

What prevents this from creating a reference cycle that causes objects to leak? Maybe we have a guarantee that invalidate will always be called?

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

WebKit formatting does not put a space before the "&".

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:238
> +    if (!(self = [super initWithFrame:NSMakeRect(0, 0, rect.width() - 2, 0)]))

What is this magic number 2? The thickness of some border perhaps?

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:245
> +    RetainPtr<NSTableColumn> column = adoptNS([[NSTableColumn alloc] init]);

Could use auto here to avoid writing NSTableColumn twice:

    auto column = adoptNS([[NSTableColumn alloc] init]);

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:246
> +    [column setWidth:rect.width() - 2];

Same question about the 2; maybe we should put the width into a local or call self.bounds.width here instead of recomputing it?

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:255
> +    RetainPtr<NSShadow> dropShadow = adoptNS([[NSShadow alloc] init]);

Could use auto here to avoid writing NSShadow twice:

    auto dropShadow = adoptNS([[NSShadow alloc] init]);

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:258
> +    [dropShadow setShadowBlurRadius:3];

Seems a little strange to have dropdownShadowHeight be a named constant, but the blur radius be a hard-coded 3.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:262
> +    _activeRow = -1;

Do we really need to use a magic number -1 to represent no row active? In C++ we would use std::optional instead of a magic number. The mixed code that results from this where some places do "== -1" and others do "< 0" are inelegant and this can lead to problems.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:269
> +    [self setFrame:NSMakeRect(0, 0, NSWidth(rect) - dropdownShadowHeight*2, 0)];

WebKit formatting puts spaces around the "*".

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:280
> +    _activeRow = row;

Why doesn’t this method have to do any invalidation? Is that a caller responsibility?

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:287
> +    [_enclosingScrollView flashScrollers];

Is this behavior really wanted any time reload is called? That seems strange.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:316
> +    _suggestions = information.suggestions;

Unfortunate to have to copy the vector of suggestion strings here. Would be slightly nicer to use rvalue reference and move semantics to transfer ownership of the vector instead.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:336
> +    NSInteger selectedRow = [_table currentActiveRow];

Do we have a strong guarantee that currentActiveRow won’t be larger than the current size of _suggestions? If not, this could lead to security bugs; might be better to do range checking here.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:337
> +    return (selectedRow < 0) ? String("") : _suggestions.at(selectedRow);

More efficient to call emptyString() rather than writing String("").

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:342
> +    _suggestions = information.suggestions;

Unfortunate to have to copy the vector of suggestion strings here. Would be slightly nicer to use rvalue reference and move semantics to transfer ownership of the vector instead.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:350
> +    WKDataListSuggestionCell *cell;

Doesn’t seem useful to reuse this local variable for both the old and new cells, so I suggest separate local below.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:353
> +    NSInteger oldSelection = [_table currentActiveRow];
> +    NSInteger newSelection = ((oldSelection + value) % size + size) % size;

Should check that this function does the right thing when the size is 0 or 1; unless we guarantee we won’t use this class in cases like that.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:360
> +        if (value == -1)
> +            newSelection = size - 1;

Seems like an inelegant non-obvious too-clever trick here to rely on oldSelection being -1 and value being +1 to make newSelection be 0. Instead  I suggest having the newSelection math be done separately for the "no old selection" case.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:377
> +    [_table setAction:nil];

I don’t think this is needed.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:391
> +    return NSMakeRect(NSMinX(windowRect) - dropdownShadowHeight, NSMinY(windowRect) - height - dropdownShadowHeight - 2, rect.width() + dropdownShadowHeight*2, height + dropdownShadowHeight);

Spaces around "*" in WebKit coding style. Unclear what the magic "- 2" is for exactly, maybe some border width?

> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:197
> +    // <datalist>
> +    readonly attribute boolean isShowingDatalistSuggestions;

Since HTMLDataListElement spells DataList as two words with the "List" capitalized, I suggest we do the same in our interfaces, even internal testing ones.

[Aditya Keerthi]

Created attachment 343000 [details]
Patch

[Aditya Keerthi]

(In reply to Darin Adler from comment #17)
> Comment on attachment 342864 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342864&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:4731
> > +    if (!m_dataListSuggestionsDropdown)
> > +        m_dataListSuggestionsDropdown = m_pageClient.createDataListSuggestionsDropdown(this);
> > +
> > +    m_dataListSuggestionsDropdown->show(info);
> 
> Under what circumstance is it OK for m_dataListSuggestionsDropdown to
> already be non-null here? I think we should be asserting it’s null and maybe
> returning early and doing nothing in that case. But maybe I am missing
> something. It seems to me that at least we would need to close the existing
> one before just calling show on it a second time.

This method can be called when m_dataListSuggestionsDropdown is non-null. For example, consider the case where the list of suggestions is already showing, the user enters a new character, and the suggestions are updated.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:26
> > +#pragma once
> 
> If this is included only from Objective-C, then we should use import on it
> and not add #pragma once to it.
> 
> If this is included from non-Objective-C then I think we need to use
> OBJC_CLASS rather than @class below.

The header is included from non-Objective-C, I have updated my patch to use OBJC_CLASS.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:59
> > +    NSView* m_view;
> 
> What guarantees we won’t use this pointer after the view has been
> deallocated?
>
> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:73
> > +    NSView *_view;
> 
> What guarantees code won’t dereference this pointer after the view is
> deallocated?

If the view is deallocated, we will not be able to interact with the suggestions in any way, and the pointer will be unused.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:46
> > +    RetainPtr<NSTextField> _textField;
> 
> What prevents this from creating a reference cycle that causes objects to
> leak?

_textField does not maintain a reference to the object that created it.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:53
> > +- (BOOL)isActive;
> 
> Do we need this method? I don’t see any uses of it.

Removed.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:57
> > +    RetainPtr<NSScrollView> _enclosingScrollView;
> 
> What prevents this from creating a reference cycle that causes objects to
> leak?
> 
> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:70
> > +    RetainPtr<WKDataListSuggestionTable> _table;
> 
> What prevents this from creating a reference cycle that causes objects to
> leak? Maybe we have a guarantee that invalidate will always be called?
>
> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:74
> > +    RetainPtr<NSWindow> _enclosingWindow;
> 
> What prevents this from creating a reference cycle that causes objects to
> leak? Maybe we have a guarantee that invalidate will always be called?

Invalidate is called whenever the suggestions are closed.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:71
> > +    WebDataListSuggestionsDropdownMac *_dropdown;
> 
> The formatting here is not correct for a C++ object pointer. We put the *
> next to the type in cases like that.
> 
> What guarantees code won’t dereference this pointer after the dropdown is
> deleted?

The dropdown is only deallocated once the suggestions are closed. Consequently, the invalidate method is called, deallocating the objects that could have dereferenced this pointer.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:238
> > +    if (!(self = [super initWithFrame:NSMakeRect(0, 0, rect.width() - 2, 0)]))
> 
> What is this magic number 2? The thickness of some border perhaps?

Removed the magic number as it was found to be unnecessary.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:262
> > +    _activeRow = -1;
> 
> Do we really need to use a magic number -1 to represent no row active? In
> C++ we would use std::optional instead of a magic number. The mixed code
> that results from this where some places do "== -1" and others do "< 0" are
> inelegant and this can lead to problems.

Changed implementation to use std::optional.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:280
> > +    _activeRow = row;
> 
> Why doesn’t this method have to do any invalidation? Is that a caller
> responsibility?

I moved some of the logic from the caller into this method.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:287
> > +    [_enclosingScrollView flashScrollers];
> 
> Is this behavior really wanted any time reload is called? That seems strange.

Modified so that scrollers are only flashed the first time.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:316
> > +    _suggestions = information.suggestions;
> 
> Unfortunate to have to copy the vector of suggestion strings here. Would be
> slightly nicer to use rvalue reference and move semantics to transfer
> ownership of the vector instead.

Updated to use move semantics.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:336
> > +    NSInteger selectedRow = [_table currentActiveRow];
> 
> Do we have a strong guarantee that currentActiveRow won’t be larger than the
> current size of _suggestions? If not, this could lead to security bugs;
> might be better to do range checking here.

We have a semantic guarantee based on the current use of the variable. However, I have included the range check for safety.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:391
> > +    return NSMakeRect(NSMinX(windowRect) - dropdownShadowHeight, NSMinY(windowRect) - height - dropdownShadowHeight - 2, rect.width() + dropdownShadowHeight*2, height + dropdownShadowHeight);
> 
> Spaces around "*" in WebKit coding style. Unclear what the magic "- 2" is
> for exactly, maybe some border width?

I put this value in a constant, a margin for the suggestions view.

[EWS Watchlist]

Comment on attachment 343000 [details]
Patch

Attachment 343000 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/8241944

New failing tests:
imported/w3c/web-platform-tests/FileAPI/blob/Blob-constructor.html

[EWS Watchlist]

Created attachment 343019 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Tim Horton]

Comment on attachment 343000 [details]
Patch

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

> Source/WebKit/UIProcess/WebDataListSuggestionsDropdown.h:51
> +    virtual void close() { m_client->didCloseSuggestions(); m_client = nullptr; }

WebKit style generally doesn’t allow two statements on one line like this. Split this into multiple lines at least (and possibly also move it to the implementation? Why is it here?).

> Source/WebKit/UIProcess/WebPageProxy.cpp:7492
> +void WebPageProxy::closeOverlayedViews()

INTERESTING. I wonder if we should merge this with the dismissContentRelativeChildWindows mechanism that dismisses e.g. find highlights and force-touch previews...

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:50
> +- (void)setText:(NSString *)text;
> +- (void)setActive:(BOOL)shouldActivate;

I realize you don’t need getters now, but is there any reason these aren’t properties?

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:148
> +@implementation WKDataListSuggestionCell

Is there a reason this isn’t just NSTextFieldCell (or a subclass), instead of being our own implementation of that? Seems like that would get rid of a lot of code.

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:158
> +    [_textField setBackgroundColor:[NSColor clearColor]];

Does this do the right thing in Dark Mode? (I’m not actually sure what the right thing is in this case)

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:161
> +    [self setIdentifier:@"WKDataListSuggestionCell"];

I thought you had a constant for this!

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:163
> +    [self addTrackingRect:NSMakeRect(0, 0, NSWidth(frameRect), NSHeight(frameRect)) owner:self userData:nil assumeInside:NO];

Isn’t this rect just self.bounds?

[Aditya Keerthi]

Created attachment 345100 [details]
Patch

[Aditya Keerthi]

(In reply to Tim Horton from comment #22)
> Comment on attachment 343000 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343000&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:7492
> > +void WebPageProxy::closeOverlayedViews()
> 
> INTERESTING. I wonder if we should merge this with the
> dismissContentRelativeChildWindows mechanism that dismisses e.g. find
> highlights and force-touch previews...

Maybe I can look into that in another patch? This patch is fairly large as-is.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:50
> > +- (void)setText:(NSString *)text;
> > +- (void)setActive:(BOOL)shouldActivate;
> 
> I realize you don’t need getters now, but is there any reason these aren’t
> properties?

I didn't make a text property its value is never stored and setText serves only to update the NSTextField inside the view.

Replaced setActive with a property.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:148
> > +@implementation WKDataListSuggestionCell
> 
> Is there a reason this isn’t just NSTextFieldCell (or a subclass), instead
> of being our own implementation of that? Seems like that would get rid of a
> lot of code.

I initially had this as an NSTextField subclass. However, NSTextField does not vertically center it's text. For this reason, WKDataListSuggestionCell is a view that contains a vertically centered NSTextField.

> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:158
> > +    [_textField setBackgroundColor:[NSColor clearColor]];
> 
> Does this do the right thing in Dark Mode? (I’m not actually sure what the
> right thing is in this case)

This does the right thing as the WKDataListSuggestionCell uses semantic colors for its background. Note that the NSTextField does not take up the entire frame of the WKDataListSuggestionCell due to the vertical centering issue discussed above.

[Tim Horton]

(In reply to Aditya Keerthi from comment #24)
> (In reply to Tim Horton from comment #22)
> > Comment on attachment 343000 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=343000&action=review
> > 
> > > Source/WebKit/UIProcess/WebPageProxy.cpp:7492
> > > +void WebPageProxy::closeOverlayedViews()
> > 
> > INTERESTING. I wonder if we should merge this with the
> > dismissContentRelativeChildWindows mechanism that dismisses e.g. find
> > highlights and force-touch previews...
> 
> Maybe I can look into that in another patch? This patch is fairly large
> as-is.

At /least/ leave a FIXME, since it seems clear these should be merged.

> > > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:50
> > > +- (void)setText:(NSString *)text;
> > > +- (void)setActive:(BOOL)shouldActivate;
> > 
> > I realize you don’t need getters now, but is there any reason these aren’t
> > properties?
> 
> I didn't make a text property its value is never stored and setText serves
> only to update the NSTextField inside the view.
> 
> Replaced setActive with a property.
> 
> > > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:148
> > > +@implementation WKDataListSuggestionCell
> > 
> > Is there a reason this isn’t just NSTextFieldCell (or a subclass), instead
> > of being our own implementation of that? Seems like that would get rid of a
> > lot of code.
> 
> I initially had this as an NSTextField subclass. However, NSTextField does
> not vertically center it's text. For this reason, WKDataListSuggestionCell
> is a view that contains a vertically centered NSTextField.
> 
> > > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:158
> > > +    [_textField setBackgroundColor:[NSColor clearColor]];
> > 
> > Does this do the right thing in Dark Mode? (I’m not actually sure what the
> > right thing is in this case)
> 
> This does the right thing as the WKDataListSuggestionCell uses semantic
> colors for its background. Note that the NSTextField does not take up the
> entire frame of the WKDataListSuggestionCell due to the vertical centering
> issue discussed above.

[Aditya Keerthi]

Created attachment 345106 [details]
Patch

[Aditya Keerthi]

(In reply to Tim Horton from comment #25)
> (In reply to Aditya Keerthi from comment #24)
> > (In reply to Tim Horton from comment #22)
> > > Comment on attachment 343000 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=343000&action=review
> > > 
> > > > Source/WebKit/UIProcess/WebPageProxy.cpp:7492
> > > > +void WebPageProxy::closeOverlayedViews()
> > > 
> > > INTERESTING. I wonder if we should merge this with the
> > > dismissContentRelativeChildWindows mechanism that dismisses e.g. find
> > > highlights and force-touch previews...
> > 
> > Maybe I can look into that in another patch? This patch is fairly large
> > as-is.
> 
> At /least/ leave a FIXME, since it seems clear these should be merged.

Added FIXME.

[Tim Horton]

Comment on attachment 345106 [details]
Patch

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

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:53
> +    NSView *m_view;

What's the lifetime of m_view vs. WebDataListSuggestionsDropdownMac? Is it OK that this is a raw pointer?

[Aditya Keerthi]

(In reply to Tim Horton from comment #28)
> Comment on attachment 345106 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345106&action=review
> 
> > Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.h:53
> > +    NSView *m_view;
> 
> What's the lifetime of m_view vs. WebDataListSuggestionsDropdownMac? Is it
> OK that this is a raw pointer?

The suggestions view is destroyed each time it is hidden, so m_view (the WKWebView) should outlast WebDataListSuggestionsDropdownMac in most instances. If for some reason the WKWebView is destroyed while the suggestions view is still showing, WebPageProxy::close() will destroy the instance of WebDataListSuggestionsDropdownMac.

[WebKit Commit Bot]

Comment on attachment 345106 [details]
Patch

Clearing flags on attachment: 345106

Committed r233866: <https://trac.webkit.org/changeset/233866>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42255930>