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

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

If the browser does not yet have permission to access the user's contacts, one
of the Autofill suggestions has the text "Enable Autofill using Contacts". If
the user selects that suggestion, the browser will prompt the user for access
to the user's address book.

The act of clicking the permissions dialog causes Blink to lose its focus on
the text field, which also dismisses the Autofill popup. If the user has
granted Chrome access to the address book, the autofill popup will be presented
again, and filled in.

This CL does not include the image asset, nor does it include the logic to
display the image asset on the left of the autofill entry.

BUG=139154
TEST=Run the command `tccutil reset AddressBook` to reset AddressBook
permissions for all applications. Clear the default Chromium profile:
`rm -rf ~/Library/Application\ Support/Chromium/Default/`.  Launch Chromium by
double clicking on it from Finder.  Navigate to
`https://www.mycontactform.com/samples/change_address.php`. Double click the
"First Name" field. There should not be a prompt to access your "Contacts".
There should be exactly 1 autofill entry: "Enable Autofill using Contacts...".
Clicking that autofill entry should open a system prompt that asks for Contacts
permissions for Chromium.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274040

[erikchen, 2014-05-16 22:37:05]

Please review.

Are there any particular tests that you would like to see? The logic is simple
enough that unit tests are not likely to provide dividends. I could add a
browser test and stub out the interaction with the address book?

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:250: for (auto it :
form_structures_) {
I couldn't find another way to get the type of the field. Suggestions?

[Ilya Sherman, 2014-05-16 22:54:03]

Thanks!

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:99:
labels.push_back(base::ASCIIToUTF16(""));
nit: "base::ASCIIToUTF16("")" -> "base::string16()"

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:100:
icons.push_back(base::ASCIIToUTF16("americanExpressCC"));
I'd rather have no icon than a bizarre one for now.

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:90: const FormFieldData&
fieldData);
This method is too closely tied to the UI.  Instead, this can be split into two
parts:
  (1) A method "bool HasQueriedForAccessToMacContacts()" (or, you know, a
shorter and better method name :P) that returns whether or not we've ever shown
the permission prompt to the user.
  (2) If the popup has no Autofill suggestions for a field, and
