[Webkit-unassigned] [Bug 24692] Allow ChromeClientChromium to access an HTML select element's menu items

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24692


fishd at chromium.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29099|review?(fishd at chromium.org) |review-
               Flag|                            |




------- Comment #7 from fishd at chromium.org  2009-04-03 13:01 PDT -------
(From update of attachment 29099)
> 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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.