Clean up WebPagePopup APIs and move them from WebWidget to WebView.

This moves WebWidget::GetPagePopup() to WebView, as there is only 1
popup per view, not per widget. WebTest code was grabbing the popup
off the WebWidget and is changed to grab it from the WebView instead
when the WebWidget is for the main frame (which it always is right now
but we add conditions for correctness regardless).

WebViewImpl had a bunch of ways to close the current popup, and we
remove most of the redundant ways: HideSelectPopup() is removed.
HidePopups() is removed, and CancelPagePopup() is changed to replace
it.

SetLastHiddenPagePopup() is removed from the public API of WebViewImpl
as it is no longer used externally, and code inside changed to use the
variable directly.

WebPagePopupClient::ClosePopup() is renamed to CancelPopup() to give
consistent naming throughout the Cancel code.

WebViewTestProxy changed to defer WidgetClient() through to the
WebViewTestClient, like it does for other methods, and we pass the
ProxyWebWidgetClient to the WebViewTestClient for it to hand out,
instead of giving it null.

R=dcheng@chromium.org
TBR=

Change-Id: Ibaddec943ead28153290cd03a3d94790ac5d4fe0
Bug: 912193
Reviewed-on: https://chromium-review.googlesource.com/c/1372657
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616455}

android_webview/renderer/aw_render_frame_ext.cc

@@ -272,7 +272,7 @@ void AwRenderFrameExt::OnSetTextZoomFactor(float zoom_factor) {
     return;
 
   // Hide selection and autofill popups.
-  webview->HidePopups();
+  webview->CancelPagePopup();
   webview->SetTextZoomFactor(zoom_factor);
 }
 

content/renderer/render_view_impl.cc

@@ -1091,7 +1091,7 @@ void RenderViewImpl::ApplyNewSizeForWidget(const gfx::Size& old_size,
     // the ChromeOS virtual keyboard) where we send a resize message with no
     // change in size, but we don't want to close popups.
     // See https://crbug.com/761908.
-    webview()->HidePopups();
+    webview()->CancelPagePopup();
   }
 }
 
@@ -1837,7 +1837,7 @@ void RenderViewImpl::OnSetPageScale(float page_scale_factor) {
 }
 
 void RenderViewImpl::UpdateZoomLevel(double zoom_level) {
-  webview()->HidePopups();
+  webview()->CancelPagePopup();
   SetZoomLevel(zoom_level);
 }
 
