mac: Clean up autofill integration with Address Book.

mac: Clean up autofill integration with Address Book.

Changes:
 - Update the copy of the string that describes giving Chrome access to the
 user's Address Book.
 - When the user selects the option to pull autofill suggestions from the
 Address Book, Chrome immediately prompts the user for permission to use the
 Address Book. I considered delaying the prompt until the option screen is
 dismissed, but that works less because there are too many ways to dismiss the
 screen that cause the prompt to appear contextless. (e.g. The user
 performs a history navigation).
 - Add a resource for the Contacts icon for the autofill entry.
 - Add metrics for the autofill entry that suggests that the user give Chrome
 access to the Address Book.
 - The pref for auxiliary profiles should default to NO on mac, and should not
 be syncable.
 - Update the logic in PersonalDataManagerMac to reflect the new default state
 of the pref.
 - Update layout logic of the autofill entry to move the text up by 1 pt, to
 better account for the weighting of the Contacts icon asset.
BUG=139154
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275367

[erikchen, 2014-05-30 02:06:14]

Please review.

https://codereview.chromium.org/301343002/diff/10001/chrome/browser/resources...
File chrome/browser/resources/options/autofill_options.js (right):

https://codereview.chromium.org/301343002/diff/10001/chrome/browser/resources...
chrome/browser/resources/options/autofill_options.js:66:
chrome.send('accessAddressBook');
I assumed that we would need to refresh the list of addresses, but the addresses
from the contact book don't seem to ever show up in this list? I haven't looked
in detail into this.

[Ilya Sherman, 2014-05-30 23:39:47]

Thanks, Erik!

https://codereview.chromium.org/301343002/diff/10001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/301343002/diff/10001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9677: <message
name="IDS_AUTOFILL_USE_MAC_ADDRESS_BOOK" desc="Checkbox label to enable access
to the user's Address Book on Mac. On Mac the application that manages the
Address Book is called Contacts.">
To be clear, the application was previously called "Address Book".  It was
renamed in 10.8 ("Mountain Lion").

https://codereview.chromium.org/301343002/diff/10001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9678: Include addresses from Contacts
I think it's important to continue to try to indicate that these are just the
user's own addresses, rather than addresses of all the user's contacts.

https://codereview.chromium.org/301343002/diff/10001/chrome/browser/resources...
File chrome/browser/resources/options/autofill_options.js (right):

https://codereview.chromium.org/301343002/diff/10001/chrome/browser/resources...
chrome/browser/resources/options/autofill_options.js:66:
chrome.send('accessAddressBook');
On 2014/05/30 02:06:15, erikchen1 wrote:
> I assumed that we would need to refresh the list of addresses, but the
addresses
> from the contact book don't seem to ever show up in this list? I haven't
looked
> in detail into this.

Indeed, we do not show Address Book addresses in this list.  There's a bug on
file somewhere about showing them in a "disabled for editing" state, but nobody
has implemented that behavior.

https://codereview.chromium.org/301343002/diff/10001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/autofill_options_handler.h (right):

