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).
Created attachment 28743 [details] Allows ChromeClientChromium to access popup menu items
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.
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.
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.
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?
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.
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.
Created attachment 29242 [details] Update for handling Mac Chromium popups with native controls Updated patch as per previous review comments.
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
Created attachment 29243 [details] Same as previous patch, but with updated ChangeLog Identical to previous patch, but with a properly updated ChangeLog.
Landed as http://trac.webkit.org/changeset/42287.