[webkit-reviews] review denied: [Bug 73304] Opening two popup menus by dispatchEvent() makes problems : [Attachment 117841] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Jing Zhao <jingzhao at chromium.org>'s
request for review:
Bug 73304: Opening two popup menus by dispatchEvent() makes problems
https://bugs.webkit.org/show_bug.cgi?id=73304

Attachment 117841: Patch
https://bugs.webkit.org/attachment.cgi?id=117841&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=117841&action=review


> Source/WebCore/ChangeLog:3
> +	   [Chromium] Fix assertion fails when opening two popup menus.

Please update this line.  This is not a Chromium-specific issue now.

> Source/WebCore/page/ChromeClient.h:305
> +	   virtual bool popupIsVisible() = 0;

Please add a comment so that developers for non-Chromium ports can know what
this is.

> Source/WebKit2/ChangeLog:14
> +	   By using element.dispatchEvent(), a user written script can open two

> +	   popup menus, which causes the assertion in
WebViewImpl::popupOpened()
> +	   fail.
> +
> +	   Add a popupIsVisible() method in ChromeClientImpl and a wrapper in
> +	   Chrome. In RenderMenuList::showPopup(), check if there is an opened
> +	   popup menu before opening a new popup menu.

You don't need to copy the content of WebCore/ChangeLog.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:659
> +    return false;

You should add notImplemented().

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:549
> +    return false;

ditto.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:857
> +    return false;

ditto.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:717
> +    return false;

ditto.