https://codereview.chromium.org/301343002/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/autofill_options_handler.h:50: // The user wants
to grant Chrome access to the user's Address Book.
Please also mention in this comment that we will immediately try to access the
Address Book, so that the resulting modal dialog is shown in context rather than
at a later, surprising time, out of context.

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:106:
"Autofill_Showed_Access_Mac_Address_Book_Entry"));
Please use an enumerated histogram rather than a user action.  Also, FYI, user
action names do not typically have so many underscores in them.

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:209: #else  //
defined(OS_ANDROID)
Hmm, what happens if the user enables access to their Android contacts, and also
uses a Mac?  I'm not sure how Sync handles prefs that are only syncable some of
the time.  (Before, syncing the pref to other platforms simply had no effect, so
this wasn't as relevant a question.)

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:214: #endif  //
defined(OS_MACOSX) || defined(OS_ANDROID)
nit: Please update this comment.

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:214: #endif  //
defined(OS_MACOSX) || defined(OS_ANDROID)
What happens to users who have already granted Chrome access to their Address
Book?  Will we be effectively turning off the feature for them?  It seems like
we should try to affect only those users who have not been prompted yet.

[erikchen, 2014-05-31 00:14:08]

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:214: #endif  //
defined(OS_MACOSX) || defined(OS_ANDROID)
On 2014/05/30 23:39:48, Ilya Sherman wrote:
> What happens to users who have already granted Chrome access to their Address
> Book?  Will we be effectively turning off the feature for them?  It seems like
> we should try to affect only those users who have not been prompted yet.

ack. The logic in this CL for personal_data_manager_mac is not currently correct
(it doesn't bother checking the pref at all). To avoid confusion with android,
we should create a new pref and migrate over the old values. Is there a well
defined way for doing this? A quick search revealed:
session_startup_pref.cc::MigrateIfNecessary, should I copy it's format?

[Ilya Sherman, 2014-05-31 00:39:15]

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:214: #endif  //
defined(OS_MACOSX) || defined(OS_ANDROID)
On 2014/05/31 00:14:08, erikchen1 wrote:
> On 2014/05/30 23:39:48, Ilya Sherman wrote:
> > What happens to users who have already granted Chrome access to their
Address
> > Book?  Will we be effectively turning off the feature for them?  It seems
like
> > we should try to affect only those users who have not been prompted yet.
> 
> ack. The logic in this CL for personal_data_manager_mac is not currently
correct
> (it doesn't bother checking the pref at all). To avoid confusion with android,
> we should create a new pref and migrate over the old values. Is there a well
> defined way for doing this? A quick search revealed:
> session_startup_pref.cc::MigrateIfNecessary, should I copy it's format?

Hmm, that looks a little more complicated than what we need, but otherwise
roughly right.  We essentially need (1) to check whether the migration has
already happened, and (2) if not, migrate the pref and record that the migration
has now happened.  One option would be to rename the pref on both Mac and
Android.  If we do that, then we can test (1) by checking for the existence of
the old pref, which we'd clear in step (2).

[erikchen, 2014-06-02 21:21:16]

isherman: PTAL

https://codereview.chromium.org/301343002/diff/10001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/301343002/diff/10001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9677: <message
name="IDS_AUTOFILL_USE_MAC_ADDRESS_BOOK" desc="Checkbox label to enable access
to the user's Address Book on Mac. On Mac the application that manages the
Address Book is called Contacts.">
On 2014/05/30 23:39:48, Ilya Sherman wrote:
> To be clear, the application was previously called "Address Book".  It was
> renamed in 10.8 ("Mountain Lion").
I've updated the comment to include the information that in 10.8+, the app is
called Contacts.

https://codereview.chromium.org/301343002/diff/10001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9678: Include addresses from Contacts
On 2014/05/30 23:39:48, Ilya Sherman wrote:
> I think it's important to continue to try to indicate that these are just the
> user's own addresses, rather than addresses of all the user's contacts.

I've reverted to the original copy, with the exception of replacing Address Book
with Contacts

https://codereview.chromium.org/301343002/diff/10001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/autofill_options_handler.h (right):

https://codereview.chromium.org/301343002/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/autofill_options_handler.h:50: // The user wants
to grant Chrome access to the user's Address Book.
On 2014/05/30 23:39:48, Ilya Sherman wrote:
> Please also mention in this comment that we will immediately try to access the
> Address Book, so that the resulting modal dialog is shown in context rather
than
> at a later, surprising time, out of context.

Done.

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:106:
"Autofill_Showed_Access_Mac_Address_Book_Entry"));
On 2014/05/30 23:39:48, Ilya Sherman wrote:
> Please use an enumerated histogram rather than a user action.  Also, FYI, user
> action names do not typically have so many underscores in them.

Done.

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:209: #else  //
defined(OS_ANDROID)
On 2014/05/30 23:39:48, Ilya Sherman wrote:
> Hmm, what happens if the user enables access to their Android contacts, and
also
> uses a Mac?  I'm not sure how Sync handles prefs that are only syncable some
of
> the time.  (Before, syncing the pref to other platforms simply had no effect,
so
> this wasn't as relevant a question.)

I've added a new pref for mac.

https://codereview.chromium.org/301343002/diff/10001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:214: #endif  //
defined(OS_MACOSX) || defined(OS_ANDROID)
On 2014/05/31 00:39:15, Ilya Sherman wrote:
> On 2014/05/31 00:14:08, erikchen1 wrote:
> > On 2014/05/30 23:39:48, Ilya Sherman wrote:
> > > What happens to users who have already granted Chrome access to their
> Address
> > > Book?  Will we be effectively turning off the feature for them?  It seems
> like
> > > we should try to affect only those users who have not been prompted yet.
> > 
> > ack. The logic in this CL for personal_data_manager_mac is not currently
> correct
> > (it doesn't bother checking the pref at all). To avoid confusion with
android,
> > we should create a new pref and migrate over the old values. Is there a well
> > defined way for doing this? A quick search revealed:
> > session_startup_pref.cc::MigrateIfNecessary, should I copy it's format?
> 
> Hmm, that looks a little more complicated than what we need, but otherwise
> roughly right.  We essentially need (1) to check whether the migration has
> already happened, and (2) if not, migrate the pref and record that the
migration
> has now happened.  One option would be to rename the pref on both Mac and
> Android.  If we do that, then we can test (1) by checking for the existence of
> the old pref, which we'd clear in step (2).

I've added a new pref for mac, as well as a migration pref. The migration is
performed once on each installed instance of Chrome that's running OSX.

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:314:
pref_service_->SetBoolean(prefs::kAutofillUseMacAddressBook, true);
I've intentionally removed the check against the pref for
HasPromptedForAccessToAddressBook, as it is theoretically possible for the user
to manually modify his prefs to change that value. If for some reason the pref
is out of sync, then calling GetAddressBook will not present a blocking dialog,
and the pref will be updated.

[Ilya Sherman, 2014-06-02 22:54:26]

Thanks, Erik.

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:21: // An
autofill entry was shown that prompts the user to give Chrome access to
nit: "autofill" -> "Autofill"

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:23:
SHOWED_ACCESS_AB_ENTRY = 0,
nit: "AB" -> "ADDRESS_BOOK"

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:124:
UMA_HISTOGRAM_ENUMERATION("Autofill.MacAccessAddressBook",
Optional nit: I'd drop "Access" from the histogram name, i.e. just
"Autofill.MacAddressBook"

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:126:
ACCESS_AB_ENTRY_MAX);
nit: Please define a wrapper function for emitting to this histogram, so that
it's hard to accidentally make a typo for just one of the code paths.

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:214: #endif  //
defined(OS_MACOSX) || defined(OS_ANDROID)
This is only needed on Android now, right?  It's probably a good idea to rename
the pref if it's only used on Android, to clarify its semantics.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:217:
prefs::kAutofillAuxiliaryProfilesQueried,
nit: It's probably worth renaming this pref to match the new pref's name.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:236:
prefs::kAutofillUseMacAddressBookMigration,
Optional nit: Perhaps something more like
"kAutofillHasMigratedUseMacAddressBookPref"?

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
File components/autofill/core/browser/autofill_test_utils.cc (right):

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_test_utils.cc:214:
prefs->SetBoolean(prefs::kAutofillAuxiliaryProfilesEnabled, false);
I think you can just replace this line, rather than keeping it and adding the
new line.

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:531: }
nit: No need for curly braces; ditto below.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:536: #endif
nit: "// defined(OS_MACOSX) && !defined(OS_IOS)"

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:40: }
nit: Please leave a blank line after this one.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
File components/autofill/core/common/autofill_pref_names.cc (right):

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/common/autofill_pref_names.cc:30: // Whether autofill
should try to use the Mac Address Book. If this value is
nit: "autofill" -> "Autofill" (as it is a feature name)

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/common/autofill_pref_names.cc:31: // true, then
kAutofillAuxiliaryProfilesQueried is expected to be true.
nit: "is expected to be true" -> "is expected to also be true"

[erikchen, 2014-06-03 01:34:18]

isherman: PTAL

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:21: // An
autofill entry was shown that prompts the user to give Chrome access to
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> nit: "autofill" -> "Autofill"

I've gone through the CL and replaced all instances of "autofill" in a comment
with "Autofill".

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:23:
SHOWED_ACCESS_AB_ENTRY = 0,
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> nit: "AB" -> "ADDRESS_BOOK"

Done.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:124:
UMA_HISTOGRAM_ENUMERATION("Autofill.MacAccessAddressBook",
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> Optional nit: I'd drop "Access" from the histogram name, i.e. just
> "Autofill.MacAddressBook"

Done.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:126:
ACCESS_AB_ENTRY_MAX);
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> nit: Please define a wrapper function for emitting to this histogram, so that
> it's hard to accidentally make a typo for just one of the code paths.

Done.

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:214: #endif  //
defined(OS_MACOSX) || defined(OS_ANDROID)
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> This is only needed on Android now, right?  It's probably a good idea to
rename
> the pref if it's only used on Android, to clarify its semantics.

Done. I've kept the OS_MACOSX preprocessor conditional for consistency.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:217:
prefs::kAutofillAuxiliaryProfilesQueried,
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> nit: It's probably worth renaming this pref to match the new pref's name.

Done.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:236:
prefs::kAutofillUseMacAddressBookMigration,
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> Optional nit: Perhaps something more like
> "kAutofillHasMigratedUseMacAddressBookPref"?

Done.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
File components/autofill/core/browser/autofill_test_utils.cc (right):

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/autofill_test_utils.cc:214:
prefs->SetBoolean(prefs::kAutofillAuxiliaryProfilesEnabled, false);
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> I think you can just replace this line, rather than keeping it and adding the
> new line.

Done.

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:531: }
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> nit: No need for curly braces; ditto below.

