Bug 24692 – Allow ChromeClientChromium to access an HTML select element's menu items

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Paul Godavari]

Chromium's HTML select element menus will be rendered with native cocoa controls on Mac OS X in the browser process. This change will allow Chromium's ChromeClient to access the menu items in the PopupMenu for marshalling to the browser process or another host (test_shell, for example).

[Paul Godavari]

Created attachment 28743 [details]
Allows ChromeClientChromium to access popup menu items

[Paul Godavari]

Created attachment 28757 [details]
Updated patch to allow platform specific handling of Chromium HTML select popups

Updated patch which removes unnecessary includes and adds the update to ChangeLog.

[Darin Fisher (:fishd, Google)]

Comment on attachment 28757 [details]
Updated patch to allow platform specific handling of Chromium HTML select popups

Windows and Linux implementations are exactly the same, and it is bad to duplicate code.  How about using #ifdefs in PopupMenuChromium.cpp to select between Mac and non-Mac?

also, it seems like it would be nicer to have a single struct that contains the entire 'model' for the PopupMenu.  that would include itemHeight, selectedIndex and menuItems.

[Paul Godavari]

Created attachment 28880 [details]
Updated patch for handling select elements in the chromium port

1. Removed the per platform files and used PLATFORM(DARWIN) for determining whether to call show() or showSelect().

2. I didn't create a struct for the PopupListBox data, since we're only interested in a part of it (item height, selected index and item list), and there remains several other data members. It just seems a little strange to include some members and not the others in the struct. If you feel strongly about it, I'll make that change.

[Darin Fisher (:fishd, Google)]

Comment on attachment 28880 [details]
Updated patch for handling select elements in the chromium port

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

[Paul Godavari]

Created attachment 29099 [details]
Updated patch for handling HTML selects using native controls on the Mac

I believe this patch addresses your previous comments and our discussions.

[Darin Fisher (:fishd, Google)]

Comment on attachment 29099 [details]
Updated patch for handling HTML selects using native controls on the Mac

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

[Paul Godavari]

Created attachment 29242 [details]
Update for handling Mac Chromium popups with native controls

Updated patch as per previous review comments.

[Darin Fisher (:fishd, Google)]

Comment on attachment 29242 [details]
Update for handling Mac Chromium popups with native controls

> Index: WebCore/ChangeLog
...
> +        (WebCore::PopupContainer::showSelectPopup):

ChangeLog was not updated to match the function renaming.

Otherwise, LGTM

[Paul Godavari]

Created attachment 29243 [details]
Same as previous patch, but with updated ChangeLog

Identical to previous patch, but with a properly updated ChangeLog.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/42287.