Add Datalist Support to New Autofill UI

chrome/browser/autofill/autofill_external_delegate_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 <vector>
 
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/string16.h"
 #include "chrome/browser/autofill/autofill_manager.h"
@@ skipping 70 matching lines @@
 
   virtual void SetUp() OVERRIDE {
     TabContentsWrapperTestHarness::SetUp();
     autofill_manager_ = new MockAutofillManager(contents_wrapper());
     external_delegate_.reset(new MockAutofillExternalDelegate(
         contents_wrapper(),
         autofill_manager_));
   }
 
  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_;
 
  private:
   content::TestBrowserThread ui_thread_;
 
   DISALLOW_COPY_AND_ASSIGN(AutofillExternalDelegateUnitTest);
 };
 
 // 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(_, _, _, _));
 
   // 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);
 
 
   EXPECT_CALL(*external_delegate_, HideAutofillPopup());
 
   // Called by DidAutofillSuggestions, add expectation to remove warning.
   EXPECT_CALL(*autofill_manager_, OnFillAutofillFormData(_, _, _, _));
 
   // This should trigger a call to hide the popup since
   // we've selected an option.
   external_delegate_->DidAcceptAutofillSuggestions(autofill_item[0],
                                                    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) {
   // Ensure it doesn't try to preview the negative id.
   EXPECT_CALL(*autofill_manager_, OnFillAutofillFormData(_, _, _, _)).Times(0);
   external_delegate_->SelectAutofillSuggestionAtIndex(-1);
 
   // Ensure it doesn't try to fill the form in with the negative id.
   EXPECT_CALL(*autofill_manager_, OnFillAutofillFormData(_, _, _, _)).Times(0);
   external_delegate_->DidAcceptAutofillSuggestions(string16(), -1, 0);
@@ skipping 16 matching lines @@
   external_delegate_->SelectAutofillSuggestionAtIndex(1);
 }
 
 // Test that the popup is hidden once we are done editing the autofill field.
 TEST_F(AutofillExternalDelegateUnitTest,
        ExternalDelegateHidePopupAfterEditing) {
   EXPECT_CALL(*external_delegate_, HideAutofillPopup());
 
   external_delegate_->DidEndTextFieldEditing();
 }