[webkit-reviews] review denied: [Bug 46016] [chromium] There should be an API for showing an external popup that does not require creation of a WebWidget : [Attachment 68316] Fixing style take 2

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jay Civelli
<jcivelli at chromium.org>'s request for review:
Bug 46016: [chromium] There should be an API for showing an external popup that
does not require creation of a WebWidget
https://bugs.webkit.org/show_bug.cgi?id=46016

Attachment 68316: Fixing style take 2
https://bugs.webkit.org/attachment.cgi?id=68316&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68316&action=review

> WebKit/chromium/public/WebExternalPopupMenu.h:38
> +    virtual void showExternalPopupMenu() = 0;

nit: these method names can use shorter names like "show" and "close"
they are already defined within the context of WebExternalPopupMenu,
so there is no need to re-type that context.

> WebKit/chromium/public/WebExternalPopupMenuClient.h:40
> +    virtual void selectionChanged(int index) = 0;

style-nit:  selectionChanged -> didChangeSelection

> WebKit/chromium/public/WebExternalPopupMenuClient.h:45
> +    virtual void acceptedIndex(int index) = 0;

style-nit:  acceptedIndex -> didAcceptIndex

> WebKit/chromium/public/WebExternalPopupMenuClient.h:51
> +    virtual void canceled() = 0;

style-nit:  canceled -> didCancel

> WebKit/chromium/src/ChromeClientImpl.cpp:853
> +#if defined(EXTERNAL_POPUP)

I don't see where this is defined.  Is this something you want to alter in the
build
system to engage this new API?	An alternative way to accomplish this for
migration
purposes is to call createExternalPopupMenu here, and if that returns non-null,
then
pass the WebExternalPopupMenu instance to the ExternalPopupMenu constructor,
etc.
Otherwise, create a PopupMenuChromium as the old code was doing.