Mac: Autofill should not immediately request access to address book.

components/autofill/core/browser/autofill_external_delegate.cc

Index: components/autofill/core/browser/autofill_external_delegate.cc
diff --git a/components/autofill/core/browser/autofill_external_delegate.cc b/components/autofill/core/browser/autofill_external_delegate.cc
index 95cf8a14a1aff8ea0f06f75969d6685ee4919d12..cab1e81a8a25b95893ec2d1a79a925c9d2a67920 100644
--- a/components/autofill/core/browser/autofill_external_delegate.cc
+++ b/components/autofill/core/browser/autofill_external_delegate.cc
@@ -91,6 +91,17 @@ void AutofillExternalDelegate::OnSuggestionsReturned(
   // updated to match.
   InsertDataListValues(&values, &labels, &icons, &ids);
 
+#if defined(OS_MACOSX)
+  if (values.empty() && !manager_->HasPromptedForAccessToAddressBook() &&
+      manager_->FieldSupportsAddressBookAutofill(query_form_, query_field_)) {
+    values.push_back(
+        l10n_util::GetStringUTF16(IDS_AUTOFILL_ACCESS_MAC_CONTACTS));
+    labels.push_back(base::string16());
+    icons.push_back(base::string16());
+    ids.push_back(POPUP_ITEM_ID_MAC_ACCESS_CONTACTS);
+  }

[Ilya Sherman, 2014/05/21 11:29:15]

Sorry, I think I gave you bad advice earlier about handling this logic within
the external delegate.  I think most of this should instead be part of
PersonalDataManager::GetProfileSuggestions().  That will allow you to avoid
changing AutofillManager, plus will reduce a bit of code duplication that you'd
otherwise need (for finding the cached form and field).

However, I think there will still be a bit of cleanup work that the external
delegate code will need to do: It's possible that there aren't any Autofill
profile suggestions returned from the PersonalDataManager, but that there are
autocomplete suggestions.  Assuming that we don't want to show this popup entry
if there are autocomplete suggestions, you'll need to do some similar cleanup to
what's done in ApplyAutofillWarnings().

[erikchen, 2014/05/21 22:00:54]

I don't think that your suggestion will work as you intended.
PersonalDataManager::GetProfileSuggestions() is only called by AutofillManager
if personal_data_.GetProfiles() is not empty (as well as some other
conditionals), so the internal logic of AutofillManager would have to be
reworked, which I'd like to avoid.

It's also poor encapsulation for personal_data_manager to sometimes add an
autofill suggestion, which is then sometimes removed by
autofill_external_delegate. I'd prefer for all the business logic to live in
personal_data_manager, and the only consumer to be autofill_external_delegate.

I've updated this CL to do exactly that. All of the logic now lives in
personal_data_manager, and this class is the only other consumer of that logic.

+#endif
+
   if (values.empty()) {
     // No suggestions, any popup currently showing is obsolete.
     manager_->delegate()->HideAutofillPopup();
@@ -157,6 +168,13 @@ void AutofillExternalDelegate::DidAcceptSuggestion(const base::string16& value,
   } else if (identifier == POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY) {
     // User selected an Autocomplete, so we fill directly.
     driver_->RendererShouldFillFieldWithValue(value);
+  } else if (identifier == POPUP_ITEM_ID_MAC_ACCESS_CONTACTS) {
+#if defined(OS_MACOSX)
+    // User wants to give Chromium access to user's address book.
+    manager_->AccessAddressBook();

[Ilya Sherman, 2014/05/21 11:29:15]

This should also re-issue the Autofill query, since the whole point of clicking
on this UI element is to potentially surface Autofill entries from the
Contacts/AddressBook.

[erikchen, 2014/05/21 22:00:54]

Good point. I've updated the logic to do this.

+#else
+    NOTREACHED();
+#endif
   } else {
     FillAutofillFormData(identifier, false);
   }