Done.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:536: #endif
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> nit: "// defined(OS_MACOSX) && !defined(OS_IOS)"

I've added the comment. As far as I can know, the trailing comment is optional,
and there are other instances in this CL where an "#endif" does not have a
trailing comment. Let me know if you want me to change all instances of #endif
not having a trailing comment.

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_mac.mm:40: }
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
File components/autofill/core/common/autofill_pref_names.cc (right):

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/common/autofill_pref_names.cc:30: // Whether autofill
should try to use the Mac Address Book. If this value is
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> nit: "autofill" -> "Autofill" (as it is a feature name)

Done.

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/common/autofill_pref_names.cc:31: // true, then
kAutofillAuxiliaryProfilesQueried is expected to be true.
On 2014/06/02 22:54:27, Ilya Sherman wrote:
> nit: "is expected to be true" -> "is expected to also be true"

Done.

https://codereview.chromium.org/301343002/diff/130001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/301343002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:2478:
TEST_F(AutofillManagerTest, AuxiliaryProfilesReset) {
I refrained from adding a test case for OS_MACOSX, since the test is already a
reiteration of logic in autofill_manager.cc

[Ilya Sherman, 2014-06-03 23:49:00]

Thanks, Erik.

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:536: #endif
On 2014/06/03 01:34:19, erikchen1 wrote:
> On 2014/06/02 22:54:27, Ilya Sherman wrote:
> > nit: "// defined(OS_MACOSX) && !defined(OS_IOS)"
> 
> I've added the comment. As far as I can know, the trailing comment is
optional,
> and there are other instances in this CL where an "#endif" does not have a
> trailing comment. Let me know if you want me to change all instances of #endif
> not having a trailing comment.

I think the trailing comment is strongly recommended except for header includes
and forward declarations -- it's basically making up for the lack of curly
braces to indicate scope.  So, please do go through and add trailing comments
for all of the #endif's.

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

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:36:
"Autofill.MacAddressBook", type, ACCESS_ADDRESS_BOOK_ENTRY_MAX);
Please add this histogram to histograms.xml.

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

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:244: if
(prefs->GetBoolean(prefs::kAutofillHasMigratedUseMacAddressBookPref))
It looks like the PrefService exports a method named IsDefaultValue().  Can we
use that method to eliminate this extra pref entirely?

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:251:
prefs->GetBoolean(prefs::kAutofillAndroidAuxiliaryProfilesEnabled);
Hmm, this looks really odd.  It makes me think that I was wrong to suggest just
changing the pref name in code -- we should either revert to the previous name,
or actually migrate from the old pref to a new pref that's defined just on
Android.

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
File components/autofill/core/browser/autofill_test_utils.cc (right):

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/autofill_test_utils.cc:207: #if
defined(OS_MACOSX)
I bet you can skip the line below for iOS as well (or does iOS have a similar
Keychain concept?), and thusly combine the two #ifs.

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/autofill_test_utils.cc:216: #endif
nit: Please add the trailing comment.

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

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:531: #else
nit: Should this be only on Android?

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
File components/autofill/core/common/autofill_pref_names.cc (right):

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/common/autofill_pref_names.cc:16: // Book on mac. This
preference is important since the first time Chrome
nit: "mac" -> "Mac"

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
File components/autofill/core/common/autofill_pref_names.h (right):

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/common/autofill_pref_names.h:14: extern const char
kAutofillMacAddressBookQueried[];
nit: Please keep this list alphabetically sorted.

