[webkit-reviews] review denied: [Bug 24692] Allow ChromeClientChromium to access an HTML select element's menu items : [Attachment 29099] Updated patch for handling HTML selects using native controls on the Mac

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Paul Godavari
<paul at chromium.org>'s request for review:
Bug 24692: Allow ChromeClientChromium to access an HTML select element's menu
items
https://bugs.webkit.org/show_bug.cgi?id=24692

Attachment 29099: Updated patch for handling HTML selects using native controls
on the Mac
https://bugs.webkit.org/attachment.cgi?id=29099&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebCore/page/chromium/ChromeClientChromium.h

> +	   // If handleExternal is true, then then drawing and input handling
for

nit: "then then"


> Index: WebCore/platform/chromium/PopupMenuChromium.cpp

> +void PopupContainer::showSelectPopup(const IntRect& rect, FrameView* v, int
index)

nit: I think this name could be better.  Afterall, the other "show" method is
also used to render SELECT popups, so this name doesn't really make sense.
How about showExternal?  That matches up with handleExternal being true in
the popupOpened call.


> +void PopupMenu::show(const IntRect& r, FrameView* v, int index)
>  {
>      if (!p.popup)
>	   p.popup = PopupContainer::create(client(), dropDownSettings);
> +#if PLATFORM(DARWIN)
> +    p.popup->showSelectPopup(r, v, index);
> +#else
>      p.popup->show(r, v, index);
> +#endif

nit: Please add a comment here explaining why there is a platform
difference.


> Index: WebCore/platform/chromium/PopupMenuChromium.h
...
> +	   PopupItem(const String& label, Type type)
> +	       : label(label), type(type), y(0) {}

nit: WebKit style is one initializer per line and write "{ }" instead of "{}"


> +	   String label;
> +	   Type type;
> +	   int y;  // y offset of this item, relative to the top of the popup.

Perhaps this variable would be better named "offset"


> -
> +	 

please make sure that blank lines do not have any whitespaces.


OK, looks like this is getting really close.  One more update should do it.