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..7425239a8ee5318dbaf5a1b02d32e5672cd36625 100644
--- a/components/autofill/core/browser/autofill_external_delegate.cc
+++ b/components/autofill/core/browser/autofill_external_delegate.cc
@@ -14,15 +14,17 @@
 
 namespace autofill {
 
-AutofillExternalDelegate::AutofillExternalDelegate(
-    AutofillManager* manager,
-    AutofillDriver* driver)
+AutofillExternalDelegate::AutofillExternalDelegate(AutofillManager* manager,
+                                                   AutofillDriver* driver)
     : manager_(manager),
       driver_(driver),
       query_id_(0),
       display_warning_if_disabled_(false),
       has_suggestion_(false),
       has_shown_popup_for_current_edit_(false),
+#if defined(OS_MACOSX)
+      redisplay_popop_on_focus_lost_(false),
+#endif
       weak_ptr_factory_(this) {
   DCHECK(manager);
 }
@@ -91,6 +93,18 @@ void AutofillExternalDelegate::OnSuggestionsReturned(
   // updated to match.
   InsertDataListValues(&values, &labels, &icons, &ids);
 
+#if defined(OS_MACOSX)
+  if (values.empty() &&
+      manager_->ShouldShowAccessAddressBookSuggestion(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);
+  }
+#endif
+
   if (values.empty()) {
     // No suggestions, any popup currently showing is obsolete.
     manager_->delegate()->HideAutofillPopup();
@@ -157,6 +171,26 @@ 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 Chrome access to user's address book.
+    bool modal_presented = manager_->AccessAddressBook();
+
+    // If a modal view was not presented, immediately refresh the popup.
+    if (!modal_presented) {
+      manager_->delegate()->HideAutofillPopup();
+      ReissueQuery();
+      return;
+    }
+
+    // A blocking modal was presented. When the user responds, the text field
+    // loses focus. At that point, the user may have granted permission to their
+    // address book. Immediately redisplay the popup, which may contain new
+    // suggestions from the address book.
+    redisplay_popop_on_focus_lost_ = true;

[Ilya Sherman, 2014/05/23 15:47:10]

Hmm, this still seems not really correct.  Presumably focus is lost as soon as
the modal dialog is displayed, right?  In that case, wouldn't we be reissuing
the query too soon?  I think instead it makes sense to just always post a task
to redisplay the popup.  That way, if the modal dialog is blocking the UI
thread, the task will wait.  If it's not, it'll fire immediately.  Does that
sound sensible?

[erikchen, 2014/05/23 17:19:38]

Focus is not lost as soon as the modal dialog is displayed. I've updated the
comment to more accurately describe the behavior of Cocoa. (There's an NSEvent
queued in the NSRunLoop which is going to cause the popup to lose focus. The
current logic redisplays the popup at the earliest possible point)

[Ilya Sherman, 2014/05/28 06:43:27]

Hmm, this still seems like more complexity than we need to handle this case. 
I'm also worried that it might be a little brittle -- what if the Cocoa behavior
changes in a subsequent OS release?

Did you consider my suggestion of posting a task back to the current message
loop?  That seems like an easier way to always do the right thing, assuming that
it works correctly.

+#else
+    NOTREACHED();
+#endif
   } else {
     FillAutofillFormData(identifier, false);
   }
@@ -176,6 +210,18 @@ void AutofillExternalDelegate::DidEndTextFieldEditing() {
   manager_->delegate()->HideAutofillPopup();
 
   has_shown_popup_for_current_edit_ = false;
+
+#if defined(OS_MACOSX)
+  // The user was presented a modal dialog, which they had to interact with
+  // before the Chrome process could resume. Their interaction is guaranteed to
+  // cause the text field to lose focus. They were either granting or denying
+  // Chrome access to their address book, so immediately try to redisplay the
+  // autofill popup.
+  if (redisplay_popop_on_focus_lost_) {
+    ReissueQuery();
+    redisplay_popop_on_focus_lost_ = false;
+  }
+#endif
 }
 
 void AutofillExternalDelegate::ClearPreviewedForm() {
@@ -308,4 +354,14 @@ void AutofillExternalDelegate::InsertDataListValues(
                      POPUP_ITEM_ID_DATALIST_ENTRY);
 }
 
+#if defined(OS_MACOSX)
+void AutofillExternalDelegate::ReissueQuery() {
+  manager_->OnQueryFormFieldAutofill(query_id_,
+                                     query_form_,
+                                     query_field_,
+                                     element_bounds_,
+                                     display_warning_if_disabled_);
+}
+#endif
+
 }  // namespace autofill