Add Datalist Support to New Autofill UI

chrome/browser/autofill/autofill_external_delegate_unittest.cc

Index: chrome/browser/autofill/autofill_external_delegate_unittest.cc
diff --git a/chrome/browser/autofill/autofill_external_delegate_unittest.cc b/chrome/browser/autofill/autofill_external_delegate_unittest.cc
index 44e21e88b61b09cfb463041e33209a720972a540..3f11c6b1d20cfa18342ac5f7fd563d1ce6e575c4 100644
--- a/chrome/browser/autofill/autofill_external_delegate_unittest.cc
+++ b/chrome/browser/autofill/autofill_external_delegate_unittest.cc
@@ -88,6 +88,22 @@ class AutofillExternalDelegateUnitTest : public TabContentsWrapperTestHarness {
   }
 
  protected:
+  // Setup the expectation for a platform specific OnQuery call and then execute

[Ilya Sherman, 2012/06/06 22:25:45]

nit: "Setup" -> "Set up"

[csharp, 2012/06/07 14:57:43]

Done.

+  // it with the given QueryId.
+  void SetupOnQuery(int query_id) {

[Ilya Sherman, 2012/06/06 22:25:45]

nit: Perhaps name this "IssueOnQuery()" or just "OnQuery()"?

[csharp, 2012/06/07 14:57:43]

Done.

+    const FormData form;
+    FormField field;
+    field.is_focusable = true;
+    field.should_autocomplete = true;
+    const gfx::Rect bounds;
+
+    EXPECT_CALL(*external_delegate_,
+                OnQueryPlatformSpecific(query_id, form, field, bounds));

[Ilya Sherman, 2012/06/06 22:25:45]

nit: I would move this back into the TestExternalDelegateVirtualCalls() test,
since it's not likely to be relevant to other tests.  (If you move this, please
move out the "// This should call OnQueryPlatform specific." comment as well.)

[csharp, 2012/06/07 14:57:43]

I agree that its not too relevant to other tests, but I feel that moving it out
of this function will just make it a bit more complex then it needs to be. The
TestExternalDelegateVirtualCalls would need to be kept in sync with the code
here and all the other tests that used this function would see warnings messages
about OnQuery hitting the empty mock function.

[Ilya Sherman, 2012/06/07 22:56:05]

Ok, I'm convinced :)

+
+    // This should call OnQueryPlatform specific.
+    external_delegate_->OnQuery(query_id, form, field, bounds, false);
+  }
+
   scoped_refptr<MockAutofillManager> autofill_manager_;
   scoped_ptr<MockAutofillExternalDelegate> external_delegate_;
 
@@ -100,17 +116,7 @@ class AutofillExternalDelegateUnitTest : public TabContentsWrapperTestHarness {
 // Test that our external delegate called the virtual methods at the right time.
 TEST_F(AutofillExternalDelegateUnitTest, TestExternalDelegateVirtualCalls) {
   const int kQueryId = 5;

[Ilya Sherman, 2012/06/06 22:25:45]

nit: Why not define this outside the test, so that it can be shared among the
tests?

[csharp, 2012/06/07 14:57:43]

Done.

-  const FormData form;
-  FormField field;
-  field.is_focusable = true;
-  field.should_autocomplete = true;
-  const gfx::Rect bounds;
-
-  EXPECT_CALL(*external_delegate_,
-              OnQueryPlatformSpecific(kQueryId, form, field, bounds));
-
-  // This should call OnQueryPlatform specific.
-  external_delegate_->OnQuery(kQueryId, form, field, bounds, false);
+  SetupOnQuery(kQueryId);
 
   EXPECT_CALL(*external_delegate_, ApplyAutofillSuggestions(_, _, _, _));
 
@@ -137,6 +143,38 @@ TEST_F(AutofillExternalDelegateUnitTest, TestExternalDelegateVirtualCalls) {
                                                    autofill_ids[0], 0);
 }
 
+// Test that data list elements for a node will appear in the Autofill popup.
+TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegateDataList) {
+  const int kQueryId = 5;
+  SetupOnQuery(kQueryId);
+
+  std::vector<string16> data_list_items;
+  data_list_items.push_back(string16());
+  std::vector<int> data_list_ids;
+  data_list_ids.push_back(WebAutofillClient::MenuItemIDDataListEntry);
+
+  external_delegate_->SetCurrentDataListValues(data_list_items,
+                                               data_list_items,
+                                               data_list_items,
+                                               data_list_ids);
+
+  EXPECT_CALL(*external_delegate_,
+              ApplyAutofillSuggestions(
+                  _, _, _, testing::Contains(
+                      WebAutofillClient::MenuItemIDDataListEntry)));
+
+  // This should call ApplyAutofillSuggestions.
+  std::vector<string16> autofill_item;
+  autofill_item.push_back(string16());
+  std::vector<int> autofill_ids;
+  autofill_ids.push_back(1);
+  external_delegate_->OnSuggestionsReturned(kQueryId,
+                                            autofill_item,
+                                            autofill_item,
+                                            autofill_item,
+                                            autofill_ids);
+}

[Ilya Sherman, 2012/06/06 22:25:45]

There are a lot more branches in the code than this test covers.  It would be
nice to add more coverage.  For example: verify that datalist items precede
other items; verify that the popup is shown if it contains only datalist items;
verify that separators are positioned correctly depending on whether there are
datalist items and/or other suggestions; etc.

[csharp, 2012/06/07 14:57:43]

Updated some of the tests to ensure the placement of all elements are correct.
(Good advice, I did catch a small bug)

+
 // Test that the Autofill delegate doesn't try and fill a form with a
 // negative unique id.
 TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegateInvalidUniqueId) {