[webkit-reviews] review denied: [Bug 36062] Chromium should not lose activation to select popups : [Attachment 51159] Applied changes suggested by Eric

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jay Campan
<jcampan at google.com>'s request for review:
Bug 36062: Chromium should not lose activation to select popups
https://bugs.webkit.org/show_bug.cgi?id=36062

Attachment 51159: Applied changes suggested by Eric
https://bugs.webkit.org/attachment.cgi?id=51159&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebCore/page/chromium/ChromeClientChromium.h
...
> +    // Notifies the client a popup was closed.
> +    virtual void popupClosed(WebCore::PopupContainer* popupContainer) = 0;

nit: ^^^ no need for WebCore:: prefix when already within the WebCore
namespace.


> Index: WebCore/platform/chromium/PopupMenuChromium.h
...
> +	   // Convenience method that returns the ChromeClient of the frame
view this
> +	   // popup is associated with.
> +	   ChromeClientChromium* chromeClientChromium();

nit:
	   // Returns the ChromeClientChromium of the page this popup is
associated with.


> Index: WebKit/chromium/src/WebViewImpl.cpp
...
> +bool WebViewImpl::selectPopupHandleKeyEvent(const WebKeyboardEvent& event)
> +{
> +    if (!m_selectPopup)
> +	 return false;

nit: ^^^ indentation


It turns out that the rule for indentation within a namespace changed, and
the guidance is to revise the indentation of files as we modify them.  So,
in this case, PopupContainer.h should have its style fixed.

Otherwise, R=me