[erikchen, 2014-06-04 01:51:26]

isherman: PTAL

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

https://codereview.chromium.org/301343002/diff/110001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:536: #endif
On 2014/06/03 23:48:59, Ilya Sherman wrote:
> On 2014/06/03 01:34:19, erikchen1 wrote:
> > On 2014/06/02 22:54:27, Ilya Sherman wrote:
> > > nit: "// defined(OS_MACOSX) && !defined(OS_IOS)"
> > 
> > I've added the comment. As far as I can know, the trailing comment is
> optional,
> > and there are other instances in this CL where an "#endif" does not have a
> > trailing comment. Let me know if you want me to change all instances of
#endif
> > not having a trailing comment.
> 
> I think the trailing comment is strongly recommended except for header
includes
> and forward declarations -- it's basically making up for the lack of curly
> braces to indicate scope.  So, please do go through and add trailing comments
> for all of the #endif's.

I've added trailing comments to all #endif's contained in the .diff file for
this CL, except for code unrelated to autofill.

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

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:36:
"Autofill.MacAddressBook", type, ACCESS_ADDRESS_BOOK_ENTRY_MAX);
On 2014/06/03 23:48:59, Ilya Sherman wrote:
> Please add this histogram to histograms.xml.

Done.

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

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:244: if
(prefs->GetBoolean(prefs::kAutofillHasMigratedUseMacAddressBookPref))
On 2014/06/03 23:48:59, Ilya Sherman wrote:
> It looks like the PrefService exports a method named IsDefaultValue().  Can we
> use that method to eliminate this extra pref entirely?