HasQueriedForAccessToMacContacts() returns false, show the extra row.

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:93: void
AccessMacContacts();
These methods should only be available on Mac OS X.

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:922: void
PersonalDataManager::AccessMacContacts() {
Either these methods should not have "Mac" in the name at all, or they should
only be defined on Mac OS X.

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:100: return;
nit: Please call into HasAccessMacContacts() here rather than duplicating the
code.

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:300: if
(!pref_service_->GetBoolean(prefs::kAutofillAuxiliaryProfilesQueried)) {
This will always be false now, right?

https://codereview.chromium.org/286243002/diff/180001/components/autofill_str...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill_str...
components/autofill_strings.grdp:116: Enable Autofill using Contacts…
I think this string should be updated.  Please check with Alex.  If he has no
particular preferences, I'll think about this more deeply and make some
suggestions.  (I don't have any concrete suggestions right now, but I find the
current string pretty hard to make sense of.)

[erikchen, 2014-05-19 20:58:32]

PTAL

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:99:
labels.push_back(base::ASCIIToUTF16(""));
On 2014/05/16 22:54:02, Ilya Sherman wrote:
> nit: "base::ASCIIToUTF16("")" -> "base::string16()"

Done.

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:100:
icons.push_back(base::ASCIIToUTF16("americanExpressCC"));
On 2014/05/16 22:54:02, Ilya Sherman wrote:
> I'd rather have no icon than a bizarre one for now.

Done.

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:90: const FormFieldData&
fieldData);
On 2014/05/16 22:54:02, Ilya Sherman wrote:
> This method is too closely tied to the UI.  Instead, this can be split into
two
> parts:
>   (1) A method "bool HasQueriedForAccessToMacContacts()" (or, you know, a
> shorter and better method name :P) that returns whether or not we've ever
shown
> the permission prompt to the user.
>   (2) If the popup has no Autofill suggestions for a field, and
> HasQueriedForAccessToMacContacts() returns false, show the extra row.

I've done as you've suggested and split the logic into two methods: 
bool HasPromptedForAccessToAddressBook
bool FieldSupportsAddressBookAutofill

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:93: void
AccessMacContacts();
On 2014/05/16 22:54:02, Ilya Sherman wrote:
> These methods should only be available on Mac OS X.

I've added #if defined(OS_MACOSX) to all the mac specific methods in this CL.

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:922: void
PersonalDataManager::AccessMacContacts() {
On 2014/05/16 22:54:02, Ilya Sherman wrote:
> Either these methods should not have "Mac" in the name at all, or they should
> only be defined on Mac OS X.

I've removed these methods from personal_data_manager.cc

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:100: return;
On 2014/05/16 22:54:03, Ilya Sherman wrote:
> nit: Please call into HasAccessMacContacts() here rather than duplicating the
> code.

I've created an anonymous-namespace scoped function to remove the logic
duplication.

https://codereview.chromium.org/286243002/diff/180001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:300: if
(!pref_service_->GetBoolean(prefs::kAutofillAuxiliaryProfilesQueried)) {
On 2014/05/16 22:54:03, Ilya Sherman wrote:
> This will always be false now, right?

Right you are. I've removed the conditional.

https://codereview.chromium.org/286243002/diff/180001/components/autofill_str...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/286243002/diff/180001/components/autofill_str...
components/autofill_strings.grdp:116: Enable Autofill using Contacts…
On 2014/05/16 22:54:03, Ilya Sherman wrote:
> I think this string should be updated.  Please check with Alex.  If he has no
> particular preferences, I'll think about this more deeply and make some
> suggestions.  (I don't have any concrete suggestions right now, but I find the
> current string pretty hard to make sense of.)

Alex has indicated that he has intended this string for production, although
he's willing to take suggestions. As a heavy OSX user, I found the phrasing to
be slightly odd, but the meaning to be clear. I will have a second CL to update
the UI (add the asset, show the asset on the left side of the autofill entry)
and I'd be happy to update this string in that CL, if you have better
suggestions.

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

Apologies for the delayed review -- I'm traveling, in Paris atm.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:102: }
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().

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:174:
manager_->AccessAddressBook();
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.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:255: const FormFieldData&
fieldData) {
nit: "fieldData" -> "field" or "field_data"

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:256: for (auto it :
form_structures_) {
C++ files should not use auto.  It's fine to keep the "auto" if you move this
into the .mm file, though.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:292: #endif  //
defined(OS_MACOSX)
I'd prefer to not make any changes to the AutofillManager class, and instead to
get at the personal_data_manager via
autofill_manager_->delegate()->GetPersonalDataManager().

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:204: // Whether
Chromium has prompted the user for permission to access the user's
nit: "Chromium" -> "Chrome" (in general, comments in the code refer to "Chrome"
(the main binary that we ship), even though the open source project name is
Chromium).

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:206: bool
HasPromptedForAccessToAddressBook();
nit: Please leave a blank line after this one.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:207: // If Chromium has
not prompted for access to the user's address book, prompts
nit: Ditto.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:301:
pref_service_->SetBoolean(prefs::kAutofillAuxiliaryProfilesQueried, true);
I'd prefer that this remain in the GetAddressBook() method, since that is what
actually prompts for access.  If there were ever another codepath introduced
that could call that method, it would be correct to set the pref to true in that
case.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:307: addressBook
!= nil);
I'd prefer to be extra careful, and only set this when the pref was previously
false.

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

No problems, enjoy Paris!

PTAL

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:102: }
On 2014/05/21 11:29:15, Ilya Sherman wrote:
> 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().

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.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:174:
manager_->AccessAddressBook();
On 2014/05/21 11:29:15, Ilya Sherman wrote:
> 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.

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

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:255: const FormFieldData&
fieldData) {
On 2014/05/21 11:29:15, Ilya Sherman wrote:
> nit: "fieldData" -> "field" or "field_data"

ack, thanks. I've been writing too much ObjC.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:256: for (auto it :
form_structures_) {
On 2014/05/21 11:29:15, Ilya Sherman wrote:
> C++ files should not use auto.  It's fine to keep the "auto" if you move this
> into the .mm file, though.

I've moved this logic into personal_data_manager_mac.mm, but also removed the
'auto' keyword. I seem to recall a conversation with thakis where he mentioned
that Chromium still supports non C++11 compliant compilers?

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:292: #endif  //
defined(OS_MACOSX)
On 2014/05/21 11:29:15, Ilya Sherman wrote:
> I'd prefer to not make any changes to the AutofillManager class, and instead
to
> get at the personal_data_manager via
> autofill_manager_->delegate()->GetPersonalDataManager().

The method ShouldShowAccessAddressBookSuggestion needed to be piped through
autofill_manager to get access to the form_structures_ vector. For symmetry, I
piped AccessAddressBook through the AutofillManager, even though that is not
strictly necessary.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:204: // Whether
Chromium has prompted the user for permission to access the user's
On 2014/05/21 11:29:15, Ilya Sherman wrote:
> nit: "Chromium" -> "Chrome" (in general, comments in the code refer to
"Chrome"
> (the main binary that we ship), even though the open source project name is
> Chromium).

good to know, thanks!

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:206: bool
HasPromptedForAccessToAddressBook();
On 2014/05/21 11:29:15, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:207: // If Chromium has
not prompted for access to the user's address book, prompts
On 2014/05/21 11:29:15, Ilya Sherman wrote:
> nit: Ditto.

Done. I made sure that the diff file has no more references to 'Chromium'.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:301:
pref_service_->SetBoolean(prefs::kAutofillAuxiliaryProfilesQueried, true);
On 2014/05/21 11:29:15, Ilya Sherman wrote:
> I'd prefer that this remain in the GetAddressBook() method, since that is what
> actually prompts for access.  If there were ever another codepath introduced
> that could call that method, it would be correct to set the pref to true in
that
> case.

Good point. I've done as you suggested.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:307: addressBook
!= nil);
On 2014/05/21 11:29:15, Ilya Sherman wrote:
> I'd prefer to be extra careful, and only set this when the pref was previously
> false.

I've added a check for that pref value, as well as a check for
kAutofillAuxiliaryProfilesEnabled, which was missing.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:306:
ABAddressBook* addressBook = GetAddressBook(pref_service_);
It's possible for a user to mess with the configuration file, which would cause
this method to not present a blocking modal dialog. There's no good way to
detect if this has happened. I considered grabbing a timestamp before and after
this API call to try to heuristically determine whether a dialog was presented,
but decided that that would cause more problems than it solved.

[Ilya Sherman, 2014-05-22 15:13:58]

Thanks, Erik.

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/286243002/diff/280001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:256: for (auto it :
form_structures_) {
On 2014/05/21 22:00:54, erikchen1 wrote:
> On 2014/05/21 11:29:15, Ilya Sherman wrote:
> > C++ files should not use auto.  It's fine to keep the "auto" if you move
this
> > into the .mm file, though.
> 
> I've moved this logic into personal_data_manager_mac.mm, but also removed the
> 'auto' keyword. I seem to recall a conversation with thakis where he mentioned
> that Chromium still supports non C++11 compliant compilers?

Chromium in general still supports non C++11 compliant compilers, but Chromium
on Mac only supports clang, which is C++11 compliant.  Hence, you can use auto
in Mac-only files.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:179:
redisplay_autofill_ = true;
Why does it matter whether the modal dialog was presented?  If the user was able
to click on the relevant entry in the popup, then we should re-issue the query,
even if this particular click wasn't the cause of access being granted.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.h:148: bool
redisplay_autofill_;
nit: Perhaps "redisplay_popop_on_focus_lost_" or something like that?  I'd
definitely replace "autofill" with "popup"; the rest I feel less strongly about.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:250: data, field_data,
form_structures_);
You should probably call GetCachedFormAndField(), and just pass in the field
type to this method.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:91: const FormFieldData&
field_data);
nit: Please leave a blank line after this one.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:213: // prompted with a
modal dialog.
nit: Please update this comment.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:217: const
ScopedVector<FormStructure>& form_structures);
Note: It's generally better to pass the underlying std::vector, rather than
passing a ScopedVector.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:309: addressBook
!= nil);
I'd prefer for this to be in GetAddressBook() as well, for the same reason.

