[Webkit-unassigned] [Bug 186531] [Datalist][macOS] Add suggestions UI for TextFieldInputTypes

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jun 17 11:56:29 PDT 2018


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
                 CC|                            |darin at apple.com

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

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

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/20180617/725b5b12/attachment-0001.html>

More information about the webkit-unassigned mailing list