DataList UI (Chromium part)

chrome/renderer/autofill/autofill_agent.cc

Index: chrome/renderer/autofill/autofill_agent.cc
diff --git a/chrome/renderer/autofill/autofill_agent.cc b/chrome/renderer/autofill/autofill_agent.cc
index 294ba19b33f091f1dc731a7d51979b523c3ebd65..5a2c326b2435e3a142eb20c799136a28c9d85431 100644
--- a/chrome/renderer/autofill/autofill_agent.cc
+++ b/chrome/renderer/autofill/autofill_agent.cc
@@ -15,13 +15,16 @@
 #include "content/public/renderer/render_view.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebRect.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebDataListElement.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebDocument.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFormControlElement.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebInputEvent.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebNode.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebNodeCollection.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebOptionElement.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebView.h"
-#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebRect.h"
 #include "ui/base/keycodes/keyboard_codes.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "webkit/forms/form_data.h"
@@ -29,12 +32,15 @@
 #include "webkit/forms/form_field.h"
 #include "webkit/forms/password_form.h"
 
+using WebKit::WebDataListElement;
 using WebKit::WebFormControlElement;
 using WebKit::WebFormElement;
 using WebKit::WebFrame;
 using WebKit::WebInputElement;
 using WebKit::WebKeyboardEvent;
 using WebKit::WebNode;
+using WebKit::WebNodeCollection;
+using WebKit::WebOptionElement;
 using WebKit::WebString;
 using webkit::forms::FormData;
 using webkit::forms::FormDataPredictions;