[erikchen, 2014-05-22 20:40:58]

isherman: PTAL

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:179:
redisplay_autofill_ = true;
On 2014/05/22 15:13:59, Ilya Sherman wrote:
> Why does it matter whether the modal dialog was presented?  If the user was
able
> to click on the relevant entry in the popup, then we should re-issue the
query,
> even if this particular click wasn't the cause of access being granted.

It's important to not set the redisplay_popop_on_focus_lost_ flag if a modal was
not presented, as that would cause an undesired popup redisplay. I've added
logic to immediately refresh the popup if a modal was not presented.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.h:148: bool
redisplay_autofill_;
On 2014/05/22 15:13:59, Ilya Sherman wrote:
> nit: Perhaps "redisplay_popop_on_focus_lost_" or something like that?  I'd
> definitely replace "autofill" with "popup"; the rest I feel less strongly
about.

I've changed the property name to your suggestion.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:250: data, field_data,
form_structures_);
On 2014/05/22 15:13:59, Ilya Sherman wrote:
> You should probably call GetCachedFormAndField(), and just pass in the field
> type to this method.

ah, good point. done.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:91: const FormFieldData&
field_data);
On 2014/05/22 15:13:59, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:213: // prompted with a
modal dialog.
On 2014/05/22 15:13:59, Ilya Sherman wrote:
> nit: Please update this comment.