Yes. I've done so.

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:251:
prefs->GetBoolean(prefs::kAutofillAndroidAuxiliaryProfilesEnabled);
On 2014/06/03 23:48:59, Ilya Sherman wrote:
> Hmm, this looks really odd.  It makes me think that I was wrong to suggest
just
> changing the pref name in code -- we should either revert to the previous
name,
> or actually migrate from the old pref to a new pref that's defined just on
> Android.

I reverted to the previous name.

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
File components/autofill/core/browser/autofill_test_utils.cc (right):

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/autofill_test_utils.cc:207: #if
defined(OS_MACOSX)
On 2014/06/03 23:48:59, Ilya Sherman wrote:
> I bet you can skip the line below for iOS as well (or does iOS have a similar
> Keychain concept?), and thusly combine the two #ifs.

A cursory inspection did not reveal any reason why chrome for iOS would not use
keychain, so I didn't touch this ifdef.

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/autofill_test_utils.cc:216: #endif
On 2014/06/03 23:48:59, Ilya Sherman wrote:
> nit: Please add the trailing comment.

Done.

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

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:531: #else
On 2014/06/03 23:48:59, Ilya Sherman wrote:
> nit: Should this be only on Android?

After the revert of the rename of the preference, there should not be any
confusion. The preference does not need to be ifdef'ed to be android only.

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
File components/autofill/core/common/autofill_pref_names.cc (right):

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/common/autofill_pref_names.cc:16: // Book on mac. This
preference is important since the first time Chrome
On 2014/06/03 23:48:59, Ilya Sherman wrote:
> nit: "mac" -> "Mac"

Done.

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
File components/autofill/core/common/autofill_pref_names.h (right):

https://codereview.chromium.org/301343002/diff/150001/components/autofill/cor...
components/autofill/core/common/autofill_pref_names.h:14: extern const char
kAutofillMacAddressBookQueried[];
On 2014/06/03 23:48:59, Ilya Sherman wrote:
> nit: Please keep this list alphabetically sorted.

Done.

[Ilya Sherman, 2014-06-04 20:46:09]

Thanks, Erik!

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

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:264: }
Hmm, is this extra work necessary?  The default value of the accessed pref is
false, and the default value of the UseMacAddressBook pref is also false, so it
seems like the result is the same either way.  Are you worried about the
defaults changing and thereby breaking the new user case if you omit this
explicit code?

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:1558: #else  //
defined(OS_MACOSX) || !defined(OS_IOS)
nit: I think these comments are typically omitted on the "#else" branches,
because it's a little ambiguous as to whether the comment reflects the "#if"
condition or its inverse.  (I promise I'll stop nitpicking the preprocessor
comments after this!)

[erikchen, 2014-06-04 21:13:03]

isherman: PTAL

Please note patch set 7, which includes layout and asset changes requested by
cleer, the designer. I generally try to avoid adding new behavior to CLs this
late in the review process, but the change was sufficiently small that I snuck
it in.

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

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:264: }
On 2014/06/04 20:46:09, Ilya Sherman wrote:
> Hmm, is this extra work necessary?  The default value of the accessed pref is
> false, and the default value of the UseMacAddressBook pref is also false, so
it
> seems like the result is the same either way.  Are you worried about the
> defaults changing and thereby breaking the new user case if you omit this
> explicit code?

I want to make sure that the migration code is called exactly once. If I don't
manually write to the pref, then the migration code is going to be called on
every launch (which requires 3 pref checks), whereas this one time write ensures
that for all future launches, there is only 1 pref checked before the migration
is skipped.

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:1558: #else  //
defined(OS_MACOSX) || !defined(OS_IOS)
On 2014/06/04 20:46:09, Ilya Sherman wrote:
> nit: I think these comments are typically omitted on the "#else" branches,
> because it's a little ambiguous as to whether the comment reflects the "#if"
> condition or its inverse.  (I promise I'll stop nitpicking the preprocessor
> comments after this!)

Done.

