Hide popups when handling mouse down, mouse wheel, or gesture tap in a WebFrameWidget

third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Index: third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp b/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
index fa53cf4be94d6f5fdaa592e667ebd34d4fd8f577..15da3f1e668340bdf0cfc23618e94af127f3a143 100644
--- a/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
+++ b/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
@@ -71,6 +71,7 @@
 #include "web/WebInputEventConversion.h"
 #include "web/WebInputMethodControllerImpl.h"
 #include "web/WebLocalFrameImpl.h"
+#include "web/WebPagePopupImpl.h"
 #include "web/WebPluginContainerImpl.h"
 #include "web/WebRemoteFrameImpl.h"
 #include "web/WebViewFrameWidget.h"
@@ -768,6 +769,14 @@ void WebFrameWidgetImpl::handleMouseLeave(LocalFrame& mainFrame,
 
 void WebFrameWidgetImpl::handleMouseDown(LocalFrame& mainFrame,
                                          const WebMouseEvent& event) {
+  RefPtr<WebPagePopupImpl> pagePopup;
+  if (event.button == WebMouseEvent::Button::Left) {
+    // The mouse down has been outside of any open popups so we should cancel
+    // the current page popup for this process.

[dcheng, 2017/04/07 06:32:01]

Nit: I prefer the original comment (since it also describes why we save
pagePopup)

[EhsanK, 2017/04/07 16:16:12]

Acknowledged.

+    pagePopup = static_cast<WebPagePopupImpl*>(view()->pagePopup());

[dcheng, 2017/04/07 06:32:01]

Let's take advantage of the fact that view() already returns a WebViewImpl and
change pagePopup() to just return a WebPagePopupImpl for WebViewImpl. Covariant
return types are allowed for virtual functions, so this should work.

[EhsanK, 2017/04/07 16:16:12]

Yes good idea. Thanks!

+    view()->hidePopups();
+  }
+
   // Take capture on a mouse down on a plugin so we can send it mouse events.
   // If the hit node is a plugin but a scrollbar is over it don't start mouse
   // capture because it will interfere with the scrollbar receiving events.
@@ -792,6 +801,15 @@ void WebFrameWidgetImpl::handleMouseDown(LocalFrame& mainFrame,
     m_mouseCaptureGestureToken =
         mainFrame.eventHandler().takeLastMouseDownGestureToken();
 
+  WebPagePopupImpl* pagePopupImpl =

[dcheng, 2017/04/07 06:32:01]

Nit: Call this currentPagePopup

[EhsanK, 2017/04/07 16:16:12]

Or I can just remove it now that view()->pagePopup() is a WebPagePopupImpl*.

+      static_cast<WebPagePopupImpl*>(view()->pagePopup());
+  if (pagePopupImpl && pagePopupImpl == pagePopup &&
+      pagePopupImpl->hasSamePopupClient(pagePopup.get())) {
+    // That click triggered a page popup that is the same as the one we just
+    // closed.  It needs to be closed.
+    view()->hidePopups();

[dcheng, 2017/04/07 06:32:01]

The logic in WebViewImpl calls cancel() -- is it OK to call a different method
here?

[EhsanK, 2017/04/07 16:16:12]

hidePopups() simply calls cancelPagePopup(). That being said, I guess
hidePopups() should be replaced by cancelPagePopup() in the public API. The
reason is that hidePopups() used to call hideSelectPopup() or hideAutofill...
amongs other things. But they are removed now, e.g., here
https://codereview.chromium.org/1142303005

Now it simply calls cancelPagePopup() and I think its name is only as accurate
as the comment above it in WebView.h (which is wrong):
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebView.h?...

+  }
+
   // Dispatch the contextmenu event regardless of if the click was swallowed.
   if (!page()->settings().getShowContextMenuOnMouseUp()) {
 #if OS(MACOSX)