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

components/autofill/core/browser/autofill_external_delegate.cc

 // Copyright 2013 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 "components/autofill/core/browser/autofill_external_delegate.h"
 
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/autocomplete_history_manager.h"
 #include "components/autofill/core/browser/autofill_driver.h"
 #include "components/autofill/core/browser/autofill_manager.h"
@@ skipping 73 matching lines @@
     labels.pop_back();
     icons.pop_back();
     ids.pop_back();
   }
 
   // If anything else is added to modify the values after inserting the data
   // list, AutofillPopupControllerImpl::UpdateDataListValues will need to be
   // 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();
     return;
   }
 
   // Send to display.
   if (query_field_.is_focusable) {
     manager_->delegate()->ShowAutofillPopup(
         element_bounds_,
@@ skipping 46 matching lines @@
   } else if (identifier == POPUP_ITEM_ID_CLEAR_FORM) {
     // User selected 'Clear form'.
     driver_->RendererShouldClearFilledForm();
   } else if (identifier == POPUP_ITEM_ID_PASSWORD_ENTRY) {
     NOTREACHED();  // Should be handled elsewhere.
   } else if (identifier == POPUP_ITEM_ID_DATALIST_ENTRY) {
     driver_->RendererShouldAcceptDataListSuggestion(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);
   }
 
   manager_->delegate()->HideAutofillPopup();
 }
 
 void AutofillExternalDelegate::RemoveSuggestion(const base::string16& value,
                                                 int identifier) {
   if (identifier > 0)
@@ skipping 132 matching lines @@
   // Set the values that all datalist elements share.
   icons->insert(icons->begin(),
                 data_list_values_.size(),
                 base::string16());
   unique_ids->insert(unique_ids->begin(),
                      data_list_values_.size(),
                      POPUP_ITEM_ID_DATALIST_ENTRY);
 }
 
 }  // namespace autofill