@@ -1933,7 +1933,7 @@ void RenderViewImpl::OnClosePage() {
 
 void RenderViewImpl::OnMoveOrResizeStarted() {
   if (webview())
-    webview()->HidePopups();
+    webview()->CancelPagePopup();
 }
 
 void RenderViewImpl::OnPageWasHidden() {

content/shell/renderer/web_test/web_test_render_frame_observer.cc

@@ -55,10 +55,20 @@ void WebTestRenderFrameObserver::CaptureDump(CaptureDumpCallback callback) {
 void WebTestRenderFrameObserver::CompositeWithRaster(
     CompositeWithRasterCallback callback) {
   blink::WebWidget* widget = render_frame()->GetWebFrame()->FrameWidget();
+  blink::WebView* view = render_frame()->GetWebFrame()->View();
   if (widget) {
     widget->UpdateAllLifecyclePhasesAndCompositeForTesting(/*do_raster=*/true);
-    if (blink::WebPagePopup* popup = widget->GetPagePopup())
-      popup->UpdateAllLifecyclePhasesAndCompositeForTesting(/*do_raster=*/true);
+
+    // The current PagePopup is composited together with the main frame.
+    // TODO(danakj): This means that an OOPIF's popup, which is attached to a
+    // WebView without a main frame, would have no opportunity to execute this
+    // method call.
+    if (render_frame()->IsMainFrame()) {
+      if (blink::WebPagePopup* popup = view->GetPagePopup()) {
+        popup->UpdateAllLifecyclePhasesAndCompositeForTesting(
+            /*do_raster=*/true);
+      }
+    }
   }
   std::move(callback).Run();
 }

content/shell/test_runner/event_sender.cc

@@ -2305,7 +2305,7 @@ void EventSender::SendCurrentTouchEvent(WebInputEvent::Type type,
       HandleInputEventOnViewOrPopup(pointer_event);
     }
   }
-  WebPagePopup* popup = widget()->GetPagePopup();
+  WebPagePopup* popup = view()->GetPagePopup();
   if (popup)
     popup->DispatchBufferedTouchEvents();
   else
@@ -2783,7 +2783,7 @@ WebInputEventResult EventSender::HandleInputEventOnViewOrPopup(
     const WebInputEvent& raw_event) {
   last_event_timestamp_ = raw_event.TimeStamp();
 
-  WebPagePopup* popup = widget()->GetPagePopup();
+  WebPagePopup* popup = view()->GetPagePopup();
   if (popup && !WebInputEvent::IsKeyboardEventType(raw_event.GetType())) {
     // ui::ScaleWebInputEvent returns nullptr when the scale is 1.0f as the
     // event does not have to be converted.

content/shell/test_runner/layout_and_paint_async_then.cc

@@ -12,11 +12,12 @@
 
 namespace test_runner {
 
-void LayoutAndPaintAsyncThen(blink::WebWidget* web_widget,
+void LayoutAndPaintAsyncThen(blink::WebPagePopup* popup,
+                             blink::WebWidget* web_widget,
                              base::OnceClosure callback) {
   TRACE_EVENT0("shell", "LayoutAndPaintAsyncThen");
 
-  if (blink::WebPagePopup* popup = web_widget->GetPagePopup()) {
+  if (popup) {
     auto barrier = base::BarrierClosure(2, std::move(callback));
     web_widget->LayoutAndPaintAsync(barrier);
     popup->LayoutAndPaintAsync(barrier);

content/shell/test_runner/layout_and_paint_async_then.h

@@ -9,15 +9,17 @@
 #include "content/shell/test_runner/test_runner_export.h"
 
 namespace blink {
+class WebPagePopup;
 class WebWidget;
 }  // namespace blink
 
 namespace test_runner {
 
-// Triggers a layout and paint of |web_widget| and its popup (if any).
-// Calls |callback| after the layout and paint happens (for both the
-// |web_widget| and its popup (if any)).
-TEST_RUNNER_EXPORT void LayoutAndPaintAsyncThen(blink::WebWidget* web_widget,
+// Triggers a layout and paint of WebWidget and the active popup (if any).
+// Calls |callback| after the layout and paint happens for both the
+// widget and the popup.
+TEST_RUNNER_EXPORT void LayoutAndPaintAsyncThen(blink::WebPagePopup* popup,
+                                                blink::WebWidget* web_widget,
                                                 base::OnceClosure callback);
 
 }  // namespace test_runner

content/shell/test_runner/pixel_dump.cc

@@ -30,6 +30,8 @@
 #include "third_party/blink/public/web/web_local_frame.h"
 #include "third_party/blink/public/web/web_page_popup.h"
 #include "third_party/blink/public/web/web_print_params.h"
+#include "third_party/blink/public/web/web_view.h"
+#include "third_party/blink/public/web/web_widget.h"
 #include "ui/gfx/geometry/point.h"
 
 namespace test_runner {
@@ -135,13 +137,16 @@ void DumpPixelsAsync(blink::WebLocalFrame* web_frame,
   auto did_readback = base::BindRepeating(
       &CaptureCallback::DidCompositeAndReadback, capture_callback);
   web_widget->CompositeAndReadbackAsync(did_readback);
-  if (blink::WebPagePopup* popup = web_widget->GetPagePopup()) {
-    capture_callback->set_wait_for_popup(true);
-    blink::WebPoint position = popup->PositionRelativeToOwner();
-    position.x *= device_scale_factor_for_test;
-    position.y *= device_scale_factor_for_test;
-    capture_callback->set_popup_position(position);
-    popup->CompositeAndReadbackAsync(did_readback);
+  // The current PagePopup is composited together with the main frame.
+  if (!web_frame->Parent()) {
+    if (blink::WebPagePopup* popup = web_frame->View()->GetPagePopup()) {
+      capture_callback->set_wait_for_popup(true);
+      blink::WebPoint position = popup->PositionRelativeToOwner();
+      position.x *= device_scale_factor_for_test;
+      position.y *= device_scale_factor_for_test;
+      capture_callback->set_popup_position(position);
+      popup->CompositeAndReadbackAsync(did_readback);
+    }
   }
 }
 

content/shell/test_runner/test_runner_for_specific_view.cc

@@ -221,15 +221,18 @@ void TestRunnerForSpecificView::LayoutAndPaintAsync() {
   // TODO(lfg, lukasza): TestRunnerForSpecificView assumes that there's a single
   // WebWidget for the entire view, but with out-of-process iframes there may be
   // multiple WebWidgets, one for each local root. We should look into making
-  // this structure more generic.
+  // this structure more generic. Also the PagePopup for an OOPIF would be
+  // attached to a WebView without a main frame, which should be handled.
   test_runner::LayoutAndPaintAsyncThen(
+      web_view()->GetPagePopup(),
       web_view()->MainFrame()->ToWebLocalFrame()->FrameWidget(),
       base::DoNothing());
 }
 
 void TestRunnerForSpecificView::LayoutAndPaintAsyncThen(
     v8::Local<v8::Function> callback) {
   test_runner::LayoutAndPaintAsyncThen(
+      web_view()->GetPagePopup(),
       web_view()->MainFrame()->ToWebLocalFrame()->FrameWidget(),
       CreateClosureThatPostsV8Callback(callback));
 }

content/shell/test_runner/web_test_interfaces.cc

@@ -10,10 +10,7 @@
 #include "content/shell/test_runner/test_interfaces.h"
 #include "content/shell/test_runner/test_runner.h"
 #include "content/shell/test_runner/web_frame_test_client.h"
-#include "content/shell/test_runner/web_view_test_client.h"
 #include "content/shell/test_runner/web_view_test_proxy.h"
-#include "content/shell/test_runner/web_widget_test_client.h"
-#include "content/shell/test_runner/web_widget_test_proxy.h"
 #include "third_party/blink/public/platform/modules/webmidi/web_midi_accessor.h"
 
 namespace test_runner {
@@ -74,19 +71,6 @@ std::unique_ptr<WebFrameTestClient> WebTestInterfaces::CreateWebFrameTestClient(
                                               web_frame_test_proxy_base);
 }
 
-std::unique_ptr<WebViewTestClient> WebTestInterfaces::CreateWebViewTestClient(
-    WebViewTestProxyBase* web_view_test_proxy_base,
-    std::unique_ptr<WebWidgetTestClient> web_widget_test_client) {
-  return std::make_unique<WebViewTestClient>(web_view_test_proxy_base,
-                                             std::move(web_widget_test_client));
-}
-
-std::unique_ptr<WebWidgetTestClient>
-WebTestInterfaces::CreateWebWidgetTestClient(
-    WebWidgetTestProxyBase* web_widget_test_proxy_base) {
-  return std::make_unique<WebWidgetTestClient>(web_widget_test_proxy_base);
-}
-
 std::vector<blink::WebView*> WebTestInterfaces::GetWindowList() {
   std::vector<blink::WebView*> result;
   for (WebViewTestProxyBase* proxy : interfaces_->GetWindowList())

content/shell/test_runner/web_test_interfaces.h

@@ -29,10 +29,7 @@ class WebFrameTestClient;
 class WebFrameTestProxyBase;
 class WebTestDelegate;
 class WebViewTestProxyBase;
-class WebWidgetTestProxyBase;
 class WebTestRunner;
-class WebViewTestClient;
-class WebWidgetTestClient;
 
 class TEST_RUNNER_EXPORT WebTestInterfaces {
  public:
@@ -70,21 +67,6 @@ class TEST_RUNNER_EXPORT WebTestInterfaces {
       WebViewTestProxyBase* web_view_test_proxy_base,
       WebFrameTestProxyBase* web_frame_test_proxy_base);
 
-  // Creates a WebViewClient implementation providing test behavior (i.e.
-  // providing a mocked speech recognizer).  The caller should guarantee that
-  // the returned pointer won't be used beyond the lifetime of WebTestInterfaces
-  // and/or the lifetime of |web_view_test_proxy_base|.
-  std::unique_ptr<WebViewTestClient> CreateWebViewTestClient(
-      WebViewTestProxyBase* web_view_test_proxy_base,
-      std::unique_ptr<WebWidgetTestClient> web_widget_test_client);
-
-  // Creates a WebWidgetClient implementation providing test behavior (i.e.
-  // providing a mocked screen orientation).  The caller should guarantee that
-  // the returned pointer won't be used beyond the lifetime of WebTestInterfaces
-  // and/or the lifetime of |web_widget_test_proxy_base|.
-  std::unique_ptr<WebWidgetTestClient> CreateWebWidgetTestClient(
-      WebWidgetTestProxyBase* web_widget_test_proxy_base);
-
   // Gets a list of currently opened windows created by the current test.
   std::vector<blink::WebView*> GetWindowList();
 

content/shell/test_runner/web_view_test_proxy.cc

@@ -186,21 +186,24 @@ void WebViewTestProxy::Initialize(WebTestInterfaces* interfaces,
   // On WebViewTestProxyBase.
   set_delegate(delegate);
 
-  std::unique_ptr<WebWidgetTestClient> web_widget_client =
-      interfaces->CreateWebWidgetTestClient(web_widget_test_proxy_base());
-  view_test_client_ = interfaces->CreateWebViewTestClient(this, nullptr);
-  // This uses the widget_test_client set above on WebWidgetTestProxyBase.
-  proxy_widget_client_ = std::make_unique<ProxyWebWidgetClient>(
-      RenderViewImpl::WidgetClient(), web_widget_client.get(),
+  auto test_widget_client = std::make_unique<WebWidgetTestClient>(
+      /*main_frame_widget=*/true, web_widget_test_proxy_base());
+  // This passes calls through to the the |test_widget_client| as well as the
+  // production client pulled from RenderViewImpl as needed.
+  auto proxy_widget_client = std::make_unique<ProxyWebWidgetClient>(
+      RenderViewImpl::WidgetClient(), test_widget_client.get(),
       RenderViewImpl::GetWidget());
+  // This returns the |proxy_widget_client| as the WebWidgetClient.
+  view_test_client_ =
+      std::make_unique<WebViewTestClient>(this, std::move(proxy_widget_client));
 
   // On WebWidgetTestProxyBase.
   // It's weird that the WebView has the proxy client, but the
   // WebWidgetProxyBase has the test one only, but that's because the
   // WebWidgetTestProxyBase does not itself use the WebWidgetClient, only its
   // subclasses do.
   web_widget_test_proxy_base()->set_widget_test_client(
-      std::move(web_widget_client));
+      std::move(test_widget_client));
 
   // On WebViewTestProxyBase.
   set_test_interfaces(interfaces->GetTestInterfaces());
@@ -249,7 +252,7 @@ blink::WebScreenInfo WebViewTestProxy::GetScreenInfo() {
 }
 
 blink::WebWidgetClient* WebViewTestProxy::WidgetClient() {
-  return proxy_widget_client_.get();
+  return view_test_client_->WidgetClient();
 }
 
 WebViewTestProxy::~WebViewTestProxy() = default;

content/shell/test_runner/web_view_test_proxy.h

@@ -206,7 +206,6 @@ class TEST_RUNNER_EXPORT WebViewTestProxy : public content::RenderViewImpl,
   // RenderViewImpl has no public destructor.
   ~WebViewTestProxy() override;
 
-  std::unique_ptr<ProxyWebWidgetClient> proxy_widget_client_;
   std::unique_ptr<WebViewTestClient> view_test_client_;
 
   DISALLOW_COPY_AND_ASSIGN(WebViewTestProxy);

content/shell/test_runner/web_widget_test_client.cc

@@ -17,13 +17,16 @@
 #include "content/shell/test_runner/web_widget_test_proxy.h"
 #include "third_party/blink/public/platform/web_screen_info.h"
 #include "third_party/blink/public/web/web_page_popup.h"
+#include "third_party/blink/public/web/web_view.h"
 #include "third_party/blink/public/web/web_widget.h"
 
 namespace test_runner {
 
 WebWidgetTestClient::WebWidgetTestClient(
+    bool main_frame_widget,
     WebWidgetTestProxyBase* web_widget_test_proxy_base)
-    : web_widget_test_proxy_base_(web_widget_test_proxy_base),
+    : main_frame_widget_(main_frame_widget),
+      web_widget_test_proxy_base_(web_widget_test_proxy_base),
       animation_scheduled_(false),
       weak_factory_(this) {
   DCHECK(web_widget_test_proxy_base_);
@@ -52,9 +55,19 @@ void WebWidgetTestClient::AnimateNow() {
   blink::WebWidget* web_widget = web_widget_test_proxy_base_->web_widget();
   web_widget->UpdateAllLifecyclePhasesAndCompositeForTesting(
       animation_requires_raster);
-  if (blink::WebPagePopup* popup = web_widget->GetPagePopup()) {
-    popup->UpdateAllLifecyclePhasesAndCompositeForTesting(
-        animation_requires_raster);
+
+  // If this is the main frame, we composite the current PagePopup with the
+  // widget.
+  // TODO(danakj): This means that an OOPIF's popup, which is attached to a
+  // WebView without a main frame, would have no opportunity to execute this
+  // method call.
+  if (main_frame_widget_) {
+    blink::WebView* view =
+        web_widget_test_proxy_base_->web_view_test_proxy_base()->web_view();
+    if (blink::WebPagePopup* popup = view->GetPagePopup()) {
+      popup->UpdateAllLifecyclePhasesAndCompositeForTesting(
+          animation_requires_raster);
+    }
   }
 }
 

content/shell/test_runner/web_widget_test_client.h

@@ -25,7 +25,8 @@ class WebWidgetTestClient : public blink::WebWidgetClient {
  public:
   // Caller has to ensure that all arguments (i.e. |test_runner| and |delegate|)
   // live longer than |this|.
-  WebWidgetTestClient(WebWidgetTestProxyBase* web_widget_test_proxy_base);
+  WebWidgetTestClient(bool main_frame_widget,
+                      WebWidgetTestProxyBase* web_widget_test_proxy_base);
 
   ~WebWidgetTestClient() override;
 
@@ -49,6 +50,8 @@ class WebWidgetTestClient : public blink::WebWidgetClient {
   TestRunnerForSpecificView* view_test_runner();
   TestRunner* test_runner();
 
+  const bool main_frame_widget_;
+
   // Borrowed pointer to WebWidgetTestProxyBase.
   WebWidgetTestProxyBase* web_widget_test_proxy_base_;
 

content/shell/test_runner/web_widget_test_proxy.cc

@@ -35,7 +35,8 @@ void WebWidgetTestProxy::Initialize(
   // On WebWidgetTestProxyBase.
   set_web_widget(web_widget);
   set_web_view_test_proxy_base(view_proxy_for_local_root);
-  set_widget_test_client(interfaces->CreateWebWidgetTestClient(this));
+  set_widget_test_client(
+      std::make_unique<WebWidgetTestClient>(/*main_frame_widget=*/false, this));
 }
 
 void WebWidgetTestProxy::ScheduleAnimation() {

third_party/blink/public/web/web_view.h

@@ -48,6 +48,7 @@ class WebFrame;
 class WebHitTestResult;
 class WebLocalFrame;
 class WebPageImportanceSignals;
+class WebPagePopup;
 class WebPrerendererClient;
 class WebRemoteFrame;
 class WebSettings;
@@ -335,8 +336,11 @@ class WebView {
   // Sets whether select popup menus should be rendered by the browser.
   BLINK_EXPORT static void SetUseExternalPopupMenus(bool);
 
-  // Hides any popup (suggestions, selects...) that might be showing.
-  virtual void HidePopups() = 0;
+  // Cancels and hides the current popup (datetime, select...) if any.
+  virtual void CancelPagePopup() = 0;
+
+  // Returns the current popup if any.
+  virtual WebPagePopup* GetPagePopup() const = 0;
 
   // Visited link state --------------------------------------------------
 

third_party/blink/public/web/web_widget.h

@@ -243,10 +243,6 @@ class WebWidget {
     return 0xFFFFFFFF; /* SK_ColorWHITE */
   }
 
-  // The currently open page popup, which are calendar and datalist pickers
-  // but not the select popup.
-  virtual WebPagePopup* GetPagePopup() const { return 0; }
-
   // Called by client to request showing the context menu.
   virtual void ShowContextMenu(WebMenuSourceType) {}
 

third_party/blink/renderer/core/exported/web_page_popup_impl.cc

@@ -594,7 +594,7 @@ WebPoint WebPagePopupImpl::PositionRelativeToOwner() {
 
 void WebPagePopupImpl::Cancel() {
   if (popup_client_)
-    popup_client_->ClosePopup();
+    popup_client_->CancelPopup();
 }
 
 WebRect WebPagePopupImpl::WindowRectInScreen() const {