[Ilya Sherman, 2014-06-04 21:21:03]

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

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:264: }
On 2014/06/04 21:13:03, erikchen1 wrote:
> On 2014/06/04 20:46:09, Ilya Sherman wrote:
> > Hmm, is this extra work necessary?  The default value of the accessed pref
is
> > false, and the default value of the UseMacAddressBook pref is also false, so
> it
> > seems like the result is the same either way.  Are you worried about the
> > defaults changing and thereby breaking the new user case if you omit this
> > explicit code?
> 
> I want to make sure that the migration code is called exactly once. If I don't
> manually write to the pref, then the migration code is going to be called on
> every launch (which requires 3 pref checks), whereas this one time write
ensures
> that for all future launches, there is only 1 pref checked before the
migration
> is skipped.

I'm not following what you mean -- line 269 would always write to the pref,
right?

https://codereview.chromium.org/301343002/diff/270001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right):

https://codereview.chromium.org/301343002/diff/270001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:119: // text needs
to be bumped up by 1 pt to make it look vertically aligned.
nit: "1 pt" -> "1 pixel"?

[Ilya Sherman, 2014-06-04 21:24:46]

Sorry, I missed the histograms.xml file previously, because it doesn't have
unified diffs listed in the "Delta from patch set" column.

https://codereview.chromium.org/301343002/diff/270001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/301343002/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1613: +<histogram
name="AutoFill.MacAddressBook" enum="AutofillMacAddressBook">
nit: "AutoFill" -> "Autofill"

https://codereview.chromium.org/301343002/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1617: +    them to give Chrome access to
their Address Book.
I'd suggest rewording this to clarify what is being measured for someone who's
not very familiar with the Autofill feature.  Specifically, I'd mention the OS's
blocking dialog, and mention that this entry appears in the Autofill popup, and
that it only appears when there are no other suggestions.

https://codereview.chromium.org/301343002/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32282: +  <int value="1" label="Selected
Autofill entry"/>
Optional nit: I'd suggest replacing "Autofill" with "popup"

[erikchen, 2014-06-04 22:11:12]

isherman: PTAL

https://codereview.chromium.org/301343002/diff/270001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right):

https://codereview.chromium.org/301343002/diff/270001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:119: // text needs
to be bumped up by 1 pt to make it look vertically aligned.
On 2014/06/04 21:21:04, Ilya Sherman wrote:
> nit: "1 pt" -> "1 pixel"?

I do mean point, and not pixel. It's the standard Cocoa unit of layout.

pt is a common abbreviation for point, which I prefer since it makes it clear
that the 1 is a measurement. 

See figure 1-5.
https://developer.apple.com/library/mac/documentation/GraphicsAnimation/Conce...

https://codereview.chromium.org/301343002/diff/270001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/301343002/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1613: +<histogram
name="AutoFill.MacAddressBook" enum="AutofillMacAddressBook">
On 2014/06/04 21:24:47, Ilya Sherman wrote:
> nit: "AutoFill" -> "Autofill"

Good catch.

https://codereview.chromium.org/301343002/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1617: +    them to give Chrome access to
their Address Book.
On 2014/06/04 21:24:47, Ilya Sherman wrote:
> I'd suggest rewording this to clarify what is being measured for someone who's
> not very familiar with the Autofill feature.  Specifically, I'd mention the
OS's
> blocking dialog, and mention that this entry appears in the Autofill popup,
and
> that it only appears when there are no other suggestions.

I've rewritten and expanded the summary.

https://codereview.chromium.org/301343002/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32282: +  <int value="1" label="Selected
Autofill entry"/>
On 2014/06/04 21:24:47, Ilya Sherman wrote:
> Optional nit: I'd suggest replacing "Autofill" with "popup"

Done

[Ilya Sherman, 2014-06-04 22:21:44]

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

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:264: }
On 2014/06/04 21:21:03, Ilya Sherman wrote:
> On 2014/06/04 21:13:03, erikchen1 wrote:
> > On 2014/06/04 20:46:09, Ilya Sherman wrote:
> > > Hmm, is this extra work necessary?  The default value of the accessed pref
> is
> > > false, and the default value of the UseMacAddressBook pref is also false,
so
> > it
> > > seems like the result is the same either way.  Are you worried about the
> > > defaults changing and thereby breaking the new user case if you omit this
> > > explicit code?
> > 
> > I want to make sure that the migration code is called exactly once. If I
don't
> > manually write to the pref, then the migration code is going to be called on
> > every launch (which requires 3 pref checks), whereas this one time write
> ensures
> > that for all future launches, there is only 1 pref checked before the
> migration
> > is skipped.
> 
> I'm not following what you mean -- line 269 would always write to the pref,
> right?