Done.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:217: const
ScopedVector<FormStructure>& form_structures);
On 2014/05/22 15:13:59, Ilya Sherman wrote:
> Note: It's generally better to pass the underlying std::vector, rather than
> passing a ScopedVector.

I've used your earlier suggestion of just passing in a type.

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/286243002/diff/360001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:309: addressBook
!= nil);
On 2014/05/22 15:13:59, Ilya Sherman wrote:
> I'd prefer for this to be in GetAddressBook() as well, for the same reason.

Done.

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

Thanks!  Just one last point of contention:

https://codereview.chromium.org/286243002/diff/400001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/400001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:190:
redisplay_popop_on_focus_lost_ = true;
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?

https://codereview.chromium.org/286243002/diff/400001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/286243002/diff/400001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:43: bool
first_access = !HasPromptedForAccessToAddressBook(pref_service);
nit: I'd leave a blank line after this one.

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

PTAL

https://codereview.chromium.org/286243002/diff/400001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/400001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:190:
redisplay_popop_on_focus_lost_ = true;
On 2014/05/23 15:47:10, Ilya Sherman wrote:
> 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?

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)

https://codereview.chromium.org/286243002/diff/400001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/286243002/diff/400001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:43: bool
first_access = !HasPromptedForAccessToAddressBook(pref_service);
On 2014/05/23 15:47:10, Ilya Sherman wrote:
> nit: I'd leave a blank line after this one.

Done.

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

https://codereview.chromium.org/286243002/diff/400001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/400001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:190:
redisplay_popop_on_focus_lost_ = true;
On 2014/05/23 17:19:38, erikchen1 wrote:
> On 2014/05/23 15:47:10, Ilya Sherman wrote:
> > 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?
> 
> 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)

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.

[erikchen, 2014-05-28 18:31:17]

I just tried your suggestion. It doesn't quite work because my comment was
inaccurate. After the blocking modal dialog is dismissed, there are NSEvents
queued in the NSRunLoop. When a specific NSEvent gets unqueued, it is sent via
IPC to blink, which then sends an IPC back to Chrome which dismisses the popup.
I see two solutions:

