AutofillDialogControllerImpl refinements

1) merge Hide() and DelegateDestroyed()
2) remove possible hide/show race condition

BUG=none


Review URL: https://chromiumcodereview.appspot.com/11570057

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@174098 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 121 insertions, 25 deletions

diff --git a/chrome/browser/autofill/autofill_external_delegate.cc b/chrome/browser/autofill/autofill_external_delegate.cc
index 4afc3d6..c20ad81 100644
--- a/chrome/browser/autofill/autofill_external_delegate.cc
+++ b/chrome/browser/autofill/autofill_external_delegate.cc
@@ -65,7 +65,7 @@ AutofillExternalDelegate::AutofillExternalDelegate(
 
 AutofillExternalDelegate::~AutofillExternalDelegate() {
   if (controller_)
-    controller_->DelegateDestroyed();
+    controller_->Hide();
 }
 
 void AutofillExternalDelegate::SelectAutofillSuggestionAtIndex(int unique_id) {
@@ -172,11 +172,9 @@ void AutofillExternalDelegate::OnShowPasswordSuggestions(
 
 void AutofillExternalDelegate::EnsurePopupForElement(
     const gfx::Rect& element_bounds) {
-  if (controller_)
-    return;
-
   // |controller_| owns itself.
-  controller_ = new AutofillPopupControllerImpl(
+  controller_ = AutofillPopupControllerImpl::GetOrCreate(
+      controller_,
       this,
       // web_contents() may be NULL during testing.
       web_contents() ? web_contents()->GetView()->GetContentNativeView() : NULL,
@@ -281,6 +279,9 @@ void AutofillExternalDelegate::HideAutofillPopup() {
   if (controller_) {
     ClearPreviewedForm();
     controller_->Hide();
+    // Go ahead and invalidate |controller_|. After calling Hide(), it won't
+    // inform |this| of its destruction.
+    ControllerDestroyed();
   }
 }
 
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
index ae01eb0..2d74b87 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
@@ -60,6 +60,27 @@ const DataResource kDataResources[] = {
 
 }  // end namespace
 
+// static
+AutofillPopupControllerImpl* AutofillPopupControllerImpl::GetOrCreate(
+    AutofillPopupControllerImpl* previous,
+    AutofillPopupDelegate* delegate,
+    gfx::NativeView container_view,
+    const gfx::Rect& element_bounds) {
+  DCHECK(!previous || previous->delegate_ == delegate);
+
+  if (previous &&
+      previous->container_view() == container_view &&
+      previous->element_bounds() == element_bounds) {
+    return previous;
+  }
+
+  if (previous)
+    previous->Hide();
+
+  return new AutofillPopupControllerImpl(
+      delegate, container_view, element_bounds);
+}
+
 AutofillPopupControllerImpl::AutofillPopupControllerImpl(
     AutofillPopupDelegate* delegate,
     gfx::NativeView container_view,
@@ -69,7 +90,8 @@ AutofillPopupControllerImpl::AutofillPopupControllerImpl(
       container_view_(container_view),
       element_bounds_(element_bounds),
       selected_line_(kNoSelection),
-      delete_icon_hovered_(false) {
+      delete_icon_hovered_(false),
+      is_hiding_(false) {
 #if !defined(OS_ANDROID)
   label_font_ = value_font_.DeriveFont(kLabelFontSizeDelta);
 #endif
@@ -111,15 +133,8 @@ void AutofillPopupControllerImpl::Show(
 }
 
 void AutofillPopupControllerImpl::Hide() {
-  if (view_)
-    view_->Hide();
-  else
-    delete this;
-}
-
-void AutofillPopupControllerImpl::DelegateDestroyed() {
   delegate_ = NULL;
-  Hide();
+  HideInternal();
 }
 
 void AutofillPopupControllerImpl::ViewDestroyed() {
@@ -316,7 +331,7 @@ bool AutofillPopupControllerImpl::HandleKeyPressEvent(
       SetSelectedLine(autofill_values().size() - 1);
       return true;
     case ui::VKEY_ESCAPE:
-      Hide();
+      HideInternal();
       return true;
     case ui::VKEY_DELETE:
       return (event.modifiers & content::NativeWebKeyboardEvent::ShiftKey) &&
@@ -328,6 +343,17 @@ bool AutofillPopupControllerImpl::HandleKeyPressEvent(
   }
 }
 
+void AutofillPopupControllerImpl::HideInternal() {
+  if (is_hiding_)
+    return;
+  is_hiding_ = true;
+
+  if (view_)
+    view_->Hide();
+  else
+    delete this;
+}
+
 void AutofillPopupControllerImpl::SetSelectedLine(int selected_line) {
   if (selected_line_ == selected_line)
     return;
@@ -422,7 +448,7 @@ bool AutofillPopupControllerImpl::RemoveSelectedLine() {
     delegate_->ClearPreviewedForm();
     UpdateBoundsAndRedrawPopup();
   } else {
-    Hide();
+    HideInternal();
   }
 
   return true;
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_impl.h b/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
index 54cda77..4de5c2f 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
@@ -25,9 +25,14 @@ class KeyEvent;
 class AutofillPopupControllerImpl : public AutofillPopupController,
                                     public content::KeyboardListener {
  public:
-  AutofillPopupControllerImpl(AutofillPopupDelegate* delegate,
-                              gfx::NativeView container_view,
-                              const gfx::Rect& element_bounds);
+  // Creates a new |AutofillPopupControllerImpl|, or reuses |previous| if
+  // the construction arguments are the same. |previous| may be invalidated by
+  // this call.
+  static AutofillPopupControllerImpl* GetOrCreate(
+      AutofillPopupControllerImpl* previous,
+      AutofillPopupDelegate* delegate,
+      gfx::NativeView container_view,
+      const gfx::Rect& element_bounds);
 
   // Shows the popup, or updates the existing popup with the given values.
   void Show(const std::vector<string16>& autofill_values,
@@ -35,15 +40,17 @@ class AutofillPopupControllerImpl : public AutofillPopupController,
             const std::vector<string16>& autofill_icons,
             const std::vector<int>& autofill_unique_ids);
 
-  // Hides the popup and destroys the controller.
-  void Hide();
-
-  // Called when |delegate_| is no longer valid.
-  void DelegateDestroyed();
+  // Hides the popup and destroys the controller. This also invalidates
+  // |delegate_|. Virtual for testing.
+  virtual void Hide();
 
  protected:
   FRIEND_TEST_ALL_PREFIXES(AutofillExternalDelegateBrowserTest,
                            CloseWidgetAndNoLeaking);
+
+  AutofillPopupControllerImpl(AutofillPopupDelegate* delegate,
+                              gfx::NativeView container_view,
+                              const gfx::Rect& element_bounds);
   virtual ~AutofillPopupControllerImpl();
 
   // AutofillPopupController implementation.
@@ -82,6 +89,10 @@ class AutofillPopupControllerImpl : public AutofillPopupController,
   virtual bool HandleKeyPressEvent(
       const content::NativeWebKeyboardEvent& event) OVERRIDE;
 
+  // Like Hide(), but doesn't invalidate |delegate_| (the delegate will still
+  // be informed of destruction).
+  void HideInternal();
+
   // Change which line is currently selected by the user.
   void SetSelectedLine(int selected_line);
 
@@ -145,6 +156,9 @@ class AutofillPopupControllerImpl : public AutofillPopupController,
 
   // Used to indicate if the delete icon within a row is currently selected.
   bool delete_icon_hovered_;
+
+  // True if |HideInternal| has already been called.
+  bool is_hiding_;
 };
 
 #endif  // CHROME_BROWSER_UI_AUTOFILL_AUTOFILL_POPUP_CONTROLLER_IMPL_H_
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc b/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
index f90fd19..efc5e4d 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
@@ -57,9 +57,13 @@ class TestAutofillPopupController : public AutofillPopupControllerImpl {
   bool RemoveSelectedLine() {
     return AutofillPopupControllerImpl::RemoveSelectedLine();
   }
+  void DoHide() {
+    AutofillPopupControllerImpl::Hide();
+  }
 
   MOCK_METHOD1(InvalidateRow, void(size_t));
   MOCK_METHOD0(UpdateBoundsAndRedrawPopup, void());
+  MOCK_METHOD0(Hide, void());
 
  private:
   virtual void ShowView() OVERRIDE {}
@@ -77,7 +81,7 @@ class AutofillPopupControllerUnitTest : public ::testing::Test {
     // This will make sure the controller and the view (if any) are both
     // cleaned up.
     if (autofill_popup_controller_)
-      autofill_popup_controller_->Hide();
+      autofill_popup_controller_->DoHide();
   }
 
   AutofillPopupController* popup_controller() {
@@ -209,3 +213,54 @@ TEST_F(AutofillPopupControllerUnitTest, SkipSeparator) {
   autofill_popup_controller_->SelectPreviousLine();
   EXPECT_EQ(0, autofill_popup_controller_->selected_line());
 }
+
+TEST_F(AutofillPopupControllerUnitTest, GetOrCreate) {
+  MockAutofillExternalDelegate delegate;
+
+  AutofillPopupControllerImpl* controller =
+      AutofillPopupControllerImpl::GetOrCreate(
+          NULL,
+          &delegate,
+          NULL,
+          gfx::Rect());
+  EXPECT_TRUE(controller);
+
+  // This should not inform |delegate| of its destruction.
+  EXPECT_CALL(delegate, ControllerDestroyed()).Times(0);
+  controller->Hide();
+
+  controller =
+      AutofillPopupControllerImpl::GetOrCreate(
+          NULL,
+          &delegate,
+          NULL,
+          gfx::Rect());
+  EXPECT_TRUE(controller);
+  AutofillPopupControllerImpl* controller2 =
+      AutofillPopupControllerImpl::GetOrCreate(
+          controller,
+          &delegate,
+          NULL,
+          gfx::Rect());
+  EXPECT_EQ(controller, controller2);
+  controller->Hide();
+
+  TestAutofillPopupController* test_controller =
+      new TestAutofillPopupController(&delegate);
+  EXPECT_CALL(*test_controller, Hide());
+
+  gfx::Rect bounds(0, 0, 1, 2);
+  AutofillPopupControllerImpl* controller3 =
+      AutofillPopupControllerImpl::GetOrCreate(
+          test_controller,
+          &delegate,
+          NULL,
+          bounds);
+  EXPECT_EQ(
+      bounds,
+      static_cast<AutofillPopupController*>(controller3)->element_bounds());
+  controller3->Hide();
+
+  EXPECT_CALL(delegate, ControllerDestroyed());
+  delete test_controller;
+}