[Autofill] Refactor AutofillPopupControllerImpl and AutofillPopupBaseView

AutofillPopupBaseView should not handle user events for its children.
This CL cleans up user-event functions from AutofillPopupBaseView.

Bug: 1045401
Change-Id: I6fd91fa99b6de060393bd89c2fc2171209882fda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2050273
Commit-Queue: Vidhan Jain <vidhanj@google.com>
Reviewed-by: Matthias Körber <koerber@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742701}

chrome/browser/autofill/autofill_keyboard_accessory_adapter.cc

@@ -180,30 +180,11 @@ void AutofillKeyboardAccessoryAdapter::ViewDestroyed() {
   delete this;  // Remove dangling weak reference.
 }
 
-void AutofillKeyboardAccessoryAdapter::SetSelectionAtPoint(
-    const gfx::Point& point) {
-  if (controller_)
-    controller_->SetSelectionAtPoint(point);
-}
-
-bool AutofillKeyboardAccessoryAdapter::AcceptSelectedLine() {
-  return controller_ && controller_->AcceptSelectedLine();
-}
-
 void AutofillKeyboardAccessoryAdapter::SelectionCleared() {
   if (controller_)
     controller_->SelectionCleared();
 }
 
-bool AutofillKeyboardAccessoryAdapter::HasSelection() const {
-  return controller_ && controller_->HasSelection();
-}
-
-gfx::Rect AutofillKeyboardAccessoryAdapter::popup_bounds() const {
-  DCHECK(controller_) << "Call OnSuggestionsChanged only from its owner!";
-  return controller_->popup_bounds();
-}
-
 gfx::NativeView AutofillKeyboardAccessoryAdapter::container_view() const {
   DCHECK(controller_) << "Call OnSuggestionsChanged only from its owner!";
   return controller_->container_view();

chrome/browser/autofill/autofill_keyboard_accessory_adapter.h

@@ -78,14 +78,9 @@ class AutofillKeyboardAccessoryAdapter : public AutofillPopupView,
   base::Optional<int> selected_line() const override;
   const AutofillPopupLayoutModel& layout_model() const override;
 
-  // AutofillPopupViewDelegate implementation
   void Hide(PopupHidingReason reason) override;
   void ViewDestroyed() override;
-  void SetSelectionAtPoint(const gfx::Point& point) override;
-  bool AcceptSelectedLine() override;
   void SelectionCleared() override;
-  bool HasSelection() const override;
-  gfx::Rect popup_bounds() const override;
   gfx::NativeView container_view() const override;
   const gfx::RectF& element_bounds() const override;
   bool IsRTL() const override;

chrome/browser/autofill/autofill_keyboard_accessory_adapter_unittest.cc

@@ -233,9 +233,6 @@ TEST_F(AutofillKeyboardAccessoryAdapterTest, MapSelectedLineToChangedIndices) {
 
   EXPECT_CALL(*controller(), selected_line()).WillRepeatedly(Return(0));
   EXPECT_EQ(adapter_as_controller()->selected_line(), 1);
-
-  EXPECT_CALL(*controller(), AcceptSelectedLine());
-  adapter_as_controller()->AcceptSelectedLine();
 }
 
 }  // namespace autofill

chrome/browser/ui/autofill/autofill_popup_controller_impl.cc

@@ -310,31 +310,10 @@ void AutofillPopupControllerImpl::OnSuggestionsChanged() {
   view_->OnSuggestionsChanged();
 }
 
-void AutofillPopupControllerImpl::SetSelectionAtPoint(const gfx::Point& point) {
-  SetSelectedLine(layout_model_.LineFromY(point.y()));
-}
-
-bool AutofillPopupControllerImpl::AcceptSelectedLine() {
-  if (!selected_line_)
-    return false;
-
-  DCHECK_LT(*selected_line_, GetLineCount());
-
-  if (!CanAccept(suggestions_[*selected_line_].frontend_id))
-    return false;
-
-  AcceptSuggestion(*selected_line_);
-  return true;
-}
-
 void AutofillPopupControllerImpl::SelectionCleared() {
   SetSelectedLine(base::nullopt);
 }
 
