[webkit-reviews] review denied: [Bug 53023] WebKit2: Type-to-select doesn't work in open <select> menu : [Attachment 87878] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jon Lee <jonlee at apple.com>'s request
for review:
Bug 53023: WebKit2: Type-to-select doesn't work in open <select> menu
https://bugs.webkit.org/show_bug.cgi?id=53023

Attachment 87878: Patch
https://bugs.webkit.org/attachment.cgi?id=87878&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87878&action=review

review- because this breaks non-Mac builds

> Source/WebKit2/UIProcess/WebPopupMenuProxy.h:62
> +    virtual bool setFocusedIndex(int index, bool hotTracking = false) = 0;

By adding a new pure virtual function, you create the need to implement the
function for all classes derived from it. That means Windows and Qt versions at
least, in addition to the Mac version.

What is “hotTracking” for?

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:135
> +bool WebPopupMenuProxyMac::setFocusedIndex(int index, bool hotTracking)

Normally we omit the names of unused arguments. In WebCore we even have this
set to warn, but it’s off in WebKit2 because it’s so hard to do for Objective-C
methods.

I sugest omitting the argument names here.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:139
> +}
>  } // namespace WebKit

Missing a blank line there.