Use WeakPtr to simplify AutofillPopupControllerImpl memory management

chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autofill/test_autofill_external_delegate.h"
 #include "chrome/browser/ui/autofill/autofill_popup_controller_impl.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebAutofillClient.h"
 #include "ui/gfx/display.h"
 #include "ui/gfx/rect.h"
 
 using ::testing::_;
 using ::testing::AtLeast;
+using base::WeakPtr;
 using WebKit::WebAutofillClient;
 
 namespace {
 
 class MockAutofillExternalDelegate :
       public autofill::TestAutofillExternalDelegate {
  public:
   MockAutofillExternalDelegate() : TestAutofillExternalDelegate(NULL, NULL) {};
   virtual ~MockAutofillExternalDelegate() {};
 
@@ skipping 60 matching lines @@
     return AutofillPopupControllerImpl::subtext_font();
   }
 #endif
   int GetDesiredPopupWidth() const {
     return AutofillPopupControllerImpl::GetDesiredPopupWidth();
   }
   int GetDesiredPopupHeight() const {
     return AutofillPopupControllerImpl::GetDesiredPopupHeight();
   }
 
+  WeakPtr<AutofillPopupControllerImpl> GetWeakPtr() {
+    return AutofillPopupControllerImpl::GetWeakPtr();
+  }
+
   MOCK_METHOD1(InvalidateRow, void(size_t));
   MOCK_METHOD0(UpdateBoundsAndRedrawPopup, void());
   MOCK_METHOD0(Hide, void());
 
  private:
   virtual void ShowView() OVERRIDE {}
 
   gfx::Display display_;
 };
 
 }  // namespace
 
 class AutofillPopupControllerUnitTest : public ::testing::Test {
  public:
   AutofillPopupControllerUnitTest()
     : autofill_popup_controller_(
           new testing::NiceMock<TestAutofillPopupController>(
               &external_delegate_, gfx::Rect())) {}
+
   virtual ~AutofillPopupControllerUnitTest() {
     // This will make sure the controller and the view (if any) are both
     // cleaned up.
     if (autofill_popup_controller_)
       autofill_popup_controller_->DoHide();
   }
 
   AutofillPopupController* popup_controller() {
     return autofill_popup_controller_;
   }
@@ skipping 82 matching lines @@
       autofill_popup_controller_->subtexts().size() - 1);
   EXPECT_FALSE(autofill_popup_controller_->RemoveSelectedLine());
   EXPECT_LE(0, autofill_popup_controller_->selected_line());
 
   // Remove the first entry. The popup should be redrawn since its size has
   // changed.
   EXPECT_CALL(*autofill_popup_controller_, UpdateBoundsAndRedrawPopup());
   autofill_popup_controller_->SetSelectedLine(0);
   EXPECT_TRUE(autofill_popup_controller_->RemoveSelectedLine());
 
-  // Remove the last entry. The popup should then be hidden since there are
-  // no Autofill entries left.
-  EXPECT_CALL(external_delegate_, ControllerDestroyed());
-
   autofill_popup_controller_->SetSelectedLine(0);
-  // The controller self-deletes here, don't double delete.
+  EXPECT_CALL(*autofill_popup_controller_, Hide());
   EXPECT_TRUE(autofill_popup_controller_->RemoveSelectedLine());
-  autofill_popup_controller_ = NULL;
 }
 
 TEST_F(AutofillPopupControllerUnitTest, SkipSeparator) {
   // Set up the popup.
   std::vector<string16> names(3, string16());
   std::vector<int> autofill_ids;
   autofill_ids.push_back(1);
   autofill_ids.push_back(WebAutofillClient::MenuItemIDSeparator);
   autofill_ids.push_back(WebAutofillClient::MenuItemIDAutofillOptions);
   autofill_popup_controller_->Show(names, names, names, autofill_ids);
 
   autofill_popup_controller_->SetSelectedLine(0);
 
   // Make sure next skips the unselectable separator.
   autofill_popup_controller_->SelectNextLine();
   EXPECT_EQ(2, autofill_popup_controller_->selected_line());
 
   // Make sure previous skips the unselectable separator.
   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());
+  WeakPtr<AutofillPopupControllerImpl> controller;
+  controller = AutofillPopupControllerImpl::GetOrCreate(
+      controller, &delegate, NULL, gfx::Rect());

[Ilya Sherman, 2013/02/06 05:12:52]

Rather than passing the controller as an argument here, pass
WeakPtr<AutofillPopupControllerImpl>(), since that's really what you mean.

[kaiwang, 2013/02/06 05:56:21]

Done.

   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());
+  controller = AutofillPopupControllerImpl::GetOrCreate(
+      WeakPtr<AutofillPopupControllerImpl>(), &delegate, NULL, gfx::Rect());
   EXPECT_TRUE(controller);
-  AutofillPopupControllerImpl* controller2 =
-      AutofillPopupControllerImpl::GetOrCreate(
-          controller,
-          &delegate,
-          NULL,
-          gfx::Rect());
-  EXPECT_EQ(controller, controller2);
+
+  WeakPtr<AutofillPopupControllerImpl> controller2 =
+      AutofillPopupControllerImpl::GetOrCreate(controller, &delegate, NULL,
+                                               gfx::Rect());
+  EXPECT_EQ(controller.get(), controller2.get());
   controller->Hide();
 
   testing::NiceMock<TestAutofillPopupController>* test_controller =
       new testing::NiceMock<TestAutofillPopupController>(&delegate,
                                                          gfx::Rect());
   EXPECT_CALL(*test_controller, Hide());
 
   gfx::Rect bounds(0, 0, 1, 2);
   AutofillPopupControllerImpl* controller3 =
       AutofillPopupControllerImpl::GetOrCreate(
-          test_controller,
+          test_controller->GetWeakPtr(),
           &delegate,
           NULL,
           bounds);
   EXPECT_EQ(
       bounds,
       static_cast<AutofillPopupController*>(controller3)->element_bounds());
   controller3->Hide();
 
-  EXPECT_CALL(delegate, ControllerDestroyed());
   delete test_controller;
 }
 
 #if !defined(OS_ANDROID)
 TEST_F(AutofillPopupControllerUnitTest, ElideText) {
   std::vector<string16> names;
   names.push_back(ASCIIToUTF16("Text that will need to be trimmed"));
   names.push_back(ASCIIToUTF16("Untrimmed"));
 
   std::vector<string16> subtexts;
@@ skipping 79 matching lines @@
     TestAutofillPopupController autofill_popup_controller(&external_delegate_,
                                                           element_bounds[i]);
     autofill_popup_controller.set_display(display);
     autofill_popup_controller.Show(names, names, names, autofill_ids);
 
     EXPECT_EQ(expected_popup_bounds[i].ToString(),
               autofill_popup_controller.popup_bounds().ToString()) <<
         "Popup bounds failed to match for test " << i;
   }
 }