[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
https://bugs.webkit.org/show_bug.cgi?id=186531
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
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.
--
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