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..16b5abf51c443e5df1afc3306fd67bdd73695763 100644
--- a/components/autofill/core/browser/autofill_external_delegate.cc
+++ b/components/autofill/core/browser/autofill_external_delegate.cc
@@ -4,6 +4,7 @@
 
 #include "components/autofill/core/browser/autofill_external_delegate.h"
 
+#include "base/message_loop/message_loop.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"
@@ -14,9 +15,8 @@
 
 namespace autofill {
 
-AutofillExternalDelegate::AutofillExternalDelegate(
-    AutofillManager* manager,
-    AutofillDriver* driver)
+AutofillExternalDelegate::AutofillExternalDelegate(AutofillManager* manager,
+                                                   AutofillDriver* driver)
     : manager_(manager),
       driver_(driver),
       query_id_(0),
@@ -91,6 +91,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 +169,25 @@ 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.
+    manager_->AccessAddressBook();
+
+    // A blocking modal was presented, and the user has already responded to the

[Ilya Sherman, 2014/05/29 01:28:24]

nit: The blocking modal might not have been presented, right?  Probably worth
clarifying that a bit in the comment.

[erikchen, 2014/05/29 20:31:31]

Done.

+    // modal. The user's response added an NSEvent to the NSRunLoop. When the
+    // NSEvent is processed, it will be sent to the renderer, which will send
+    // back an IPC to cause the text field to lose focus, which will dismiss the

[Ilya Sherman, 2014/05/29 01:28:24]

nit: Technically, the text field loses focus on the renderer side rather than on
the browser side, right?

[erikchen, 2014/05/29 20:31:31]

Done. I've rewritten the comment block to add clarity and detail.

+    // autofill popup. We queue a task which will ping the renderer. When the
+    // renderer returns an IPC acknowledging the ping, FIFO processing of IPCs
+    // ensures that all side effects the blocking modal will have been

[Ilya Sherman, 2014/05/29 01:28:24]

nit: "all side effects the blocking modal" -> "all side effects of the blocking
modal"

[erikchen, 2014/05/29 20:31:31]

Done.

+    // processed. At that time, redisplay the popup.
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&AutofillExternalDelegate::PingRenderer, GetWeakPtr()));
+#else
+    NOTREACHED();
+#endif
   } else {
     FillAutofillFormData(identifier, false);
   }
@@ -186,6 +217,10 @@ void AutofillExternalDelegate::Reset() {
   manager_->delegate()->HideAutofillPopup();
 }
 
+void AutofillExternalDelegate::OnPingAck() {
+  ReissueQuery();

[Ilya Sherman, 2014/05/29 01:28:24]

nit: Possibly worth just inlining the code at this point.

[erikchen, 2014/05/29 20:31:31]

Done.

+}
+
 base::WeakPtr<AutofillExternalDelegate> AutofillExternalDelegate::GetWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
 }
@@ -308,4 +343,18 @@ 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_);
+}
+
+void AutofillExternalDelegate::PingRenderer() {
+  driver_->PingRenderer();
+}
+#endif
+
 }  // namespace autofill