1) Keep the current code, and update the comments to be accurate.
2) Post a delayed task to base::MessageLoop::current() to refresh the popup,
with an artificial delay (let's say 0.1s). This simplifies the code in
autofill_external_delegate, and changes the dependency from relying on
particular Cocoa and IPC/Blink behavior, to relying on the amount of time
aforementioned behavior takes. 

Neither solution is ideal, but the only other alternative I see would require
adding a lot more plumbing to actually trace the NSEvents and their effects,
which seems overkill for this feature.

I'm happy to implement either solution, or any alternatives you can think of.
Let me know!

[erikchen, 2014-05-29 00:09:17]

isherman: PTAL

[Ilya Sherman, 2014-05-29 01:28:23]

Thanks!

https://codereview.chromium.org/286243002/diff/500001/components/autofill/con...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/286243002/diff/500001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver.cc:192:
autofill_manager_.get(),
nit: Can this skip the AutofillManager, and just route to the
autofill_external_delegate_ directly?

https://codereview.chromium.org/286243002/diff/500001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/286243002/diff/500001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:97: // Instructs the
renderer to immediately return an IPC acknowledging the ping.
nit: Please mention that this is used to correctly sequence events, since IPCs
are guaranteed to be processed in order.

https://codereview.chromium.org/286243002/diff/500001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/500001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:177: // A
blocking modal was presented, and the user has already responded to the
nit: The blocking modal might not have been presented, right?  Probably worth
clarifying that a bit in the comment.

https://codereview.chromium.org/286243002/diff/500001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:180: // back an
IPC to cause the text field to lose focus, which will dismiss the
nit: Technically, the text field loses focus on the renderer side rather than on
the browser side, right?

https://codereview.chromium.org/286243002/diff/500001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:183: // ensures
that all side effects the blocking modal will have been
nit: "all side effects the blocking modal" -> "all side effects of the blocking
modal"

https://codereview.chromium.org/286243002/diff/500001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:221:
ReissueQuery();
nit: Possibly worth just inlining the code at this point.

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

isherman: PTAL. You'll want to diff patch set 9 against patch set 8, as patch
set 10 is a rebase.

https://codereview.chromium.org/286243002/diff/500001/components/autofill/con...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/286243002/diff/500001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver.cc:192:
autofill_manager_.get(),
On 2014/05/29 01:28:24, Ilya Sherman wrote:
> nit: Can this skip the AutofillManager, and just route to the
> autofill_external_delegate_ directly?

Done.

https://codereview.chromium.org/286243002/diff/500001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/286243002/diff/500001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:97: // Instructs the
renderer to immediately return an IPC acknowledging the ping.
On 2014/05/29 01:28:24, Ilya Sherman wrote:
> nit: Please mention that this is used to correctly sequence events, since IPCs
> are guaranteed to be processed in order.

Done.

https://codereview.chromium.org/286243002/diff/500001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/500001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:177: // A
blocking modal was presented, and the user has already responded to the
On 2014/05/29 01:28:24, Ilya Sherman wrote:
> nit: The blocking modal might not have been presented, right?  Probably worth
> clarifying that a bit in the comment.

Done.

https://codereview.chromium.org/286243002/diff/500001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:180: // back an
IPC to cause the text field to lose focus, which will dismiss the
On 2014/05/29 01:28:24, Ilya Sherman wrote:
> nit: Technically, the text field loses focus on the renderer side rather than
on
> the browser side, right?

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

https://codereview.chromium.org/286243002/diff/500001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:183: // ensures
that all side effects the blocking modal will have been
On 2014/05/29 01:28:24, Ilya Sherman wrote:
> nit: "all side effects the blocking modal" -> "all side effects of the
blocking
> modal"

Done.

https://codereview.chromium.org/286243002/diff/500001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:221:
ReissueQuery();
On 2014/05/29 01:28:24, Ilya Sherman wrote:
> nit: Possibly worth just inlining the code at this point.

Done.

[Ilya Sherman, 2014-05-29 20:35:08]

LGTM with a final nit.  Thanks, Erik!

https://codereview.chromium.org/286243002/diff/520001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/520001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:234: // Reissues
the most recent query, which will reopen the autofill popup.
nit: "Reissues" -> "Reissue"

[erikchen, 2014-05-29 20:47:26]

https://codereview.chromium.org/286243002/diff/520001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/286243002/diff/520001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:234: // Reissues
the most recent query, which will reopen the autofill popup.
On 2014/05/29 20:35:08, Ilya Sherman wrote:
> nit: "Reissues" -> "Reissue"

Done.

[erikchen, 2014-05-29 20:47:42]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-05-29 20:49:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/286243002/580001

[commit-bot: I haz the power, 2014-05-30 06:57:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-30 07:04:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-30 07:04:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[erikchen, 2014-05-30 17:15:13]

dcheng: looking for an OWNER review of
components/autofill/content/common/autofill_messages.h

[dcheng, 2014-05-30 17:55:06]

IPC changes LGTM

[erikchen, 2014-05-30 19:03:41]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-05-30 19:06:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/286243002/600001

[commit-bot: I haz the power, 2014-05-30 23:44:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[erikchen, 2014-05-30 23:54:17]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2014-05-31 00:00:06]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-05-31 00:06:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/286243002/620001

[commit-bot: I haz the power, 2014-05-31 05:14:49]

Change committed as 274040