[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 #28880|review?(fishd at chromium.org) |review-
               Flag|                            |




------- Comment #5 from fishd at chromium.org  2009-03-25 13:24 PDT -------
(From update of attachment 28880)
>Index: WebCore/page/chromium/ChromeClientChromium.h
...
>         // Notifies the client of a new popup widget.  The client should place
>         // and size the widget with the given bounds, relative to the screen.
>         virtual void popupOpened(FramelessScrollView* popupView, const IntRect& bounds, bool focusOnShow) = 0;
>+
>+        // Similar to popupOpened above, but used for clients that will draw the
>+        // popup menu with native controls and thus need a more information.
>+        virtual void popupOpenedWithItems(FramelessScrollView* popupView, const IntRect& bounds,
>+                                          bool focusOnShow, int itemHeight, int selectedIndex,
>+                                          const Vector<PopupListData*>& menuItems) = 0;

train of thought here, sorry...

it occurs to me that in layout-test mode of test_shell we don't currently
have a way to run the tests that probe the drop-down select.  perhaps what
you are doing here would be reusable there.  that's an argument in favor
of not wrapping this new method in an #ifdef :)

perhaps we should also have a static setter on PopupContainer that can be
used to switch between modes?  so instead of an #ifdef, we'd have the Mac
code call that setter at startup.

Just commenting on this API, I'm surprised that you are not also passing
the PopupMenuStyle object.  It seems like we need to still respect some
of the styling options.

popupOpenedWithItems seems like a poor name for this.  what we are really
doing is asking the client to open a popup for us, and we are giving it
all of the data for that.

Maybe the better choice here would be to just pass the PopupContainer as
the parameter to popupOpened.  Then, the popupOpened implementation could
just read the menuItems, itemHeight, and selectedIndex from that.  Then
we would not need a new method on ChromeClientChromium.


>Index: WebCore/platform/chromium/PopupMenuChromium.cpp
...
> PopupContainer::~PopupContainer()
> {
>+#if !PLATFORM(DARWIN)
>     if (m_listBox)
>         removeChild(m_listBox.get());
>+#endif

instead, it seems like we should just check if m_listBox has a parent.

      if (m_listBox && m_listBox->parent())
          removeChild(m_listBox);


>+void PopupContainer::showSelect(const IntRect& rect, FrameView* v, int index)
>+{
>+     if (!listBox())
>+        return;
>+     
>+     listBox()->updateFromElement();
>+     
>+     // Get the ChromeClient and pass it the popup menu's listbox data.
>+     ChromeClientChromium* client = static_cast<ChromeClientChromium*>(v->frame()->page()->chrome()->client());

nit: this line is really long.  can you wrap it like we do in showPopup?


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

maybe the #ifdef should just move into the PopupContainer::show method
since otherwise the show method doesn't make sense on ChromiumMac?


>Index: WebCore/platform/chromium/PopupMenuChromium.h
...
>+    // A container for the data for each menu item (represented by <option> or

nit: please put "e.g., " in front of "represented" since that is only
one such case where this could be used.


>+    // <optgroup> in a <select> widget) and is used by PopupListBox.
>+    typedef struct PopupListData {

please remove the typedef.  it should just be "struct PopupListData {"

also, I think a better name for this is PopupItem.


>+        enum Type {
>+            TypeOption = 0,

nit: no need to initialize to 0 explicitly.


>+            TypeGroup,
>+            TypeSeparator
>+        };
>+
>+        PopupListData(const String& label, Type type)
>+            : label(label.copy()), type(type), y(0) {}

nit: the .copy() call is unnecessary.  just use the copy-constructor.


>+        // Used on Mac Chromium for HTML Select menus.
>+        void showSelect(const IntRect&, FrameView*, int index);

this comment is a little off since this code is used for more than just
HTML select menus.  It is also used for the autocomplete dropdown, and
there is another HTML usage as well.  Can you come up with a better
name for this method?


-- 
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.