[webkit-reviews] review granted: [Bug 172905] [GTK] Add API to allow overriding popup menus : [Attachment 311960] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 172905: [GTK] Add API to allow overriding popup menus
https://bugs.webkit.org/show_bug.cgi?id=172905

Attachment 311960: Patch

https://bugs.webkit.org/attachment.cgi?id=311960&action=review




--- Comment #8 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 work, as usual.

> Source/WebKit2/UIProcess/API/gtk/WebKitPopupMenu.h:29
> +class WebKitPopupMenu final : public WebPopupMenuProxyGtk {

I'm confused by your decision to make a subclass of WebPopupMenuProxyGtk, and
especially by your decision to not include "Proxy" in the name of the class.
It's quite unclear that this object is a UI proxy for a web process object. Can
you elaborate on why you chose this design?

Would it be better for WebKitPopupMenu to contain a WebPopupMenuProxyGtk member
variable rather than to inherit from it, as I suspect, or would this require
too much extra code and indirection?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:-256
> -    void (*_webkit_reserved3) (void);

Running out of space for more virtual signals... I think we'll be hitting our
limit within the next couple of years. Oh well.