[Webkit-unassigned] [Bug 172905] [GTK] Add API to allow overriding popup menus

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 14 09:42:12 PDT 2017


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

--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 311960 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311960&action=review
> 
> Nice patch! Here is the first half of your review. I still need to review
> the second half of the patch.
> 
> I don't think I like the names you chose here. The terminology used in GTK+
> is combo box. The terminology used in HTML is select element, though I
> suppose that refers to the entire element and not just the menu. You could
> have named it, for example, WebKitSelectMenu with
> WebKitWebView::show-select-menu, but you've chosen WebKitOptionMenu, which
> matches neither GTK+ nor HTML. What are your thoughts on this? Perhaps it
> needs to be discussed on the mailing list.

Yes, this is the dropdown menu part of the combobox. I used option menu on purpose, as a more generic name, just in case we ever implement datalist and maybe we could reuse this same class. But I'm open to any change in the names in any case.

> > Source/WebKit2/ChangeLog:12
> > +        contains WebKitOptionMenuItem elements representng the items to be displayed.
> 
> representing
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenu.h:29
> > +#include <webkit2/WebKitOptionMenuItem.h>
> > +#include <webkit2/WebKitDefines.h>
> 
> Alphabetize
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:27
> > +G_DEFINE_BOXED_TYPE(WebKitOptionMenuItem, webkit_option_menu_item, webkit_option_menu_item_copy, webkit_option_menu_item_free)
> 
> You're missing all the class documentation for WebKitOptionMenuItem. I think
> that's important, even if it doesn't need to be a lot for such a simple
> class.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:102
> > + * Whether a#WebKitOptionMenuItem is a group label.
> 
> Missing space
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:119
> > + * Whether a#WebKitOptionMenuItem is a group child.
> 
> Missing space
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:136
> > + * Whether a#WebKitOptionMenuItem is enabled.
> 
> Missing space
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:153
> > + * Whether a#WebKitOptionMenuItem is the currently selected one.
> 
> Missing space
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItemPrivate.h:32
> > +        , isGroupChild(item.m_text.startsWith("    "))
> 
> This seems really fragile. I know this is probably hard, but please find a
> way to determine if it's a group child that doesn't depend on the amount of
> whitespace in a user-visible string.

I don't know a batter way. We have unit tests checking this, anyway, so a change in this would be caught by the tests.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170614/b32fddee/attachment-0001.html>


More information about the webkit-unassigned mailing list