-bool AutofillPopupControllerImpl::HasSelection() const {
-  return selected_line_.has_value();
-}
-
 void AutofillPopupControllerImpl::AcceptSuggestion(int index) {
   const Suggestion& suggestion = suggestions_[index];
 #if defined(OS_ANDROID)
@@ -349,10 +328,6 @@ void AutofillPopupControllerImpl::AcceptSuggestion(int index) {
                                  index);
 }
 
-gfx::Rect AutofillPopupControllerImpl::popup_bounds() const {
-  return layout_model_.popup_bounds();
-}
-
 gfx::NativeView AutofillPopupControllerImpl::container_view() const {
   return controller_common_.container_view;
 }
@@ -566,6 +541,19 @@ void AutofillPopupControllerImpl::ElideValueAndLabelForRow(
 }
 #endif
 
+bool AutofillPopupControllerImpl::AcceptSelectedLine() {
+  if (!selected_line_)
+    return false;
+
+  DCHECK_LT(*selected_line_, GetLineCount());
+
+  if (!CanAccept(suggestions_[*selected_line_].frontend_id))
+    return false;
+
+  AcceptSuggestion(*selected_line_);
+  return true;
+}
+
 void AutofillPopupControllerImpl::ClearState() {
   // Don't clear view_, because otherwise the popup will have to get regenerated
   // and this will cause flickering.

chrome/browser/ui/autofill/autofill_popup_controller_impl.h

@@ -90,12 +90,7 @@ class AutofillPopupControllerImpl : public AutofillPopupController {
                               base::i18n::TextDirection text_direction);
   ~AutofillPopupControllerImpl() override;
 
-  // AutofillPopupViewDelegate implementation.
-  void SetSelectionAtPoint(const gfx::Point& point) override;
-  bool AcceptSelectedLine() override;
   void SelectionCleared() override;
-  bool HasSelection() const override;
-  gfx::Rect popup_bounds() const override;
   gfx::NativeView container_view() const override;
   const gfx::RectF& element_bounds() const override;
   void SetElementBounds(const gfx::RectF& bounds);
@@ -164,6 +159,10 @@ class AutofillPopupControllerImpl : public AutofillPopupController {
   void ElideValueAndLabelForRow(int row, int available_width);
 #endif
 
+  // The user has accepted the currently selected line. Returns whether there
+  // was a selection to accept.
+  bool AcceptSelectedLine();
+
   // Clear the internal state of the controller. This is needed to ensure that
   // when the popup is reused it doesn't leak values between uses.
   void ClearState();

chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc

@@ -161,7 +161,6 @@ class TestAutofillPopupController : public AutofillPopupControllerImpl {
   using AutofillPopupControllerImpl::GetRootAXPlatformNodeForWebContents;
   using AutofillPopupControllerImpl::GetSuggestionAt;
   using AutofillPopupControllerImpl::GetWeakPtr;
-  using AutofillPopupControllerImpl::popup_bounds;
   using AutofillPopupControllerImpl::RemoveSelectedLine;
   using AutofillPopupControllerImpl::selected_line;
   using AutofillPopupControllerImpl::SelectNextLine;

chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc

@@ -7,6 +7,7 @@
 #include <stddef.h>
 
 #include <memory>
+#include <vector>
 
 #include "chrome/browser/ui/autofill/autofill_popup_view.h"
 #include "chrome/browser/ui/autofill/autofill_popup_view_delegate.h"
@@ -36,11 +37,7 @@ class TestAutofillPopupViewDelegate : public AutofillPopupViewDelegate {
 
   void Hide(PopupHidingReason reason) override {}
   void ViewDestroyed() override {}
-  void SetSelectionAtPoint(const gfx::Point& point) override {}
-  bool AcceptSelectedLine() override { return true; }
   void SelectionCleared() override {}
-  bool HasSelection() const override { return false; }
-  gfx::Rect popup_bounds() const override { return gfx::Rect(0, 0, 100, 100); }
   gfx::NativeView container_view() const override { return container_view_; }
   const gfx::RectF& element_bounds() const override { return element_bounds_; }
   bool IsRTL() const override { return false; }

chrome/browser/ui/autofill/autofill_popup_view_delegate.h

@@ -34,24 +34,10 @@ class AutofillPopupViewDelegate {
   // Called whent the popup view was destroyed.
   virtual void ViewDestroyed() = 0;
 
-  // The user has selected |point|, e.g. by hovering the mouse cursor. |point|
-  // must be in popup coordinates.
-  virtual void SetSelectionAtPoint(const gfx::Point& point) = 0;
-
-  // The user has accepted the currently selected line. Returns whether there
-  // was a selection to accept.
-  virtual bool AcceptSelectedLine() = 0;
-
   // The user cleared the current selection, e.g. by moving the mouse cursor
   // out of the popup bounds.
   virtual void SelectionCleared() = 0;
 
-  // Returns true if any of the suggestions is selected.
-  virtual bool HasSelection() const = 0;
-
-  // The actual bounds of the popup.
-  virtual gfx::Rect popup_bounds() const = 0;
-
   // The view that the form field element sits in.
   virtual gfx::NativeView container_view() const = 0;
 

chrome/browser/ui/passwords/password_generation_popup_controller_impl.cc

@@ -8,6 +8,8 @@
 #include <stddef.h>
 
 #include <algorithm>
+#include <utility>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/i18n/rtl.h"
@@ -296,27 +298,10 @@ void PasswordGenerationPopupControllerImpl::ViewDestroyed() {
   Hide(PopupHidingReason::kViewDestroyed);
 }
 
-void PasswordGenerationPopupControllerImpl::SetSelectionAtPoint(
-    const gfx::Point& point) {
-  // The view handles mouse events itself.
-}
-
-bool PasswordGenerationPopupControllerImpl::AcceptSelectedLine() {
-  if (!password_selected_)
-    return false;
-
-  PasswordAccepted();
-  return true;
-}
-
 void PasswordGenerationPopupControllerImpl::SelectionCleared() {
   PasswordSelected(false);
 }
 
-bool PasswordGenerationPopupControllerImpl::HasSelection() const {
-  return password_selected();
-}
-
 void PasswordGenerationPopupControllerImpl::SetSelected() {
   PasswordSelected(true);
 }
@@ -325,11 +310,6 @@ gfx::NativeView PasswordGenerationPopupControllerImpl::container_view() const {
   return controller_common_.container_view;
 }
 
-gfx::Rect PasswordGenerationPopupControllerImpl::popup_bounds() const {
-  NOTREACHED();
-  return gfx::Rect();
-}
-
 const gfx::RectF& PasswordGenerationPopupControllerImpl::element_bounds()
     const {
   return controller_common_.element_bounds;

chrome/browser/ui/passwords/password_generation_popup_controller_impl.h

@@ -6,7 +6,9 @@
 #define CHROME_BROWSER_UI_PASSWORDS_PASSWORD_GENERATION_POPUP_CONTROLLER_IMPL_H_
 
 #include <stddef.h>
+#include <memory>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -129,14 +131,10 @@ class PasswordGenerationPopupControllerImpl
   // PasswordGenerationPopupController implementation:
   void Hide(autofill::PopupHidingReason reason) override;
   void ViewDestroyed() override;
-  void SetSelectionAtPoint(const gfx::Point& point) override;
-  bool AcceptSelectedLine() override;
   void SelectionCleared() override;
-  bool HasSelection() const override;
   void SetSelected() override;
   void PasswordAccepted() override;
   gfx::NativeView container_view() const override;
-  gfx::Rect popup_bounds() const override;
   const gfx::RectF& element_bounds() const override;
   bool IsRTL() const override;
   const std::vector<autofill::Suggestion> GetSuggestions() override;

chrome/browser/ui/views/autofill/autofill_popup_base_view.cc

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/ui/views/autofill/autofill_popup_base_view.h"
 
+#include <memory>
+#include <utility>
+
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
@@ -203,66 +206,6 @@ void AutofillPopupBaseView::OnNativeFocusChanged(gfx::NativeView focused_now) {
     HideController(PopupHidingReason::kFocusChanged);
 }
 
-void AutofillPopupBaseView::OnMouseCaptureLost() {
-  ClearSelection();
-}
-
-bool AutofillPopupBaseView::OnMouseDragged(const ui::MouseEvent& event) {
-  if (HitTestPoint(event.location())) {
-    SetSelection(event.location());
-
-    // We must return true in order to get future OnMouseDragged and
-    // OnMouseReleased events.
-    return true;
-  }
-
-  // If we move off of the popup, we lose the selection.
-  ClearSelection();
-  return false;
-}
-
-void AutofillPopupBaseView::OnMouseExited(const ui::MouseEvent& event) {
-  // There is no need to post a ClearSelection task if no row is selected.
-  if (!delegate_ || !delegate_->HasSelection())
-    return;
-
-  // Pressing return causes the cursor to hide, which will generate an
-  // OnMouseExited event. Pressing return should activate the current selection
-  // via AcceleratorPressed, so we need to let that run first.
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&AutofillPopupBaseView::ClearSelection,
-                                weak_ptr_factory_.GetWeakPtr()));
-}
-
-void AutofillPopupBaseView::OnMouseMoved(const ui::MouseEvent& event) {
-  // A synthesized mouse move will be sent when the popup is first shown.
-  // Don't preview a suggestion if the mouse happens to be hovering there.
-#if defined(OS_WIN)
-  // TODO(rouslan): Use event.time_stamp() and ui::EventTimeForNow() when they
-  // become comparable. http://crbug.com/453559
-  if (base::Time::Now() - show_time_ <= base::TimeDelta::FromMilliseconds(50))
-    return;
-#else
-  if (event.flags() & ui::EF_IS_SYNTHESIZED)
-    return;
-#endif
-
-  if (HitTestPoint(event.location()))
-    SetSelection(event.location());
-  else
-    ClearSelection();
-}
-
-bool AutofillPopupBaseView::OnMousePressed(const ui::MouseEvent& event) {
-  return event.GetClickCount() == 1;
-}
-
-void AutofillPopupBaseView::OnMouseReleased(const ui::MouseEvent& event) {
-  // We only care about the left click.
-  if (event.IsOnlyLeftMouseButton() && HitTestPoint(event.location()))
-    AcceptSelection(event.location());
-}
-
 void AutofillPopupBaseView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   // TODO(aleventhal) The correct role spec-wise to use here is kMenu, however
   // as of NVDA 2018.2.1, firing a menu event with kMenu breaks left/right
@@ -274,24 +217,6 @@ void AutofillPopupBaseView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
       l10n_util::GetStringUTF16(IDS_AUTOFILL_POPUP_ACCESSIBLE_NODE_DATA));
 }
 
-void AutofillPopupBaseView::SetSelection(const gfx::Point& point) {
-  if (delegate_)
-    delegate_->SetSelectionAtPoint(point);
-}
-
-void AutofillPopupBaseView::AcceptSelection(const gfx::Point& point) {
-  if (!delegate_)
-    return;
-
-  delegate_->SetSelectionAtPoint(point);
-  delegate_->AcceptSelectedLine();
-}
-
-void AutofillPopupBaseView::ClearSelection() {
-  if (delegate_)
-    delegate_->SelectionCleared();
-}
-
 void AutofillPopupBaseView::HideController(PopupHidingReason reason) {
   if (delegate_)
     delegate_->Hide(reason);

chrome/browser/ui/views/autofill/autofill_popup_base_view.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_AUTOFILL_AUTOFILL_POPUP_BASE_VIEW_H_
 #define CHROME_BROWSER_UI_VIEWS_AUTOFILL_AUTOFILL_POPUP_BASE_VIEW_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
@@ -68,12 +70,6 @@ class AutofillPopupBaseView : public views::WidgetDelegateView,
   friend class AutofillPopupBaseViewTest;
 
   // views::Views implementation.
-  void OnMouseCaptureLost() override;
-  bool OnMouseDragged(const ui::MouseEvent& event) override;
-  void OnMouseExited(const ui::MouseEvent& event) override;
-  void OnMouseMoved(const ui::MouseEvent& event) override;
-  bool OnMousePressed(const ui::MouseEvent& event) override;
-  void OnMouseReleased(const ui::MouseEvent& event) override;
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
 
   // views::WidgetFocusChangeListener implementation.
@@ -87,10 +83,6 @@ class AutofillPopupBaseView : public views::WidgetDelegateView,
   // Stop observing the widget.
   void RemoveWidgetObservers();
 
-  void SetSelection(const gfx::Point& point);
-  void AcceptSelection(const gfx::Point& point);
-  void ClearSelection();
-
   // Hide the controller of this view. This assumes that doing so will
   // eventually hide this view in the process.
   void HideController(PopupHidingReason reason);

chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc

@@ -90,24 +90,6 @@ class AutofillPopupBaseViewTest : public InProcessBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(AutofillPopupBaseViewTest);
 };
 
-IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, DoubleClickTest) {
-  gfx::RectF bounds(0, 0, 5, 5);
-  EXPECT_CALL(mock_delegate_, element_bounds())
-      .WillRepeatedly(ReturnRef(bounds));
-
-  view_->SetPreferredSize(gfx::Size(10, 10));
-  ShowView();
-
-  gfx::Point point = view_->GetLocalBounds().CenterPoint();
-  ui::MouseEvent mouse_down(ui::ET_MOUSE_PRESSED, point, point,
-                            ui::EventTimeForNow(), 0, 0);
-  EXPECT_TRUE(static_cast<views::View*>(view_)->OnMousePressed(mouse_down));
-
-  // Ignore double clicks.
-  mouse_down.SetClickCount(2);
-  EXPECT_FALSE(static_cast<views::View*>(view_)->OnMousePressed(mouse_down));
-}
-
 // Regression test for crbug.com/391316
 IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, CorrectBoundsTest) {
   gfx::RectF bounds(100, 150, 5, 5);
@@ -128,23 +110,4 @@ IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, CorrectBoundsTest) {
   EXPECT_EQ(expected_point, display_point);
 }
 
-IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, MouseExitedTest) {
-  gfx::RectF bounds(0, 0, 5, 5);
-  EXPECT_CALL(mock_delegate_, element_bounds())
-      .WillRepeatedly(ReturnRef(bounds));
-  for (bool has_selection : {true, false}) {
-    EXPECT_CALL(mock_delegate_, HasSelection()).WillOnce(Return(has_selection));
-    EXPECT_CALL(mock_delegate_, SelectionCleared())
-        .Times(has_selection ? 1 : 0);
-
-    ShowView();
-
-    ui::MouseEvent exit_event(ui::ET_MOUSE_EXITED, gfx::Point(), gfx::Point(),
-                              ui::EventTimeForNow(), 0, 0);
-    static_cast<views::View*>(view_)->OnMouseExited(exit_event);
-
-    base::RunLoop().RunUntilIdle();
-  }
-}
-
 }  // namespace autofill

chrome/browser/ui/views/passwords/password_generation_popup_view_views.cc

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/ui/views/passwords/password_generation_popup_view_views.h"
 
+#include <algorithm>
+#include <memory>
+
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/passwords/password_generation_popup_controller.h"
@@ -57,9 +60,10 @@ class PasswordGenerationPopupViewViews::GeneratedPasswordBox
   void reset_controller() { controller_ = nullptr; }
 
  private:
-  // View:
+  // Implements the View interface.
   void OnMouseEntered(const ui::MouseEvent& event) override;
   void OnMouseExited(const ui::MouseEvent& event) override;
+  bool OnMousePressed(const ui::MouseEvent& event) override;
   void OnMouseReleased(const ui::MouseEvent& event) override;
   void OnGestureEvent(ui::GestureEvent* event) override;
 
@@ -104,6 +108,11 @@ void PasswordGenerationPopupViewViews::GeneratedPasswordBox::OnMouseExited(
     controller_->SelectionCleared();
 }
 
+bool PasswordGenerationPopupViewViews::GeneratedPasswordBox::OnMousePressed(
+    const ui::MouseEvent& event) {
+  return event.GetClickCount() == 1;
+}
+
 void PasswordGenerationPopupViewViews::GeneratedPasswordBox::OnMouseReleased(
     const ui::MouseEvent& event) {
   if (event.IsOnlyLeftMouseButton() && controller_)