@@ -58,8 +64,6 @@ AutofillAgent::AutofillAgent(
       autofill_action_(AUTOFILL_NONE),
       display_warning_if_disabled_(false),
       was_query_node_autofilled_(false),
-      suggestions_clear_index_(-1),
-      suggestions_options_index_(-1),
       has_shown_autofill_popup_for_current_edit_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   render_view->GetWebView()->setAutofillClient(this);
@@ -146,44 +150,51 @@ bool AutofillAgent::InputElementLostFocus() {
 void AutofillAgent::didAcceptAutofillSuggestion(const WebNode& node,
                                                 const WebString& value,
                                                 const WebString& label,
-                                                int unique_id,
+                                                int item_id,
                                                 unsigned index) {
   if (password_autofill_manager_->DidAcceptAutofillSuggestion(node, value))
     return;
 
   DCHECK(node == autofill_query_element_);
 
-  if (suggestions_options_index_ != -1 &&
-      index == static_cast<unsigned>(suggestions_options_index_)) {
-    // User selected 'Autofill Options'.

[Ilya Sherman, 2012/04/11 00:18:04]

nit: Please preserve these comments.  I admit that they're fairly clear from the
code as written, but they still add some value in that they map to the user
actions that cause us to reach the code.

[keishi, 2012/04/11 11:48:08]

Done.

-    Send(new AutofillHostMsg_ShowAutofillDialog(routing_id()));
-  } else if (suggestions_clear_index_ != -1 &&
-             index == static_cast<unsigned>(suggestions_clear_index_)) {
-    // User selected 'Clear form'.
-    form_cache_.ClearFormWithElement(autofill_query_element_);
-  } else if (!unique_id) {
-    // User selected an Autocomplete entry, so we fill directly.
-    WebInputElement element = node.toConst<WebInputElement>();
-    SetNodeText(value, &element);
-  } else {
-    // Fill the values for the whole form.
-    FillAutofillFormData(node, unique_id, AUTOFILL_FILL);
+  switch (item_id) {
+    case WebKit::WarningMessageMenuItemID:

[Ilya Sherman, 2012/04/11 00:18:04]

If we are going to be relying on all warnings being assigned this constant id,
please also update the code in chrome/browser/autofill/ (in particular,
autofill_manager.cc and autofill_external_delegate.cc) to use this named
constant.

[keishi, 2012/04/11 11:48:08]

Done.

+    case WebKit::SeparatorMenuItemID: {
+      NOTREACHED();
+      break;
+    }

[Ilya Sherman, 2012/04/11 00:18:04]

nit: No need for curly braces.

[keishi, 2012/04/11 11:48:08]

Done.

+    case WebKit::AutofillOptionsMenuItemID: {
+      Send(new AutofillHostMsg_ShowAutofillDialog(routing_id()));
+      break;
+    } 
+    case WebKit::ClearFormMenuItemID: {
+      form_cache_.ClearFormWithElement(autofill_query_element_);
+      break;
+    }
+    case WebKit::AutocompleteEntryMenuItemID:
+    case WebKit::PasswordEntryMenuItemID:
+    case WebKit::DataListEntryMenuItemID: {
+      WebInputElement element = node.toConst<WebInputElement>();

[Ilya Sherman, 2012/04/11 00:18:04]

nit: Perhaps we can skip this cast by using |autofill_query_element_|?  Also, it
seems rather bad that the constness of |node| is completely ignored... we should
probably update the method signature not to take |node| as a const value.

[keishi, 2012/04/11 11:48:08]

Done.

+      SetNodeText(value, &element);
+      break;
+    }
+    default: {
+      // Fill the values for the whole form.

[Ilya Sherman, 2012/04/11 00:18:04]

nit: It's probably a good idea to DCHECK_GT(item_id, 0) and mention in a comment
that positive ids are unique ids for Autofill (vs. Autocomplete) suggestions.

[keishi, 2012/04/11 11:48:08]

Done.

+      FillAutofillFormData(node, item_id, AUTOFILL_FILL);
+    }
   }
-
-  suggestions_clear_index_ = -1;
-  suggestions_options_index_ = -1;
 }
 
 void AutofillAgent::didSelectAutofillSuggestion(const WebNode& node,
                                                 const WebString& value,
                                                 const WebString& label,
-                                                int unique_id) {
-  DCHECK_GE(unique_id, 0);
+                                                int item_id) {
+  DCHECK_GE(item_id, 0);

[Ilya Sherman, 2012/04/11 00:18:04]

Is this still valid?  Which sorts of items are selectable?  Presumably at least
password items -- with id "-2" -- should be, given the if-stmt immediately
below.

[keishi, 2012/04/11 11:48:08]

No, it was wrong. Selecting a non autofill entry should clear the autofill
preview.

   if (password_autofill_manager_->DidSelectAutofillSuggestion(node))
     return;
 
   didClearAutofillSelection(node);
-  FillAutofillFormData(node, unique_id, AUTOFILL_PREVIEW);
+  FillAutofillFormData(node, item_id, AUTOFILL_PREVIEW);
 }
 
 void AutofillAgent::didClearAutofillSelection(const WebNode& node) {
@@ -205,12 +216,6 @@ void AutofillAgent::didClearAutofillSelection(const WebNode& node) {
 
 void AutofillAgent::removeAutocompleteSuggestion(const WebString& name,
                                                  const WebString& value) {
-  // The index of clear & options will have shifted down.
-  if (suggestions_clear_index_ != -1)
-    suggestions_clear_index_--;
-  if (suggestions_options_index_ != -1)
-    suggestions_options_index_--;
-
   Send(new AutofillHostMsg_RemoveAutocompleteEntry(routing_id(), name, value));
 }
 
@@ -278,7 +283,6 @@ void AutofillAgent::OnSuggestionsReturned(int query_id,
   std::vector<string16> l(labels);
   std::vector<string16> i(icons);
   std::vector<int> ids(unique_ids);
-  int separator_index = -1;
 
   DCHECK_GT(ids.size(), 0U);
   if (!autofill_query_element_.isNull() &&
@@ -287,7 +291,7 @@ void AutofillAgent::OnSuggestionsReturned(int query_id,
     v.assign(1, l10n_util::GetStringUTF16(IDS_AUTOFILL_WARNING_FORM_DISABLED));
     l.assign(1, string16());
     i.assign(1, string16());
-    ids.assign(1, -1);
+    ids.assign(1, WebKit::WarningMessageMenuItemID);
   } else if (ids[0] < 0 && ids.size() > 1) {
     // If we received a warning instead of suggestions from autofill but regular
     // suggestions from autocomplete, don't show the autofill warning.
@@ -318,9 +322,12 @@ void AutofillAgent::OnSuggestionsReturned(int query_id,
     v.push_back(l10n_util::GetStringUTF16(IDS_AUTOFILL_CLEAR_FORM_MENU_ITEM));
     l.push_back(string16());
     i.push_back(string16());
-    ids.push_back(0);
-    suggestions_clear_index_ = v.size() - 1;
-    separator_index = v.size() - 1;
+    ids.push_back(WebKit::ClearFormMenuItemID);
+
+    v.push_back(string16());
+    l.push_back(string16());
+    i.push_back(string16());
+    ids.push_back(WebKit::SeparatorMenuItemID);

[Ilya Sherman, 2012/04/11 00:18:04]

This was not actually showing up as a separator in the mac build [1] you linked
to.  Was that build compiled before you added this, or is there something going
wrong here?
[1] https://docs.google.com/open?id=0BwRi59l_ri74a1N4d2NSYXlIdzg

[keishi, 2012/04/11 11:48:08]

The separator should come before the menu item. Fixed.

   }
 
   if (has_autofill_item) {
@@ -328,16 +335,18 @@ void AutofillAgent::OnSuggestionsReturned(int query_id,
     v.push_back(l10n_util::GetStringUTF16(IDS_AUTOFILL_OPTIONS_POPUP));
     l.push_back(string16());
     i.push_back(string16());
-    ids.push_back(0);
-    suggestions_options_index_ = v.size() - 1;
-    separator_index = values.size();
+    ids.push_back(WebKit::AutofillOptionsMenuItemID);
+
+    v.push_back(string16());
+    l.push_back(string16());
+    i.push_back(string16());
+    ids.push_back(WebKit::SeparatorMenuItemID);

[Ilya Sherman, 2012/04/11 00:18:04]

This can add two separators where we should have only one: the "Clear form" and
"Autofill options" items should have only one separator above them (and no
separators between them).

[keishi, 2012/04/11 11:48:08]

Done.

   }
 
   // Send to WebKit for display.
   if (!v.empty() && !autofill_query_element_.isNull() &&
       autofill_query_element_.isFocusable()) {
-    web_view->applyAutofillSuggestions(
-        autofill_query_element_, v, l, i, ids, separator_index);
+    CombineDataListEntriesAndShow(autofill_query_element_, v, l, i, ids);
   }
 
   Send(new AutofillHostMsg_DidShowAutofillSuggestions(
@@ -346,6 +355,49 @@ void AutofillAgent::OnSuggestionsReturned(int query_id,
   has_shown_autofill_popup_for_current_edit_ |= has_autofill_item;
 }
 
+void AutofillAgent::CombineDataListEntriesAndShow(const WebKit::WebInputElement& element,
+                                      const std::vector<string16>& values,
+                                      const std::vector<string16>& labels,
+                                      const std::vector<string16>& icons,
+                                      const std::vector<int>& item_ids) {

[Ilya Sherman, 2012/04/11 00:18:04]

nit: 80-col, wrapping

[keishi, 2012/04/11 11:48:08]

Done.

+  std::vector<string16> v;
+  std::vector<string16> l;
+  std::vector<string16> i;
+  std::vector<int> ids;
+                                        
+  WebDataListElement dataList = element.dataList();
+  if (!dataList.isNull()) {
+    WebNodeCollection options = dataList.options();
+    if (options.length() > 0) {

[Ilya Sherman, 2012/04/11 00:18:04]

nit: Is this check needed, or would it also be covered by the "!option.isNull()"
check in the while loop?

[keishi, 2012/04/11 11:48:08]

True! Removed.

+      WebOptionElement option = options.firstItem().to<WebOptionElement>();
+      while (!option.isNull()) {
+        v.push_back(option.value());
+        if (option.value() != option.label())
+          l.push_back(option.label());
+        else
+          l.push_back(string16());
+        i.push_back(string16());
+        ids.push_back(WebKit::DataListEntryMenuItemID);
+        option = options.nextItem().to<WebOptionElement>();
+      }
+    }
+  }

[Ilya Sherman, 2012/04/11 00:18:04]

nit: This code has a lot of levels of nesting.  That could be reduced by
decomposing this to a helper function with early returns.

[keishi, 2012/04/11 11:48:08]

Done.

+
+  if (v.size() > 0 && values.size() > 0) {

[Ilya Sherman, 2012/04/11 00:18:04]

nit: Please add a comment above this if-stmt -- something like "If there are
both <datalist> items and Autofill suggestions, add a separator between them."

[keishi, 2012/04/11 11:48:08]

Done.

+    v.push_back(string16());
+    l.push_back(string16());
+    i.push_back(string16());
+    ids.push_back(WebKit::SeparatorMenuItemID);
+  }
+
+  v.insert(v.end(), values.begin(), values.end());
+  l.insert(l.end(), labels.begin(), labels.end());
+  i.insert(i.end(), icons.begin(), icons.end());
+  ids.insert(ids.end(), item_ids.begin(), item_ids.end());
+  
+  render_view()->GetWebView()->applyAutofillSuggestions(element, v, l, i, ids);
+}
+
 void AutofillAgent::OnFormDataFilled(int query_id,
                                      const webkit::forms::FormData& form) {
   if (!render_view()->GetWebView() || query_id != autofill_query_id_)
@@ -418,20 +470,7 @@ void AutofillAgent::ShowSuggestions(const WebInputElement& element,
                                     bool autofill_on_empty_values,
                                     bool requires_caret_at_end,
                                     bool display_warning_if_disabled) {
-  // If autocomplete is disabled at the form level, then we might want to show
-  // a warning in place of suggestions. However, if autocomplete is disabled
-  // specifically for this field, we never want to show a warning. Otherwise,
-  // we might interfere with custom popups (e.g. search suggestions) used by
-  // the website.
-  const WebFormElement form = element.form();
-  if (!element.isEnabled() || element.isReadOnly() ||
-      (!element.autoComplete() && (form.isNull() || form.autoComplete())) ||
-      !element.isTextField() || element.isPasswordField() ||
-      !element.suggestedValue().isEmpty())
-    return;
-
-  // If the field has no name, then we won't have values.
-  if (element.nameForAutofill().isEmpty())
+  if (!element.isEnabled() || element.isReadOnly() || !element.isTextField() || element.isPasswordField() || !element.suggestedValue().isEmpty())

[Ilya Sherman, 2012/04/11 00:18:04]

nit: 80-col

[keishi, 2012/04/11 11:48:08]

Done.

     return;
 
   // Don't attempt to autofill with values that are too large or if filling
@@ -449,6 +488,19 @@ void AutofillAgent::ShowSuggestions(const WebInputElement& element,
 
     return;
   }
+  
+  // If autocomplete is disabled at the form level, then we might want to show
+  // a warning in place of suggestions. However, if autocomplete is disabled
+  // specifically for this field, we never want to show a warning. Otherwise,
+  // we might interfere with custom popups (e.g. search suggestions) used by
+  // the website.
+  const WebFormElement form = element.form();
+  if ((!element.autoComplete() && (form.isNull() || form.autoComplete())) ||
+      // If the field has no name, then we won't have values.

[Ilya Sherman, 2012/04/11 00:18:04]

nit: Please move this comment to be above the if-stmt.

[keishi, 2012/04/11 11:48:08]

Done.

+      element.nameForAutofill().isEmpty()) {
+    CombineDataListEntriesAndShow(element, std::vector<string16>(), std::vector<string16>(), std::vector<string16>(), std::vector<int>());

[Ilya Sherman, 2012/04/11 00:18:04]

nit: 80-col.

[keishi, 2012/04/11 11:48:08]

Done.

+    return;
+  }
 
   QueryAutofillSuggestions(element, display_warning_if_disabled);
 }
@@ -480,7 +532,7 @@ void AutofillAgent::QueryAutofillSuggestions(const WebInputElement& element,
 }
 
 void AutofillAgent::FillAutofillFormData(const WebNode& node,
-                                         int unique_id,
+                                         int item_id,
                                          AutofillAction action) {
   static int query_counter = 0;
   autofill_query_id_ = query_counter++;
@@ -494,7 +546,7 @@ void AutofillAgent::FillAutofillFormData(const WebNode& node,
 
   autofill_action_ = action;
   Send(new AutofillHostMsg_FillAutofillFormData(
-      routing_id(), autofill_query_id_, form, field, unique_id));
+      routing_id(), autofill_query_id_, form, field, item_id));
 }
 
 void AutofillAgent::SetNodeText(const string16& value,