^^^

https://codereview.chromium.org/301343002/diff/270001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right):

https://codereview.chromium.org/301343002/diff/270001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:119: // text needs
to be bumped up by 1 pt to make it look vertically aligned.
On 2014/06/04 22:11:12, erikchen1 wrote:
> On 2014/06/04 21:21:04, Ilya Sherman wrote:
> > nit: "1 pt" -> "1 pixel"?
> 
> I do mean point, and not pixel. It's the standard Cocoa unit of layout.
> 
> pt is a common abbreviation for point, which I prefer since it makes it clear
> that the 1 is a measurement. 
> 
> See figure 1-5.
>
https://developer.apple.com/library/mac/documentation/GraphicsAnimation/Conce...

Ah, right.  Sorry, I'm used to thinking of points as typographical concepts, not
screen metrics.  I keep forgetting that Cocoa draws its origins from PDF
rendering.

[erikchen, 2014-06-04 22:27:08]

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

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:264: }
On 2014/06/04 22:21:45, Ilya Sherman wrote:
> On 2014/06/04 21:21:03, Ilya Sherman wrote:
> > On 2014/06/04 21:13:03, erikchen1 wrote:
> > > On 2014/06/04 20:46:09, Ilya Sherman wrote:
> > > > Hmm, is this extra work necessary?  The default value of the accessed
pref
> > is
> > > > false, and the default value of the UseMacAddressBook pref is also
false,
> so
> > > it
> > > > seems like the result is the same either way.  Are you worried about the
> > > > defaults changing and thereby breaking the new user case if you omit
this
> > > > explicit code?
> > > 
> > > I want to make sure that the migration code is called exactly once. If I
> don't
> > > manually write to the pref, then the migration code is going to be called
on
> > > every launch (which requires 3 pref checks), whereas this one time write
> > ensures
> > > that for all future launches, there is only 1 pref checked before the
> > migration
> > > is skipped.
> > 
> > I'm not following what you mean -- line 269 would always write to the pref,
> > right?
> 
> ^^^

Ack, I misread your comment. Yes, I would like this code to not depend on the
default value of the accessed pref.

[Ilya Sherman, 2014-06-04 22:46:40]

LGTM % a final two nits.  Thanks, Erik!

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

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:242: DCHECK(pref);
nit: Since you use this variable immediately below, there's no need for this
DCHECK.  We generally only have DCHECKs for NULL pointers when the code that
uses them is temporally separated from the DCHECK, and hence the stack trace
from the NULL pointer crash would be insufficiently informative.

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:259: // from happening
again.
nit: Please move this comment into the if-stmt.  Generally comments outside of
if-stmts are of the form "Check for X", and within if-stmts are of the form "X
is the case, therefore Y".

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:264: }
On 2014/06/04 22:27:08, erikchen1 wrote:
> On 2014/06/04 22:21:45, Ilya Sherman wrote:
> > On 2014/06/04 21:21:03, Ilya Sherman wrote:
> > > On 2014/06/04 21:13:03, erikchen1 wrote:
> > > > On 2014/06/04 20:46:09, Ilya Sherman wrote:
> > > > > Hmm, is this extra work necessary?  The default value of the accessed
> pref
> > > is
> > > > > false, and the default value of the UseMacAddressBook pref is also
> false,
> > so
> > > > it
> > > > > seems like the result is the same either way.  Are you worried about
the
> > > > > defaults changing and thereby breaking the new user case if you omit
> this
> > > > > explicit code?
> > > > 
> > > > I want to make sure that the migration code is called exactly once. If I
> > don't
> > > > manually write to the pref, then the migration code is going to be
called
> on
> > > > every launch (which requires 3 pref checks), whereas this one time write
> > > ensures
> > > > that for all future launches, there is only 1 pref checked before the
> > > migration
> > > > is skipped.
> > > 
> > > I'm not following what you mean -- line 269 would always write to the
pref,
> > > right?
> > 
> > ^^^
> 
> Ack, I misread your comment. Yes, I would like this code to not depend on the
> default value of the accessed pref.

Hrm, alright.

[erikchen, 2014-06-04 22:50:05]

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

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:242: DCHECK(pref);
On 2014/06/04 22:46:41, Ilya Sherman wrote:
> nit: Since you use this variable immediately below, there's no need for this
> DCHECK.  We generally only have DCHECKs for NULL pointers when the code that
> uses them is temporally separated from the DCHECK, and hence the stack trace
> from the NULL pointer crash would be insufficiently informative.

Good to know, thanks! Done.

