Show display warning if the form is autocomplete off.

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 72110d0e41d54f1471a36c24144eaf555e7287ba..d52a486c6c8ef404dacac5ca57413a64d459268c 100644
--- a/chrome/renderer/autofill/autofill_agent.cc
+++ b/chrome/renderer/autofill/autofill_agent.cc
@@ -30,6 +30,7 @@ using WebKit::WebFrame;
 using WebKit::WebInputElement;
 using WebKit::WebKeyboardEvent;
 using WebKit::WebNode;
+using WebKit::WebElement;
 using WebKit::WebString;
 using webkit_glue::FormData;
 using webkit_glue::FormDataPredictions;
@@ -40,6 +41,19 @@ namespace {
 // (so to avoid sending long strings through IPC).
 const size_t kMaximumTextSizeForAutofill = 1000;
 
+bool autoCompleteOff(const WebNode& node) {

[Ilya Sherman, 2011/07/27 23:41:30]

nit: Chromium function names start with capital letters, and boolean function
names typically start with "Is".  Hence, I would name this function something
along the lines of "IsAutocompleteDisabled()"

Alternately, you could just update the type for autofill_query_node_ to be a
WebInputElement, and skip these type checks and casts.

[honten.org, 2011/07/28 06:03:06]

Change the type.

On 2011/07/27 23:41:30, Ilya Sherman wrote:

+  if (node.isNull())
+    return false;
+
+  if (node.nodeType() != WebNode::ElementNode &&
+      node.nodeType() != WebNode::TextNode)

[Ilya Sherman, 2011/07/27 23:41:30]

I don't think you want TextNode here...

+    return false;
+
+  const WebElement element = node.toConst<WebElement>();
+  const WebInputElement* input_element = toWebInputElement(&element);
+  return input_element && !input_element->autoComplete();
+}
+
 }  // namespace
 
 namespace autofill {
@@ -234,7 +248,14 @@ void AutofillAgent::OnSuggestionsReturned(int query_id,
   std::vector<int> ids(unique_ids);
   int separator_index = -1;
 
-  if (ids[0] < 0 && ids.size() > 1) {
+  if (autoCompleteOff(autofill_query_node_)) {
+    // If autofill is disabled and we had suggestions, show a warning instead.
+    v.assign(1,
+             l10n_util::GetStringUTF16(IDS_AUTOFILL_WARNING_FORM_DISABLED));

[Ilya Sherman, 2011/07/27 23:41:30]

nit: This looks like it could fit on a single line.

[honten.org, 2011/07/28 06:03:06]

Done.

+    l.assign(1, string16());
+    i.assign(1, string16());
+    ids.assign(1, -1);
+  } 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.
     v.erase(v.begin());
@@ -320,7 +341,9 @@ void AutofillAgent::ShowSuggestions(const WebInputElement& element,
                                     bool autofill_on_empty_values,
                                     bool requires_caret_at_end,
                                     bool display_warning_if_disabled) {
-  if (!element.isEnabled() || element.isReadOnly() || !element.autoComplete() ||
+  const WebFormElement form = element.form();
+  if (!element.isEnabled() || element.isReadOnly() ||
+      (!element.autoComplete() && form.autoComplete()) ||

[Ilya Sherman, 2011/07/27 23:41:30]

nit: This line is rather confusing at a glance, and could use an explanatory
comment.

[honten.org, 2011/07/28 00:04:50]

Should I use extra bool flag like this?

// We should not show warning message for disabled autocomple when
// field autocomplete is off and form autocomplete is on.
bool disabled_autocomplete_off_warning = !element.autoComplete() 
    && form.autocomplete();

Or just adding comment is fine?

On 2011/07/27 23:41:30, Ilya Sherman wrote:

[Ilya Sherman, 2011/07/28 00:29:33]

This comment only describes part of the intent.  The full intent is better
captured by something along the lines of "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."
Just adding a comment is fine, though you're welcome to break this logic out
into a boolean variable if you think that is more readable.  If you prefer to
add a variable, please try to find a clearer name for it. 
"disabled_autocomplete_off_warning" reads like a noun (with modifiers), whereas
boolean variables -- like boolean functions -- should typically read like full
sentences in the form of a question.

[honten.org, 2011/07/28 06:03:06]

Just added comment.

On 2011/07/28 00:29:33, Ilya Sherman wrote:

       !element.isTextField() || element.isPasswordField() ||
       !element.suggestedValue().isEmpty())
     return;