Add Delete Support to New Autofill UI

chrome/browser/autofill/autofill_popup_unittest.cc

Index: chrome/browser/autofill/autofill_popup_unittest.cc
diff --git a/chrome/browser/autofill/autofill_popup_unittest.cc b/chrome/browser/autofill/autofill_popup_unittest.cc
index 4332cd63e19ccbc24f92605a42b4d217946619ed..fcea1b3cef1426511425779f79941b31150ad587 100644
--- a/chrome/browser/autofill/autofill_popup_unittest.cc
+++ b/chrome/browser/autofill/autofill_popup_unittest.cc
@@ -8,8 +8,13 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
+using ::testing::_;
+using ::testing::AtLeast;
+
 namespace {
 
+const int kNoSelection = -1;
+
 class MockAutofillExternalDelegate : public TestAutofillExternalDelegate {
  public:
   MockAutofillExternalDelegate() : TestAutofillExternalDelegate(NULL, NULL) {};
@@ -17,21 +22,19 @@ class MockAutofillExternalDelegate : public TestAutofillExternalDelegate {
 
   virtual void SelectAutofillSuggestionAtIndex(int unique_id, int list_index)
       OVERRIDE {}
+
+  virtual void ClearPreviewedForm() OVERRIDE {}
 };
 
 class TestAutofillPopupView : public AutofillPopupView {
  public:
   explicit TestAutofillPopupView(AutofillExternalDelegate* external_delegate) :
       AutofillPopupView(NULL, external_delegate) {
-    std::vector<string16> autofill_values;
-    autofill_values.push_back(string16());
-    autofill_values.push_back(string16());
+    std::vector<string16> autofill_values(2, string16());
 
-    std::vector<int> autofill_ids;
-    autofill_ids.push_back(0);
-    autofill_ids.push_back(1);
+    std::vector<int> autofill_ids(2, 0);
 
-    Show(autofill_values, autofill_values, autofill_values, autofill_ids, 0);
+    Show(autofill_values, autofill_values, autofill_values, autofill_ids, 1);

[Ilya Sherman, 2012/04/17 08:31:20]

Hmm, it's not great that there is this long-distance dependency between the
constructor and the tests that expect these particular values to be shown.  It
would be better to do this set up in each test separately (it's not a whole lot
of code), so that the tests are more independent.

[csharp, 2012/04/18 15:35:58]

Done.

   }
   virtual ~TestAutofillPopupView() {}
 
@@ -51,12 +54,15 @@ class TestAutofillPopupView : public AutofillPopupView {
   void SelectPreviousLine() {
     AutofillPopupView::SelectPreviousLine();
   }
+  bool RemoveSelectedLine() {
+    return AutofillPopupView::RemoveSelectedLine();
+  }
 
   MOCK_METHOD1(InvalidateRow, void(size_t));
+  MOCK_METHOD0(HideInternal, void());
 
  private:
   virtual void ShowInternal() OVERRIDE {}
-  virtual void HideInternal() OVERRIDE {}
 };
 
 }  // namespace
@@ -107,3 +113,23 @@ TEST_F(AutofillPopupViewUnitTest, RedrawSelectedLine) {
   EXPECT_CALL(*autofill_popup_view_, InvalidateRow(selected_line));
   autofill_popup_view_->SetSelectedLine(-1);
 }
+
+TEST_F(AutofillPopupViewUnitTest, RemoveLine) {
+  // To remove warnings.
+  EXPECT_CALL(*autofill_popup_view_, InvalidateRow(_)).Times(AtLeast(0));
+
+  // No line is selected so the removal should fail.
+  EXPECT_FALSE(autofill_popup_view_->RemoveSelectedLine());
+
+  // Try to remove the last entry and ensure it fails (it is an option).
+  autofill_popup_view_->SetSelectedLine(
+      autofill_popup_view_->autofill_values().size() - 1);
+  EXPECT_FALSE(autofill_popup_view_->RemoveSelectedLine());
+  EXPECT_NE(kNoSelection, autofill_popup_view_->selected_line());

[Ilya Sherman, 2012/04/17 08:31:20]

nit: Perhaps EXPECT_GE(0, ...) so that kNoSelection is not part of the public
contract for the class?  Or, if you prefer to use the constant in the test,
please declare it in a place that both the test code and the regular
implementation can access.

[csharp, 2012/04/18 15:35:58]

Replaced with EXPECT_LE(0, 

[Ilya Sherman, 2012/04/18 18:12:51]

Sorry, I meant EXPECT_GE(..., 0) -- for the EXPECT_LE-type macros, the more
constant-y number typically goes on the right.

+
+  // Remove the first (and only) entry. The popup should then be hidden since
+  // there are no Autofill entries left.
+  EXPECT_CALL(*autofill_popup_view_, HideInternal());
+  autofill_popup_view_->SetSelectedLine(0);
+  EXPECT_TRUE(autofill_popup_view_->RemoveSelectedLine());
+}