Moving AutofillAgent Logic into Browser

chrome/browser/autofill/autofill_external_delegate.h

Index: chrome/browser/autofill/autofill_external_delegate.h
diff --git a/chrome/browser/autofill/autofill_external_delegate.h b/chrome/browser/autofill/autofill_external_delegate.h
index edc4104867d94db644245bca4fbd12f2c5be5879..3c7dff3b65ff57b495fbbbee5aa76fe4b2d37d55 100644
--- a/chrome/browser/autofill/autofill_external_delegate.h
+++ b/chrome/browser/autofill/autofill_external_delegate.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/string16.h"
+#include "webkit/glue/form_field.h"
 
 class AutofillManager;
 class TabContentsWrapper;
@@ -19,9 +20,12 @@ class Rect;
 
 namespace webkit_glue {
 struct FormData;
-struct FormField;
 }  // namespace webkit_glue
 
+// TODO(csharp): A lot of the logic in this class is copied from autofillagent.
+// Once Autofill is moved out of WebKit this class should be the only home for
+// this logic. See http://crbug.com/51644
+
 // Delegate for external processing of Autocomplete and Autofill
 // display and selection.
 class AutofillExternalDelegate {
@@ -35,24 +39,22 @@ class AutofillExternalDelegate {
 
   // Records and associates a query_id with web form data.  Called
   // when the renderer posts an Autofill query to the browser. |bounds|
-  // is window relative.
+  // is window relative. |display_warning| tells us if we should display
+  // warnings (such as autofill is disabled, but had suggestions).

[Ilya Sherman, 2011/11/18 23:16:04]

nit: I would elaborate on this explanation for |display_warning| to include the
motivation for why it might be set to false.  The reason we have a UI for
warnings in the first place is to help explain to users why Autofill is
disabled, so that the feature doesn't just feel broken.  On the other hand,
sometimes Autofill is disabled because websites want to show their own popup in
place of ours, and it's a really poor user-experience to have two competing
popups.  To account for this, we have some simple heuristics to try to detect
when it's safe to show warnings, and when it isn't.  See http://crbug.com/64286
for a bit more backstory on this.

[csharp, 2011/11/21 14:42:14]

Done.

   virtual void OnQuery(int query_id,
                        const webkit_glue::FormData& form,
                        const webkit_glue::FormField& field,
-                       const gfx::Rect& bounds) = 0;
-
-  // Records query results.  Displays them to the user with an external
-  // Autofill popup that lives completely in the browser.  Called when
-  // an Autofill query result is available.
-  // TODO(csharp): This should contain the logic found in
-  // AutofillAgent::OnSuggestionsReturned.
-  // See http://crbug.com/51644
+                       const gfx::Rect& bounds,
+                       bool display_warning);

[Ilya Sherman, 2011/11/18 23:16:04]

nit: Let's name this |display_warning_if_disabled|, to match the member
variable.

[csharp, 2011/11/21 14:42:14]

Done.

+
+  // Records query results and correctly formats them before sending them off
+  // to be displayed.  Called when an Autofill query result is available.
   virtual void OnSuggestionsReturned(
       int query_id,
       const std::vector<string16>& autofill_values,
       const std::vector<string16>& autofill_labels,
       const std::vector<string16>& autofill_icons,
-      const std::vector<int>& autofill_unique_ids) = 0;
+      const std::vector<int>& autofill_unique_ids);
 
   // Hide the Autofill poup.
   virtual void HideAutofillPopup() = 0;
@@ -64,12 +66,45 @@ class AutofillExternalDelegate {
   static AutofillExternalDelegate* Create(TabContentsWrapper*,
                                           AutofillManager*);
 
+  // Inform the delegate that the text field editing has ended, this is
+  // used to help record the metrics of when a new popup is shown.
+  void SetDidEndTextFieldEditing();

[Ilya Sherman, 2011/11/18 23:16:04]

nit: Here as well, I would omit the "Set" in the name, since there's no member
variable "did_end_text_field_editing_" whose value is being set.

[csharp, 2011/11/21 14:42:14]

Done.

+
  protected:
   explicit AutofillExternalDelegate(TabContentsWrapper* tab_contents_wrapper);
 
+  // Displays the the Autofill results to the user with an external
+  // Autofill popup that lives completely in the browser.  The suggestions
+  // have be correctly formatted by this point.
+  virtual void ApplyAutofillSuggestions(
+      const std::vector<string16>& autofill_values,
+      const std::vector<string16>& autofill_labels,
+      const std::vector<string16>& autofill_icons,
+      const std::vector<int>& autofill_unique_ids,
+      int separator_index) = 0;
+
+  // Handle instance specific OnQueryCode.
+  virtual void OnQueryPlatformSpecific(int query_id,
+                                       const webkit_glue::FormData& form,
+                                       const webkit_glue::FormField& field) = 0;
+
  private:
   TabContentsWrapper* tab_contents_wrapper_;  // weak; owns me.
 
+  // The ID of the last request sent for form field Autofill.  Used to ignore
+  // out of date responses.
+  int autofill_query_id_;
+
+  // The current field selected by Autofill.
+  webkit_glue::FormField autofill_query_field_;
+
+  // Should we display a warning if autofill is disabled?

[Ilya Sherman, 2011/11/18 23:16:04]

nit: "autofill" -> "Autofill"

[csharp, 2011/11/21 14:42:14]

Done.

+  bool display_warning_if_disabled_;
+
+  // Have we already shown Autofill suggestions for the field the user is
+  // currently editing?  Used to keep track of state for metrics logging.
+  bool has_shown_autofill_popup_for_current_edit_;
+
   DISALLOW_COPY_AND_ASSIGN(AutofillExternalDelegate);
 };