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

[bugzilla-daemon at webkit.org]

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

--- Comment #6 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 311960
  --> https://bugs.webkit.org/attachment.cgi?id=311960
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.

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

-- 
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/e7014553/attachment-0001.html>