https://codereview.chromium.org/301343002/diff/230001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:259: // from happening
again.
On 2014/06/04 22:46:41, Ilya Sherman wrote:
> nit: Please move this comment into the if-stmt.  Generally comments outside of
> if-stmts are of the form "Check for X", and within if-stmts are of the form "X
> is the case, therefore Y".

Okay!

[erikchen, 2014-06-04 22:53:57]

blundell: Looking for OWNER review of components/resources
thestig: Looking for OWNER review of chrome/browser

[Lei Zhang, 2014-06-04 22:58:35]

https://codereview.chromium.org/301343002/diff/350001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/301343002/diff/350001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9667: <message
name="IDS_AUTOFILL_USE_MAC_ADDRESS_BOOK" desc="Checkbox label to enable access
to the user's Address Book on Mac. On Mac 10.8+ the application that manages the
Address Book is called Contacts.">
Curious, what percentage of users are still on 10.6 / 10.7? Should we have 2
strings here and select the right one at runtime?

https://codereview.chromium.org/301343002/diff/350001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/301343002/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:62: {
"macContactsIcon", IDR_AUTOFILL_MAC_CONTACTS_ICON },
Should this be OS_MACOSX only?

[erikchen, 2014-06-04 23:26:23]

https://codereview.chromium.org/301343002/diff/350001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/301343002/diff/350001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9667: <message
name="IDS_AUTOFILL_USE_MAC_ADDRESS_BOOK" desc="Checkbox label to enable access
to the user's Address Book on Mac. On Mac 10.8+ the application that manages the
Address Book is called Contacts.">
On 2014/06/04 22:58:36, Lei Zhang wrote:
> Curious, what percentage of users are still on 10.6 / 10.7? Should we have 2
> strings here and select the right one at runtime?

30% of users are on 10.6 and 10.7. I'm inclined to not perform runtime switching
of the resources (there are 2 string resources, and 1 image resource that would
need duplication for 10.6 & 10.7)

https://codereview.chromium.org/301343002/diff/350001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/301343002/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:62: {
"macContactsIcon", IDR_AUTOFILL_MAC_CONTACTS_ICON },
On 2014/06/04 22:58:36, Lei Zhang wrote:
> Should this be OS_MACOSX only?

Done.

[Lei Zhang, 2014-06-04 23:28:32]

lgtm

https://codereview.chromium.org/301343002/diff/350001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/301343002/diff/350001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9667: <message
name="IDS_AUTOFILL_USE_MAC_ADDRESS_BOOK" desc="Checkbox label to enable access
to the user's Address Book on Mac. On Mac 10.8+ the application that manages the
Address Book is called Contacts.">
On 2014/06/04 23:26:23, erikchen1 wrote:
> On 2014/06/04 22:58:36, Lei Zhang wrote:
> > Curious, what percentage of users are still on 10.6 / 10.7? Should we have 2
> > strings here and select the right one at runtime?
> 
> 30% of users are on 10.6 and 10.7. I'm inclined to not perform runtime
switching
> of the resources (there are 2 string resources, and 1 image resource that
would
> need duplication for 10.6 & 10.7)

Ok, your call.

[blundell, 2014-06-05 09:31:23]

lgtm

[erikchen, 2014-06-05 17:24:37]

isherman: PTAL. Looking for a short, final pass. While addressing one of your
comments, I broke the preference migration logic. Was implicitly casting from a
pointer to a bool :O

[Ilya Sherman, 2014-06-05 17:54:44]

LGTM, thanks.

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

https://codereview.chromium.org/301343002/diff/390001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:265: bool result =
pref_accessed->GetValue()->GetAsBoolean(&accessed);
Optional nit: I typically write this as "bool success = ..." rather than "bool
result = ...", but if you prefer "result", that's fine too.

[erikchen, 2014-06-05 17:58:10]

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

https://codereview.chromium.org/301343002/diff/390001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:265: bool result =
pref_accessed->GetValue()->GetAsBoolean(&accessed);
On 2014/06/05 17:54:43, Ilya Sherman wrote:
> Optional nit: I typically write this as "bool success = ..." rather than "bool
> result = ...", but if you prefer "result", that's fine too.

hm. "success" is more accurate. Done.

[erikchen, 2014-06-05 17:58:51]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-06-05 18:00:08]

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

[commit-bot: I haz the power, 2014-06-05 23:07:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-06 00:22:23]

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

[commit-bot: I haz the power, 2014-06-06 00:22:24]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[erikchen, 2014-06-06 00:48:56]

The CQ bit was checked by erikchen@chromium.org

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

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

[commit-bot: I haz the power, 2014-06-06 08:22:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-06 10:13:37]

Change committed as 275367