[webkit-reviews] review granted: [Bug 39639] [Chromium] Implement new PopupMenuClient methods for AutoFill. : [Attachment 56959] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted James Hawkins
<jhawkins at chromium.org>'s request for review:
Bug 39639: [Chromium] Implement new PopupMenuClient methods for AutoFill.
https://bugs.webkit.org/show_bug.cgi?id=39639

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebCore/rendering/RenderMenuList.h:104
> +	 virtual void selectionChanged(unsigned, bool) {}
nit: please give these parameters some names.  the type names alone
do not adequately describe their purpose.

WebCore/rendering/RenderTextControlSingleLine.h:111
> +	 virtual void selectionChanged(unsigned, bool) {}
nit: please give these parameters some names.  the type names alone
do not adequately describe their purpose.

WebKit/chromium/src/AutoFillPopupMenuClient.cpp:102
 +	// FIXME: Refactor this method.
nit: please be a bit more descriptive with this comment.  if someone else
is reading this, it'd be great to not have to guess what you had in mind.
i'm not sure what you intend to refactor ;-)

WebKit/chromium/src/SuggestionsPopupMenuClient.h:64
 +	virtual void selectionChanged(unsigned, bool) {}
nit: please give these parameters some names...

otherwise, LGTM.