Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12282004:
Added personal_data_manager android implementation for auto-populating auto-fill on android builds … (Closed)
|
Created:
7 years, 10 months ago by apiccion Modified:
7 years, 9 months ago Reviewers:
David Trainor- moved to gerrit, Jói, Nico, nyquist, aurimas (slooooooooow), Yaron, darin (slow to review), Ilya Sherman CC:
chromium-reviews, Raman Kakilate, benquan, dhollowa+watch_chromium.org, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer, erikwright+watch_chromium.org, Ilya Sherman, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded personal_data_manager android implementation for auto-populating auto-fill on android builds
Enabled auxiliary autofill profile for Android builds. Fixed unit tests for android version for autofill. Disabled autofill address population until we can parse the info from the contacts.
Added permissions READ_CONTACTS and READ_PROFILE to testshell. These permissions are required to read the user's contact information using the ContactsContract and ContactsContract.Profile API.
TBR=thakis@chromium.org,joi@chromium.org
BUG=173268
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190842
Patch Set 1 #
Total comments: 77
Patch Set 2 : Android Autofill Populator #
Total comments: 22
Patch Set 3 : Rebased #
Total comments: 48
Patch Set 4 : Autofill nits #
Total comments: 128
Patch Set 5 : #Patch Set 6 : Fixed assortment of nits #
Total comments: 114
Patch Set 7 : Fixed various nits. #
Total comments: 46
Patch Set 8 : Fixed nits. #Patch Set 9 : Fixed Nits #
Total comments: 27
Patch Set 10 : Components Refactor + Nits #
Total comments: 16
Patch Set 11 : Autofill populator components refactor #
Total comments: 23
Patch Set 12 : Moved Test Code Back Into unit_tests #Patch Set 13 : Nits #
Total comments: 4
Patch Set 14 : nits #Patch Set 15 : Feature flag-guard #
Total comments: 1
Patch Set 16 : Replaced flag guard with run-time permission check. Removed testshell contacts+profile permissions. #Patch Set 17 : Added OS==android guard to autofill_jni_headers dependcy in autofill.gypi #Patch Set 18 : Added OS==android guard to autofill_jni_headers dependcy in autofill.gypi #Patch Set 19 : Changed Java PersonalAutofillPopulator @CalledByNative methods from public to private. #Patch Set 20 : Fixed autogenerated nits from trybot #Messages
Total messages: 48 (0 generated)
PTAL
On Fri, Feb 15, 2013 at 4:23 PM, <apiccion@chromium.org> wrote: > Reviewers: dtrainor, aurimas, > > Message: > PTAL > > Description: > Added personal_data_manager android implementation for auto-populating > auto-fill > on android builds Enabled auxiliary autofill profile for Android builds. > Fixed > unit tests for android version for autofill. Disabled autofill address > population until we can parse the info from the contacts. Added cscope.out > to > gitignore Updated gitignore to ignore all cscope.* files. > > c++ style guide > What is this? Can you remove it from the commit message? Otherwise make it a real sentence. > Touched files to meet our Java/C++ style guides. > I think that is irrelevant. As you should adhere to these style guides always and anyway. > BUG=173268 > > > Please review this at https://codereview.chromium.org/12282004/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > Affected files: > M .gitignore > A autofill > M base/basictypes.h > A > chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java > M chrome/browser/android/chrome_jni_registrar.cc > M chrome/browser/autofill/autofill_manager.cc > M chrome/browser/autofill/autofill_manager_unittest.cc > M chrome/browser/autofill/personal_data_manager.cc > A chrome/browser/autofill/personal_data_manager_android.h > A chrome/browser/autofill/personal_data_manager_android.cc > M chrome/chrome_browser.gypi > > > -- > You received this message because you are subscribed to the Google Groups > "browser-components-watch" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to browser-components-watch+unsubscribe@chromium.org. > To post to this group, send email to browser-components-watch@chromium.org. > For more options, visit > https://groups.google.com/a/chromium.org/groups/opt_out. > > -- Thiago
https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:5: // Populates data fields from Android contacts profile API (i.e. "me" contact). We do not use // style comments in Java that much. Move this to the /* */ style comment above the class definition https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:13: import android.app.Activity; There imports should be in alpha oder with no line breaks in between android.* https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:36: * Loads user profile information stored under the "Me" contact Put periods at the end of the end of your comments. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:40: SQL query definitions for obtaining specific profile information Missing a star for this line https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:52: @Override Should put an empty line above @Override https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:82: public static final int STREET = 0; Are these contants defined somewhere in Android so we would not have to redefine it? Maybe use DATA1, DATA2, DATA3, ... define in ContactsContract.java https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:143: /* Extracted data variables */ For one line comments use // https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:161: @param context a valid android context reference Missing a star. Update it for every /** */ comment. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:227: public String getFirstName() { Can't all of these methods be private since they are called from native code only? https://codereview.chromium.org/12282004/diff/1/chrome/browser/android/chrome... File chrome/browser/android/chrome_jni_registrar.cc (left): https://codereview.chromium.org/12282004/diff/1/chrome/browser/android/chrome... chrome/browser/android/chrome_jni_registrar.cc:5: #include "chrome/browser/android/chrome_jni_registrar.h" The matching *.h class always says on top. Should leave it as it was. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... File chrome/browser/autofill/personal_data_manager.cc (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager.cc:872: // Win and Linux implementations do nothing. Mac Why break the line is such a weird place? https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... File chrome/browser/autofill/personal_data_manager_android.cc (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:24: "9A9E1C06-7A3B-48FA-AA4F-135CA6FC25D9"; Where is this specified? https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:30: void LoadAddressWithPopulator(JNIEnv * env, Should all of these functions live in some sort of namespace? https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:59: ConvertJavaStringToUTF16(postalCode)); Align with ADDRESS_HOME_ZIP https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:62: ConvertJavaStringToUTF16(country)); Align with ADDRESS_HOME_COUNTRY https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:134: // LoadAddressWithPopulator(env, populator, profile.get()); Why is this like commented out? https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... File chrome/browser/autofill/personal_data_manager_android.h (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/12282004/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/12282004/diff/1/chrome/chrome_browser.gypi#ne... chrome/chrome_browser.gypi:251: 'browser/autofill/personal_data_manager_android.cc', Alpha order
What is that "/autofill" file? Should it be included in the patch? Can you add tests?
https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/autof... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager.cc:242: PrefRegistrySyncable::SYNCABLE_PREF); Can you ask nyquist@ if we sync these prefs for Android? https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/autof... File chrome/browser/autofill/autofill_manager_unittest.cc (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager_unittest.cc:2709: // Auxiliary profiles is implemented on Mac only. It enables Mac Address Do these tests pass for Android? Should this comment be updated? https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... File chrome/browser/autofill/personal_data_manager_android.h (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.h:13: Remove this empty line
https://codereview.chromium.org/12282004/diff/1/base/basictypes.h File base/basictypes.h (left): https://codereview.chromium.org/12282004/diff/1/base/basictypes.h#oldcode78 base/basictypes.h:78: // NOTE: The usage of this macro was baned from our code base, but some although correct, this is unrelated to this change. please, avoid doing unrelated changes in CLs.
https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:150: private String address; nit: Should this be mAddress? https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... File chrome/browser/autofill/personal_data_manager.cc (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager.cc:873: // and Android implementation fills in the contents of |auxiliary_profiles_|. nit: "implementation fills" -> "implementations fill" https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... File chrome/browser/autofill/personal_data_manager_android.cc (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:23: static const std::string ANDROID_ME_CONTACT_AUOTFILL_PROFILE_GUID = nit: This should be "const char kAndroidMeContactAutofillProfileGuid[] = ..." https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:32: AutofillProfile * profile) { nit: No space before the asterisk. (Applies throughout.) https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:50: ConvertJavaStringToUTF16(street)); Android addresses can't have address line 2 data? https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:53: ConvertJavaStringToUTF16(city)); nit: Curly braces, since this if-stmt's body spans multiple lines. (Applies throughout.) https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:86: profile->SetRawInfo(NAME_FULL, ConvertJavaStringToUTF16(fullName)); You should either set the first, middle, and last names; or set the full name; but not both. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:123: std::string guid(ANDROID_ME_CONTACT_AUOTFILL_PROFILE_GUID); nit: No need to make a copy here.
https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:82: public static final int STREET = 0; It looks like DATA1/DATA2/etc. are strings? I think these just correlate to which column in his query below (the String[] map) his result will be in. On 2013/02/15 19:08:35, aurimas wrote: > Are these contants defined somewhere in Android so we would not have to redefine > it? Maybe use DATA1, DATA2, DATA3, ... define in ContactsContract.java https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:86: public static final int REGION= 4; space before = https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:130: /** Takes a query object, transforms into actual query and returns cursor. I think we leave the line with /** blank and start the description on the second line for all /** comments. https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:172: private void populateName(ContentResolver contentResolver) { Should we have error handling here? I'm wondering what happens if no me contact is defined (is that possible?). If not, we should still add some kind of assert or check in these methods making sure we have a valid row. https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:175: nameCursor.moveToFirst(); Do you need this here? If so, should it be in every populate* method? https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:227: public String getFirstName() { On 2013/02/15 19:08:35, aurimas wrote: > Can't all of these methods be private since they are called from native code > only? Yeah they can. If they're not, they need javadocs! Maybe leave them public and add a test to make sure we're pulling the right data? https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/browser/autofil... File chrome/browser/autofill/autofill_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/browser/autofil... chrome/browser/autofill/autofill_manager_unittest.cc:2709: // Auxiliary profiles is implemented on Mac only. It enables Mac Address On 2013/02/15 19:16:13, aurimas wrote: > Do these tests pass for Android? Should this comment be updated? Fix comment to say Android as well if we're adding this here. https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/browser/autofil... File chrome/browser/autofill/personal_data_manager.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/browser/autofil... chrome/browser/autofill/personal_data_manager.cc:873: // and Android implementation fills in the contents of |auxiliary_profiles_|. On 2013/02/16 04:14:37, Ilya Sherman wrote: > nit: "implementation fills" -> "implementations fill" implementations fill? https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/browser/autofil... File chrome/browser/autofill/personal_data_manager_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/browser/autofil... chrome/browser/autofill/personal_data_manager_android.cc:128: Java_PersonalAutofillPopulator_create(env, I'm not sure about the formatting here. You might want to put env on a new line and base::android::GetApplicationContext() on the line below and line them up 4 in from the Java_ line (?). https://chromiumcodereview.appspot.com/12282004/diff/1/chrome/browser/autofil... chrome/browser/autofill/personal_data_manager_android.cc:134: // LoadAddressWithPopulator(env, populator, profile.get()); I think Address parsing isn't working. Can we just make the Java component return null for all of the fields (that should happen automatically anyway if we don't parse them right?) Then we can leave this in. On 2013/02/15 19:08:35, aurimas wrote: > Why is this like commented out?
https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/autof... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager.cc:242: PrefRegistrySyncable::SYNCABLE_PREF); On 2013/02/15 19:16:13, aurimas wrote: > Can you ask nyquist@ if we sync these prefs for Android? Android does not sync the preferences datatype. But I guess we need to register the pref for this to work regardless?
https://codereview.chromium.org/12282004/diff/1/base/basictypes.h File base/basictypes.h (left): https://codereview.chromium.org/12282004/diff/1/base/basictypes.h#oldcode78 base/basictypes.h:78: // NOTE: The usage of this macro was baned from our code base, but some On 2013/02/15 19:21:01, tfarina wrote: > although correct, this is unrelated to this change. please, avoid doing > unrelated changes in CLs. Done. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/15 19:08:35, aurimas wrote: > 2013 Done. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:5: // Populates data fields from Android contacts profile API (i.e. "me" contact). On 2013/02/15 19:08:35, aurimas wrote: > We do not use // style comments in Java that much. Move this to the /* */ style > comment above the class definition Done. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:13: import android.app.Activity; On 2013/02/15 19:08:35, aurimas wrote: > There imports should be in alpha oder with no line breaks in between android.* Done. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:36: * Loads user profile information stored under the "Me" contact On 2013/02/15 19:08:35, aurimas wrote: > Put periods at the end of the end of your comments. Done. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:40: SQL query definitions for obtaining specific profile information On 2013/02/15 19:08:35, aurimas wrote: > Missing a star for this line Done. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:52: @Override On 2013/02/15 19:08:35, aurimas wrote: > Should put an empty line above @Override Done. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:82: public static final int STREET = 0; Not defined in Android. They correspond to the order of entries in the projection array. On 2013/02/15 19:08:35, aurimas wrote: > Are these contants defined somewhere in Android so we would not have to redefine > it? Maybe use DATA1, DATA2, DATA3, ... define in ContactsContract.java https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:86: public static final int REGION= 4; On 2013/02/19 19:03:13, David Trainor wrote: > space before = Done. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:150: private String address; On 2013/02/16 04:14:37, Ilya Sherman wrote: > nit: Should this be mAddress? Actually dead code. Will remove. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:161: @param context a valid android context reference On 2013/02/15 19:08:35, aurimas wrote: > Missing a star. Update it for every /** */ comment. Done. https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:175: nameCursor.moveToFirst(); Actually a crash-bug because cursor could be empty. Either one of moveToFirst() or moveToNext() is sufficient. The cursor starts at index -1. On 2013/02/19 19:03:13, David Trainor wrote: > Do you need this here? If so, should it be in every populate* method? https://codereview.chromium.org/12282004/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:227: public String getFirstName() { I think the distinction of public/private should reflect the intent of the component interactions (e.g. object interactions). On 2013/02/15 19:08:35, aurimas wrote: > Can't all of these methods be private since they are called from native code > only? https://codereview.chromium.org/12282004/diff/1/chrome/browser/android/chrome... File chrome/browser/android/chrome_jni_registrar.cc (left): https://codereview.chromium.org/12282004/diff/1/chrome/browser/android/chrome... chrome/browser/android/chrome_jni_registrar.cc:5: #include "chrome/browser/android/chrome_jni_registrar.h" On 2013/02/15 19:08:35, aurimas wrote: > The matching *.h class always says on top. Should leave it as it was. Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/autof... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager.cc:242: PrefRegistrySyncable::SYNCABLE_PREF); On 2013/02/15 19:16:13, aurimas wrote: > Can you ask nyquist@ if we sync these prefs for Android? Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... File chrome/browser/autofill/personal_data_manager.cc (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager.cc:872: // Win and Linux implementations do nothing. Mac On 2013/02/15 19:08:35, aurimas wrote: > Why break the line is such a weird place? Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager.cc:873: // and Android implementation fills in the contents of |auxiliary_profiles_|. On 2013/02/16 04:14:37, Ilya Sherman wrote: > nit: "implementation fills" -> "implementations fill" Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... File chrome/browser/autofill/personal_data_manager_android.cc (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/15 19:08:35, aurimas wrote: > 2013 Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:23: static const std::string ANDROID_ME_CONTACT_AUOTFILL_PROFILE_GUID = On 2013/02/16 04:14:37, Ilya Sherman wrote: > nit: This should be "const char kAndroidMeContactAutofillProfileGuid[] = ..." Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:23: static const std::string ANDROID_ME_CONTACT_AUOTFILL_PROFILE_GUID = On 2013/02/16 04:14:37, Ilya Sherman wrote: > nit: This should be "const char kAndroidMeContactAutofillProfileGuid[] = ..." Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:24: "9A9E1C06-7A3B-48FA-AA4F-135CA6FC25D9"; Randomly generated guid. We need a consistent guid for the profile or else stuff breaks. On 2013/02/15 19:08:35, aurimas wrote: > Where is this specified? https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:30: void LoadAddressWithPopulator(JNIEnv * env, On 2013/02/15 19:08:35, aurimas wrote: > Should all of these functions live in some sort of namespace? Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:32: AutofillProfile * profile) { On 2013/02/16 04:14:37, Ilya Sherman wrote: > nit: No space before the asterisk. (Applies throughout.) Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:50: ConvertJavaStringToUTF16(street)); On 2013/02/16 04:14:37, Ilya Sherman wrote: > Android addresses can't have address line 2 data? Added function to build address line 2 from misc data. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:53: ConvertJavaStringToUTF16(city)); On 2013/02/16 04:14:37, Ilya Sherman wrote: > nit: Curly braces, since this if-stmt's body spans multiple lines. (Applies > throughout.) Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:59: ConvertJavaStringToUTF16(postalCode)); On 2013/02/15 19:08:35, aurimas wrote: > Align with ADDRESS_HOME_ZIP Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:62: ConvertJavaStringToUTF16(country)); On 2013/02/15 19:08:35, aurimas wrote: > Align with ADDRESS_HOME_COUNTRY Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:86: profile->SetRawInfo(NAME_FULL, ConvertJavaStringToUTF16(fullName)); On 2013/02/16 04:14:37, Ilya Sherman wrote: > You should either set the first, middle, and last names; or set the full name; > but not both. Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:123: std::string guid(ANDROID_ME_CONTACT_AUOTFILL_PROFILE_GUID); On 2013/02/16 04:14:37, Ilya Sherman wrote: > nit: No need to make a copy here. Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:128: Java_PersonalAutofillPopulator_create(env, On 2013/02/19 19:03:13, David Trainor wrote: > I'm not sure about the formatting here. You might want to put env on a new line > and base::android::GetApplicationContext() on the line below and line them up 4 > in from the Java_ line (?). Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.cc:134: // LoadAddressWithPopulator(env, populator, profile.get()); On 2013/02/19 19:03:13, David Trainor wrote: > I think Address parsing isn't working. Can we just make the Java component > return null for all of the fields (that should happen automatically anyway if we > don't parse them right?) Then we can leave this in. > > On 2013/02/15 19:08:35, aurimas wrote: > > Why is this like commented out? > Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... File chrome/browser/autofill/personal_data_manager_android.h (right): https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/15 19:08:35, aurimas wrote: > 2013 Done. https://codereview.chromium.org/12282004/diff/1/chrome/browser/autofill/perso... chrome/browser/autofill/personal_data_manager_android.h:13: On 2013/02/15 19:16:13, aurimas wrote: > Remove this empty line Done. https://codereview.chromium.org/12282004/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/12282004/diff/1/chrome/chrome_browser.gypi#ne... chrome/chrome_browser.gypi:251: 'browser/autofill/personal_data_manager_android.cc', On 2013/02/15 19:08:35, aurimas wrote: > Alpha order Done.
LGTM with a few nits. https://codereview.chromium.org/12282004/diff/13001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://codereview.chromium.org/12282004/diff/13001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:26: import android.widget.Toast; Need a space after this line. See http://source.android.com/source/code-style.html#order-import-statements https://codereview.chromium.org/12282004/diff/13001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:27: import java.util.ArrayList; java.* import should go after org.chromium and need spaces between these two groupings https://codereview.chromium.org/12282004/diff/13001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:105: public static final int GIVEN_NAME = 0; Does this need to be public? Is it used outside of PersonalAutofillPopulator class? https://codereview.chromium.org/12282004/diff/13001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:214: * Static factory method for instance creation It is generally nice to have periods at the end of your comments. https://codereview.chromium.org/12282004/diff/13001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:224: public String getFirstName() { Since all of these are public, you should add javadoc comments for them. https://codereview.chromium.org/12282004/diff/13001/chrome/browser/android/ch... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/12282004/diff/13001/chrome/browser/android/ch... chrome/browser/android/chrome_jni_registrar.cc:8: #include "chrome/browser/android/chrome_jni_registrar.h" This should be moved back up where it was. https://codereview.chromium.org/12282004/diff/13001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader_mock.h (right): https://codereview.chromium.org/12282004/diff/13001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_mock.h:16: void SetStreet(string16); This should be called set_street() since it is a simple setter. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/12282004/diff/13001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_mock.h:18: void SetPobox(string16); Same for all the Set... functions below. https://codereview.chromium.org/12282004/diff/13001/chrome/browser/autofill/i... File chrome/browser/autofill/i_auxiliary_profile_loader.cc (right): https://codereview.chromium.org/12282004/diff/13001/chrome/browser/autofill/i... chrome/browser/autofill/i_auxiliary_profile_loader.cc:5: #include "chrome/browser/autofill/i_auxiliary_profile_loader.h" Do we even need this .cc file? We should just keep .h if .cc has no use. https://codereview.chromium.org/12282004/diff/13001/chrome/browser/autofill/i... File chrome/browser/autofill/i_auxiliary_profile_loader.h (right): https://codereview.chromium.org/12282004/diff/13001/chrome/browser/autofill/i... chrome/browser/autofill/i_auxiliary_profile_loader.h:11: class IAuxiliaryProfileLoader { Why does this class have I in the beginning of its name? https://codereview.chromium.org/12282004/diff/13001/chrome/browser/autofill/i... chrome/browser/autofill/i_auxiliary_profile_loader.h:16: virtual string16 GetStreet() = 0; This should be called street() since it is a simple accessor. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/12282004/diff/13001/chrome/browser/autofill/i... chrome/browser/autofill/i_auxiliary_profile_loader.h:17: virtual string16 GetPobox() = 0; Same for all the functions below.
Looking good! Have a few more nits. Also look at the clang bot compile errors as they are basically style guide nits. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/and... File chrome/browser/android/chrome_jni_registrar.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/and... chrome/browser/android/chrome_jni_registrar.cc:8: #include "chrome/browser/android/chrome_jni_registrar.h" I think this should go back up top since it is the header for this specific file. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_impl_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:13: #include "chrome/browser/autofill/auxiliary_profile_impl_android.h" I think this should be at the top? https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:38: * Android user's profile contact does not prase its address prase -> parse https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:42: * LoadAddress(profile.get()); File a bug for this at crbug.com and add the bug number here. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:47: /* blank line between } and /* https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:56: accVector.push_back(pobox); For one line if statements don't add { } https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:58: if (!neighborhood.empty()) { Same https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:103: /* Trips NOTREACHED assertion in |NameInfo| due Just remove this if it's working as expected and we don't plan on using it. Otherwise link to the crbug number. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_impl_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:18: class AuxiliaryProfilesImpl { Do we need Impl on this if there isn't an interface? Can we call this AuxiliaryProfilesAndroid? Make profile -> profiles in the file name. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:20: AuxiliaryProfilesImpl(ScopedVector<AutofillProfile>* profiles, Can we move the implementation to the cc file and DCHECK on profiles and profileLoader? https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:33: private: Don't need another private here https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:34: ScopedVector<AutofillProfile>& profiles_; Might make sense to just store pointers here but I don't think it matters. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:35: IAuxiliaryProfileLoader& profileLoader_; I feel like this should be profile_loader_ but I forget if this form is also valid. Also applies to function parameter in constructor https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:23: AutofillProfile * profile = *profiles_.begin(); No space between AutofillProfile and *. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_loader_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_android.cc:17: string16 SafeJavaStringToUTF16(ScopedJavaLocalRef<jstring> jstring) { Can we put these into one of our jni android util files? Might be useful for other places as well :). https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_android.h:13: class AuxiliaryProfileLoaderAndroid : public IAuxiliaryProfileLoader { disallow copy and assign macro https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_android.h:17: // Address info Don't need these comments. Need something like: // IAuxiliaryProfileLoader implementation https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_android.h:18: virtual string16 GetStreet(); Add OVERRIDE to the end of every overridden method. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_loader_mock.h (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_mock.h:10: class AuxiliaryProfileLoaderMock : public IAuxiliaryProfileLoader { Disallow copy and assign macro https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_mock.h:15: virtual string16 GetStreet(); Add override to every overridden method. Might be cleaner to pull setters apart and have the IAuxiliaryProfileLoader implementation methods separate. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/i_auxiliary_profile_loader.cc (left): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/i_auxiliary_profile_loader.cc:2: // Use of this source code is governed by a BSD-style license that can be You don't need this file. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/i_auxiliary_profile_loader.h (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/i_auxiliary_profile_loader.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. I don't think we use i for an interface prefix (at least not much that I've seen). I think instead we do something like AuxiliaryProfileLoader -> AuxiliaryProfileLoaderImpl https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/chrome_brow... chrome/chrome_browser.gypi:3142: 'android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java', Why did some of these move?
https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:26: import android.widget.Toast; On 2013/02/27 01:56:00, aurimas wrote: > Need a space after this line. > > See http://source.android.com/source/code-style.html#order-import-statements Done. https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:27: import java.util.ArrayList; On 2013/02/27 01:56:00, aurimas wrote: > java.* import should go after org.chromium and need spaces between these two > groupings Done. https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:105: public static final int GIVEN_NAME = 0; On 2013/02/27 01:56:00, aurimas wrote: > Does this need to be public? Is it used outside of PersonalAutofillPopulator > class? Done. https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:224: public String getFirstName() { On 2013/02/27 01:56:00, aurimas wrote: > Since all of these are public, you should add javadoc comments for them. http://www.corp.google.com/eng/doc/javadocguide.html "Trivial get and set methods such as setFoo() do not require Javadoc if all the Javadoc would say is "sets Foo"." https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_loader_mock.h (right): https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_mock.h:16: void SetStreet(string16); Think this violates separation of interface from underlying implementation. It's overriding methods in an abstract interface (AuxiliaryProfileLoader/IAuxiliaryProfileLoader). Not sure. On 2013/02/27 01:56:00, aurimas wrote: > This should be called set_street() since it is a simple setter. > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_mock.h:18: void SetPobox(string16); On 2013/02/27 01:56:00, aurimas wrote: > Same for all the Set... functions below. See comment above. https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/browser/aut... File chrome/browser/autofill/i_auxiliary_profile_loader.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/browser/aut... chrome/browser/autofill/i_auxiliary_profile_loader.cc:5: #include "chrome/browser/autofill/i_auxiliary_profile_loader.h" On 2013/02/27 01:56:00, aurimas wrote: > Do we even need this .cc file? We should just keep .h if .cc has no use. Done. https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/browser/aut... File chrome/browser/autofill/i_auxiliary_profile_loader.h (right): https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/browser/aut... chrome/browser/autofill/i_auxiliary_profile_loader.h:11: class IAuxiliaryProfileLoader { Interface class. As per Dave's comment I've removed the I. On 2013/02/27 01:56:00, aurimas wrote: > Why does this class have I in the beginning of its name? https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/browser/aut... chrome/browser/autofill/i_auxiliary_profile_loader.h:16: virtual string16 GetStreet() = 0; Abstract implementation. Not necessarily a simple mutator. On 2013/02/27 01:56:00, aurimas wrote: > This should be called street() since it is a simple accessor. > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://chromiumcodereview.appspot.com/12282004/diff/13001/chrome/browser/aut... chrome/browser/autofill/i_auxiliary_profile_loader.h:17: virtual string16 GetPobox() = 0; See above. On 2013/02/27 01:56:00, aurimas wrote: > Same for all the functions below. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/and... File chrome/browser/android/chrome_jni_registrar.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/and... chrome/browser/android/chrome_jni_registrar.cc:8: #include "chrome/browser/android/chrome_jni_registrar.h" On 2013/02/27 07:49:28, David Trainor wrote: > I think this should go back up top since it is the header for this specific > file. Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_impl_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:13: #include "chrome/browser/autofill/auxiliary_profile_impl_android.h" On 2013/02/27 07:49:28, David Trainor wrote: > I think this should be at the top? Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:13: #include "chrome/browser/autofill/auxiliary_profile_impl_android.h" On 2013/02/27 07:49:28, David Trainor wrote: > I think this should be at the top? Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:38: * Android user's profile contact does not prase its address On 2013/02/27 07:49:28, David Trainor wrote: > prase -> parse Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:42: * LoadAddress(profile.get()); On 2013/02/27 07:49:28, David Trainor wrote: > File a bug for this at http://crbug.com and add the bug number here. Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:47: /* On 2013/02/27 07:49:28, David Trainor wrote: > blank line between } and /* Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:56: accVector.push_back(pobox); On 2013/02/27 07:49:28, David Trainor wrote: > For one line if statements don't add { } Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:58: if (!neighborhood.empty()) { On 2013/02/27 07:49:28, David Trainor wrote: > Same Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.cc:103: /* Trips NOTREACHED assertion in |NameInfo| due On 2013/02/27 07:49:28, David Trainor wrote: > Just remove this if it's working as expected and we don't plan on using it. > Otherwise link to the crbug number. Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_impl_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:18: class AuxiliaryProfilesImpl { On 2013/02/27 07:49:28, David Trainor wrote: > Do we need Impl on this if there isn't an interface? Can we call this > AuxiliaryProfilesAndroid? > > Make profile -> profiles in the file name. Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:18: class AuxiliaryProfilesImpl { On 2013/02/27 07:49:28, David Trainor wrote: > Do we need Impl on this if there isn't an interface? Can we call this > AuxiliaryProfilesAndroid? > > Make profile -> profiles in the file name. Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:20: AuxiliaryProfilesImpl(ScopedVector<AutofillProfile>* profiles, On 2013/02/27 07:49:28, David Trainor wrote: > Can we move the implementation to the cc file and DCHECK on profiles and > profileLoader? Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:33: private: On 2013/02/27 07:49:28, David Trainor wrote: > Don't need another private here Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:34: ScopedVector<AutofillProfile>& profiles_; On 2013/02/27 07:49:28, David Trainor wrote: > Might make sense to just store pointers here but I don't think it matters. Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_impl_android.h:35: IAuxiliaryProfileLoader& profileLoader_; Style guide says must be lowercase with the extra underscore as optional. Fixed. On 2013/02/27 07:49:28, David Trainor wrote: > I feel like this should be profile_loader_ but I forget if this form is also > valid. Also applies to function parameter in constructor https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_loader_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_android.cc:17: string16 SafeJavaStringToUTF16(ScopedJavaLocalRef<jstring> jstring) { On 2013/02/27 07:49:28, David Trainor wrote: > Can we put these into one of our jni android util files? Might be useful for > other places as well :). Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_android.h:13: class AuxiliaryProfileLoaderAndroid : public IAuxiliaryProfileLoader { On 2013/02/27 07:49:28, David Trainor wrote: > disallow copy and assign macro Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_android.h:13: class AuxiliaryProfileLoaderAndroid : public IAuxiliaryProfileLoader { On 2013/02/27 07:49:28, David Trainor wrote: > disallow copy and assign macro Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_android.h:17: // Address info On 2013/02/27 07:49:28, David Trainor wrote: > Don't need these comments. Need something like: > // IAuxiliaryProfileLoader implementation Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_android.h:18: virtual string16 GetStreet(); On 2013/02/27 07:49:28, David Trainor wrote: > Add OVERRIDE to the end of every overridden method. Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/auxiliary_profile_loader_mock.h (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_mock.h:10: class AuxiliaryProfileLoaderMock : public IAuxiliaryProfileLoader { On 2013/02/27 07:49:28, David Trainor wrote: > Disallow copy and assign macro Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/auxiliary_profile_loader_mock.h:15: virtual string16 GetStreet(); On 2013/02/27 07:49:28, David Trainor wrote: > Add override to every overridden method. Might be cleaner to pull setters apart > and have the IAuxiliaryProfileLoader implementation methods separate. Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/i_auxiliary_profile_loader.cc (left): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/i_auxiliary_profile_loader.cc:2: // Use of this source code is governed by a BSD-style license that can be On 2013/02/27 07:49:28, David Trainor wrote: > You don't need this file. Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... File chrome/browser/autofill/i_auxiliary_profile_loader.h (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/browser/aut... chrome/browser/autofill/i_auxiliary_profile_loader.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/02/27 07:49:28, David Trainor wrote: > I don't think we use i for an interface prefix (at least not much that I've > seen). I think instead we do something like AuxiliaryProfileLoader -> > AuxiliaryProfileLoaderImpl Done. https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/15018/chrome/chrome_brow... chrome/chrome_browser.gypi:3142: 'android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java', On 2013/02/27 07:49:28, David Trainor wrote: > Why did some of these move? vim :sort is case-sensitive (capital before lowercase). I guess we want case-insensitive sort?
https://codereview.chromium.org/12282004/diff/29001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://codereview.chromium.org/12282004/diff/29001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:35: * Loads user profile information stored under the "Me" contact. Does this require any permissions? If so, please include them here. https://codereview.chromium.org/12282004/diff/29001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:54: return new String[] { nit indentation is off throughout. We use the android style guide which is 4 spaces https://codereview.chromium.org/12282004/diff/29001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:103: public String mimeType() { @Override (some other places below too) https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_impl_unittest.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:15: class TestContext { I think it's more common to instead subclass testing::test with your fixture and use TEST_F. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:34: TEST(AuxiliaryProfilesAndroid, SetNameInfo) { Is this Android specific code? If so, it needs android in the filename or it'll get included in other ports (windows/linux/etc) and fail there. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... File chrome/browser/autofill/personal_data_manager.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... chrome/browser/autofill/personal_data_manager.cc:861: #if !defined(OS_MACOSX) && !defined(OS_ANDROID) The comment in the header would need an update too. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... File chrome/browser/autofill/personal_data_manager_android.h (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... chrome/browser/autofill/personal_data_manager_android.h:10: bool RegisterPersonalDataManager(JNIEnv* env); Where is this implemented? Typically it should be in the cc file and called by the appropriate jni_registrar. I don't see it called there. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... File chrome/browser/autofill/personal_data_manager_mac.mm (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... chrome/browser/autofill/personal_data_manager_mac.mm:33: // in the "me" card. This is awkward. We shouldn't have "Android" in mm files.
+darin Can you check off the base/ and chrome/ files (both gypi). https://codereview.chromium.org/12282004/diff/29001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://codereview.chromium.org/12282004/diff/29001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:35: * Loads user profile information stored under the "Me" contact. On 2013/02/28 20:40:43, Yaron wrote: > Does this require any permissions? If so, please include them here. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:54: return new String[] { On 2013/02/28 20:40:43, Yaron wrote: > nit indentation is off throughout. We use the android style guide which is 4 > spaces Done. https://codereview.chromium.org/12282004/diff/29001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:103: public String mimeType() { On 2013/02/28 20:40:43, Yaron wrote: > @Override (some other places below too) Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_impl_unittest.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:15: class TestContext { On 2013/02/28 20:40:43, Yaron wrote: > I think it's more common to instead subclass testing::test with your fixture and > use TEST_F. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:34: TEST(AuxiliaryProfilesAndroid, SetNameInfo) { On 2013/02/28 20:40:43, Yaron wrote: > Is this Android specific code? If so, it needs android in the filename or it'll > get included in other ports (windows/linux/etc) and fail there. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... File chrome/browser/autofill/personal_data_manager.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... chrome/browser/autofill/personal_data_manager.cc:861: #if !defined(OS_MACOSX) && !defined(OS_ANDROID) On 2013/02/28 20:40:43, Yaron wrote: > The comment in the header would need an update too. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... File chrome/browser/autofill/personal_data_manager_android.h (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... chrome/browser/autofill/personal_data_manager_android.h:10: bool RegisterPersonalDataManager(JNIEnv* env); Dead code. Removed this file completely. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... File chrome/browser/autofill/personal_data_manager_mac.mm (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/p... chrome/browser/autofill/personal_data_manager_mac.mm:33: // in the "me" card. On 2013/02/28 20:40:43, Yaron wrote: > This is awkward. We shouldn't have "Android" in mm files. Oops!
Please wait for less coarse OWNERS reviews prior to asking for .gypi reviews, since most .gypi OWNERS' L.G.T.M.'s will also inadvertently apply to subdirectories that the reviewers did not think they were supposed to be reviewing. https://codereview.chromium.org/12282004/diff/29001/.gitignore File .gitignore (right): https://codereview.chromium.org/12282004/diff/29001/.gitignore#newcode40 .gitignore:40: Session.vim nit: Please move these into a separate CL. https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... File base/android/jni_local_ref_extensions.cc (right): https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... base/android/jni_local_ref_extensions.cc:10: string16 SafeJavaStringToUTF16(ScopedJavaLocalRef<jstring> jstring) { nit: Please make the order of definitions in the implementation file match the order of declarations in the header file. https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... base/android/jni_local_ref_extensions.cc:15: } nit: Please write this function like so: if (jstring.is_null()) return string16(); return ConvertJavaStringToUTF16(jstring); https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... File base/android/jni_local_ref_extensions.h (right): https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... base/android/jni_local_ref_extensions.h:15: std::vector<string16> SafeJavaStringArrayToStringVector( Returning a vector by value is generally frowned upon, as it introduces a potentially expensive copy. Instead, please pass in the vector as an out-param. https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... base/android/jni_local_ref_extensions.h:15: std::vector<string16> SafeJavaStringArrayToStringVector( nit: Docs. https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... base/android/jni_local_ref_extensions.h:15: std::vector<string16> SafeJavaStringArrayToStringVector( Honestly, this function doesn't seem like it belongs in base. Why is it an onerous burden for callers to null-check their objects? https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... base/android/jni_local_ref_extensions.h:19: string16 SafeJavaStringToUTF16(ScopedJavaLocalRef<jstring> jstring); nit: Docs. https://codereview.chromium.org/12282004/diff/29001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://codereview.chromium.org/12282004/diff/29001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:54: return new String[] { On 2013/03/01 01:04:34, apiccion wrote: > On 2013/02/28 20:40:43, Yaron wrote: > > nit indentation is off throughout. We use the android style guide which is 4 > > spaces > > Done. Done where? Did you mean to upload a new patch set? https://codereview.chromium.org/12282004/diff/29001/chrome/android/testshell/... File chrome/android/testshell/java/AndroidManifest.xml (right): https://codereview.chromium.org/12282004/diff/29001/chrome/android/testshell/... chrome/android/testshell/java/AndroidManifest.xml:81: nit: Remove this newline. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_impl_unittest.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:3: // found in the LICENSE file. This file should probably be named "auxiliary_profile_unittest.cc", since there's no AuxiliaryProfileImpl class. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:21: (AuxiliaryProfileLoader*)&profile_loader_); nit: Indentation is off here. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:21: (AuxiliaryProfileLoader*)&profile_loader_); nit: Always prefer static_cast<> to C-style casts. However, it doesn't seem like you should need a cast here at all. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:27: AuxiliaryProfileLoaderMock profile_loader_; Member variables should never be public. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader.h (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:6: #define CHROME_BROWSER_AUTOFILL_AUXILIARY_PROFILE_LOADER_H_ Is this class used for any platforms other than Android? If not, please move it to an android/ subdirectory, and make sure that it's only compiled on that platform. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:8: #include <vector> nit: Leave a blank line after this one. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:11: class AuxiliaryProfileLoader { nit: Docs. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:11: class AuxiliaryProfileLoader { nit: This class should be in the autofill:: namespace. This comment applies to the contents of all the new header files introduced in this CL. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:15: // Address info nit: Please include docs for each method in this class. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:16: virtual string16 GetStreet() = 0; nit: Should this and all of the other Get() methods be const? https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:17: virtual string16 GetPobox() = 0; nit: Please spell out "GetPostOfficeBox()" https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:18: virtual string16 GetNeighborhood() = 0; nit: I have no idea what this method returns. This is why per-method docs are useful. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:31: virtual std::vector<string16> GetEmailAddresses() = 0; nit: Please make this method signature be virtual void GetEmailAddresses(std::vector<string16>* email_addresses) = 0; https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:34: virtual std::vector<string16> GetPhoneNumbers() = 0; nit: Ditto: use an outparam rather than returning a vector. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc:7: #include <vector> nit: Leave a blank line after this one. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc:15: populator_.obj()) nit: Please don't use abbreviations in code. If you really want to define a macro like this, at least name it something like JAVA_METHOD. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc:17: using namespace base::android; nit: "using namespace" is disallowed by the style guide. Something like "using base::android::SomeAndroidOnlyClass" is allowed, though. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc:26: base::android::GetApplicationContext()); nit: The indentation is off here. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc:26: base::android::GetApplicationContext()); Any reason why you can't do these initializations in the initializer list? https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader_impl_android.h (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:11: bool RegisterAuxiliaryProfileLoader(JNIEnv* env); nit: Docs. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:13: class AuxiliaryProfileLoaderImplAndroid : public AuxiliaryProfileLoader { Why have the parent class if there's only one implementation class? https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:13: class AuxiliaryProfileLoaderImplAndroid : public AuxiliaryProfileLoader { nit: Docs. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:15: explicit AuxiliaryProfileLoaderImplAndroid(); nit: No need for explicit. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:15: explicit AuxiliaryProfileLoaderImplAndroid(); nit: Include an explicitly defined destructor. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:33: base::android::ScopedJavaLocalRef<jobject> populator_; nit: Docs. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:34: JNIEnv* env_; nit: Docs. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader_mock.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_mock.cc:16: } nit: Please make sure the order of the definitions in the implementation file matches the order of the declarations in the header file. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader_mock.h (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_mock.h:10: class AuxiliaryProfileLoaderMock : public AuxiliaryProfileLoader { nit: This class should be named "TestAuxiliaryProfileLoader" https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_mock.h:12: explicit AuxiliaryProfileLoaderMock(); nit: No need for explicit. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_mock.h:12: explicit AuxiliaryProfileLoaderMock(); nit: Include an explicitly defined destructor. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profiles_android.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:8: #include <vector> nit: Leave a blank line after this one. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:25: */ nit: Use "// " for comments, not "/* ... */" https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:27: static const char kAndroidMeContactA[] = "9A9E1C06-7A3B-48FA-AA4F-135CA6FC25D9"; nit: Docs. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:27: static const char kAndroidMeContactA[] = "9A9E1C06-7A3B-48FA-AA4F-135CA6FC25D9"; nit: Omit "static" https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:31: AuxiliaryProfileLoader* profileLoader) nit: Indentation is off. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profiles_android.h (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:8: #include <jni.h> nit: Please leave a blank line after this one. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:9: #include "base/guid.h" nit: This doesn't seem to be used. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:10: #include "base/logging.h" nit: This doesn't seem to be used. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:11: #include "base/memory/scoped_ptr.h" nit: This doesn't seem to be used. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:14: #include "base/utf_string_conversions.h" nit: This doesn't seem to be used. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:15: #include "chrome/browser/autofill/autofill_profile.h" nit: Forward-declare. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:16: #include "chrome/browser/autofill/auxiliary_profile_loader.h" nit: Forward-declare. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:18: class AuxiliaryProfilesAndroid { nit: Docs. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:20: AuxiliaryProfilesAndroid(ScopedVector<AutofillProfile>* profiles, You should never pass in a ScopedVector to a method. This should almost certainly be a "const std::vector<AutofillProfile>& profiles". https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:20: AuxiliaryProfilesAndroid(ScopedVector<AutofillProfile>* profiles, nit: Docs. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:23: void GetContactsProfile(); nit: Docs. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:27: string16 neighborhood); nit: Pass strings by const-reference, not by value. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:27: string16 neighborhood); nit: Docs. (Applies throughout.) https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:28: void LoadAddress(AutofillProfile* profile); nit: Pass by const-reference? https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:34: AuxiliaryProfileLoader& profile_loader_; You should pretty much never store non-const references. Either create copies or store as pointers, depending on what's appropriate.
https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... File base/android/jni_local_ref_extensions.cc (right): https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... base/android/jni_local_ref_extensions.cc:10: string16 SafeJavaStringToUTF16(ScopedJavaLocalRef<jstring> jstring) { On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Please make the order of definitions in the implementation file match the > order of declarations in the header file. Done. https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... base/android/jni_local_ref_extensions.cc:15: } On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Please write this function like so: > > if (jstring.is_null()) > return string16(); > > return ConvertJavaStringToUTF16(jstring); Done. https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... File base/android/jni_local_ref_extensions.h (right): https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... base/android/jni_local_ref_extensions.h:15: std::vector<string16> SafeJavaStringArrayToStringVector( On 2013/03/01 01:55:04, Ilya Sherman wrote: > Honestly, this function doesn't seem like it belongs in base. Why is it an > onerous burden for callers to null-check their objects? It bloats the code. Had it as a private method in the calling file but was asked to move this outside. I don't know where else to put this. https://codereview.chromium.org/12282004/diff/29001/base/android/jni_local_re... base/android/jni_local_ref_extensions.h:19: string16 SafeJavaStringToUTF16(ScopedJavaLocalRef<jstring> jstring); On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Docs. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_impl_unittest.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:3: // found in the LICENSE file. On 2013/03/01 01:55:04, Ilya Sherman wrote: > This file should probably be named "auxiliary_profile_unittest.cc", since > there's no AuxiliaryProfileImpl class. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:21: (AuxiliaryProfileLoader*)&profile_loader_); On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Always prefer static_cast<> to C-style casts. However, it doesn't seem > like you should need a cast here at all. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_impl_unittest.cc:27: AuxiliaryProfileLoaderMock profile_loader_; On 2013/03/01 01:55:04, Ilya Sherman wrote: > Member variables should never be public. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader.h (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:6: #define CHROME_BROWSER_AUTOFILL_AUXILIARY_PROFILE_LOADER_H_ On 2013/03/01 01:55:04, Ilya Sherman wrote: > Is this class used for any platforms other than Android? If not, please move it > to an android/ subdirectory, and make sure that it's only compiled on that > platform. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:8: #include <vector> On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Leave a blank line after this one. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:11: class AuxiliaryProfileLoader { On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: This class should be in the autofill:: namespace. This comment applies to > the contents of all the new header files introduced in this CL. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:15: // Address info On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Please include docs for each method in this class. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:15: // Address info On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Please include docs for each method in this class. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:16: virtual string16 GetStreet() = 0; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Should this and all of the other Get() methods be const? Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:16: virtual string16 GetStreet() = 0; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Should this and all of the other Get() methods be const? Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:17: virtual string16 GetPobox() = 0; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Please spell out "GetPostOfficeBox()" Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:18: virtual string16 GetNeighborhood() = 0; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: I have no idea what this method returns. This is why per-method docs are > useful. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:31: virtual std::vector<string16> GetEmailAddresses() = 0; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Please make this method signature be > > virtual void GetEmailAddresses(std::vector<string16>* email_addresses) = 0; Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader.h:34: virtual std::vector<string16> GetPhoneNumbers() = 0; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Ditto: use an outparam rather than returning a vector. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc:7: #include <vector> On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Leave a blank line after this one. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc:15: populator_.obj()) On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Please don't use abbreviations in code. If you really want to define a > macro like this, at least name it something like JAVA_METHOD. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc:17: using namespace base::android; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: "using namespace" is disallowed by the style guide. Something like "using > base::android::SomeAndroidOnlyClass" is allowed, though. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc:26: base::android::GetApplicationContext()); On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: The indentation is off here. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.cc:26: base::android::GetApplicationContext()); On 2013/03/01 01:55:04, Ilya Sherman wrote: > Any reason why you can't do these initializations in the initializer list? Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader_impl_android.h (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:11: bool RegisterAuxiliaryProfileLoader(JNIEnv* env); On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Docs. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:13: class AuxiliaryProfileLoaderImplAndroid : public AuxiliaryProfileLoader { It's an abstract interface. On 2013/03/01 01:55:04, Ilya Sherman wrote: > Why have the parent class if there's only one implementation class? https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:15: explicit AuxiliaryProfileLoaderImplAndroid(); On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Include an explicitly defined destructor. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:33: base::android::ScopedJavaLocalRef<jobject> populator_; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Docs. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_impl_android.h:34: JNIEnv* env_; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Docs. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader_mock.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_mock.cc:16: } On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Please make sure the order of the definitions in the implementation file > matches the order of the declarations in the header file. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profile_loader_mock.h (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_mock.h:10: class AuxiliaryProfileLoaderMock : public AuxiliaryProfileLoader { On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: This class should be named "TestAuxiliaryProfileLoader" Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_mock.h:12: explicit AuxiliaryProfileLoaderMock(); On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: No need for explicit. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profile_loader_mock.h:12: explicit AuxiliaryProfileLoaderMock(); On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Include an explicitly defined destructor. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profiles_android.cc (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:8: #include <vector> On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Leave a blank line after this one. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:8: #include <vector> On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Leave a blank line after this one. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:25: */ On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Use "// " for comments, not "/* ... */" Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:27: static const char kAndroidMeContactA[] = "9A9E1C06-7A3B-48FA-AA4F-135CA6FC25D9"; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Omit "static" Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:27: static const char kAndroidMeContactA[] = "9A9E1C06-7A3B-48FA-AA4F-135CA6FC25D9"; On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Docs. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.cc:31: AuxiliaryProfileLoader* profileLoader) On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Indentation is off. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/auxiliary_profiles_android.h (right): https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:8: #include <jni.h> On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:9: #include "base/guid.h" On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: This doesn't seem to be used. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:10: #include "base/logging.h" On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: This doesn't seem to be used. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:11: #include "base/memory/scoped_ptr.h" On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: This doesn't seem to be used. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:14: #include "base/utf_string_conversions.h" On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: This doesn't seem to be used. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:15: #include "chrome/browser/autofill/autofill_profile.h" On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Forward-declare. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:16: #include "chrome/browser/autofill/auxiliary_profile_loader.h" On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Forward-declare. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:18: class AuxiliaryProfilesAndroid { On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Docs. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:18: class AuxiliaryProfilesAndroid { On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Docs. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:20: AuxiliaryProfilesAndroid(ScopedVector<AutofillProfile>* profiles, On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Docs. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:23: void GetContactsProfile(); On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Docs. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:27: string16 neighborhood); On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Pass strings by const-reference, not by value. Done. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:28: void LoadAddress(AutofillProfile* profile); On 2013/03/01 01:55:04, Ilya Sherman wrote: > nit: Pass by const-reference? These methods mutate their argument. https://codereview.chromium.org/12282004/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/auxiliary_profiles_android.h:34: AuxiliaryProfileLoader& profile_loader_; On 2013/03/01 01:55:04, Ilya Sherman wrote: > You should pretty much never store non-const references. Either create copies > or store as pointers, depending on what's appropriate. Done.
https://chromiumcodereview.appspot.com/12282004/diff/29001/base/android/jni_l... File base/android/jni_local_ref_extensions.h (right): https://chromiumcodereview.appspot.com/12282004/diff/29001/base/android/jni_l... base/android/jni_local_ref_extensions.h:15: std::vector<string16> SafeJavaStringArrayToStringVector( On 2013/03/02 03:37:01, apiccion wrote: > On 2013/03/01 01:55:04, Ilya Sherman wrote: > > Honestly, this function doesn't seem like it belongs in base. Why is it an > > onerous burden for callers to null-check their objects? > > It bloats the code. > Had it as a private method in the calling file but was asked to move this > outside. I don't know where else to put this. > Ok, I'll leave that up to base/android OWNERS. https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... File base/android/jni_local_ref_extensions.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... base/android/jni_local_ref_extensions.cc:20: std::vector<string16>* stringVector) { nit: Indent these three lines two more spaces each. https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... base/android/jni_local_ref_extensions.cc:20: std::vector<string16>* stringVector) { nit: Use hacker_case, not camelCase. https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... File base/android/jni_local_ref_extensions.h (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... base/android/jni_local_ref_extensions.h:15: // Returns empty string if java string is null, nit: "Returns and empty string if the Java string is null," https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... base/android/jni_local_ref_extensions.h:19: // Checks if java array is null and copies contents into stringVector nit: Perhaps something like "Fills |string_vector| with the contents of |jarray|. If the Java array is null, then |string_vector| is not modified." https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... base/android/jni_local_ref_extensions.h:23: std::vector<string16>* stringVector); nit: Use hacker_case, not camelCase. https://chromiumcodereview.appspot.com/12282004/diff/39002/base/base.gypi File base/base.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/base/base.gypi#new... base/base.gypi:44: 'android/scoped_java_ref.h', Did you mean to remove these? https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/android/tes... File chrome/android/testshell/java/AndroidManifest.xml (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/android/tes... chrome/android/testshell/java/AndroidManifest.xml:81: nit: Remove this newline. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/OWNERS (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/OWNERS:1: apiccion@chromium.org Non-committers can't approve CLs, so it doesn't make sense for you to be in this OWNERS file. I don't think this directory needs a separate OWNERS file for now. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:16: // contact information. Why do you need an abstract interface for this? That is, why not rename AuxiliaryProfileLoaderImplAndroid to AuxiliaryProfileLoaderAndroid and just use that class name everywhere? https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:16: // contact information. nit: Comment goes above the class. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:46: virtual void GetEmailAddresses(std::vector<string16>*) const = 0; nit: Parameters should always be named. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:49: virtual void GetPhoneNumbers(std::vector<string16>*) const = 0; nit: Parameters should always be named. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:26: : env_(base::android::AttachCurrentThread()), nit: Indent this two more spaces. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:27: populator_(Java_PersonalAutofillPopulator_create(env_, nit: Indent this four more spaces. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:28: base::android::GetApplicationContext())) {} nit: This should either be on the same line as |env_|, or be aligned with it. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:35: return SafeJavaStringToUTF16(JAVA_METHOD(getStreet)); Did you mean to add "using base::android::SafeJavaStringToUTF16" at the top of the file? https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:81: std::vector<string16>* strVector) const { nit: "strVector" -> "emails" https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:89: std::vector<string16>* strVector) const { nit: "strVector" -> "phone_numbers" https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.h:11: bool RegisterAuxiliaryProfileLoader(JNIEnv* env); nit: This should be defined within the "autofill" namespace. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.h:18: // |PersonalAutofillPopulator|. nit: Comments go above the class. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.h:20: explicit AuxiliaryProfileLoaderImplAndroid(); nit: No need for explicit. 'explicit' is only needed for 1-arg constructors. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:14: namespace { nit: The test fixture class itself should not be in the anonymous namespace; otherwise, it's possible to end up with identically-named test fixtures in different files that fail with strange linker errors. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:18: AuxiliaryProfileAndroidTest() : profile_loader_( nit: Break the line so that the colon is on the next line. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:33: std::vector<AutofillProfile*> profiles_; Shouldn't this be a ScopedVector? Who owns the memory for the profiles in this vector? https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:55: AutofillProfile * profile = GetAndLoadProfile(); nit: No space before the asterisk https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:109: * nit: Use "// "-style comments. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profiles_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:27: // Randomly generated guid. Autofillprofile requires a consistent unique guid nit: "Autofillprofile" -> "The AutofillProfile class" https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:35: : profile_loader_(profileLoader) { nit: Please fix the indentation here. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:42: // constructed from profileLoader. nit: Comments like this belong in the header file, rather than in the implementation file. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:44: std::vector<AutofillProfile*>* profiles) { nit: Fix indentation. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:45: DCHECK(profiles); nit: No need for this. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:57: // Chromium issue id # 178838 nit: "http://crbug.com/178838" https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:68: std::vector<string16> accVector; nit: hacker_case, spell out whatever "acc" is short for (or rename the variable to something like "parts") https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:69: if (!pobox.empty()) accVector.push_back(pobox); nit: Move the body of the if-stmt onto a separate line. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:70: if (!neighborhood.empty()) accVector.push_back(neighborhood); nit: Move the body of the if-stmt onto a separate line. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:74: } else { nit: No need for an else stmt after a return. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:75: string16 acc; nit: Spell out whatever "acc" is short for https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:82: } Can all of this code be replaced by just calling base::JoinString()? https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:87: string16 pobox = profile_loader_.GetPostOfficeBox(); nit: Spell out "pobox" https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:90: string16 postalCode = profile_loader_.GetPostalCode(); nit: hacker_case https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:107: string16 lastName = profile_loader_.GetLastName(); nit: hacker_case https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:115: std::vector<string16> emailsVector; nit: Just name this "emails" https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:121: std::vector<string16> phoneNumbersVector; nit: Name this "phone_numbers" https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profiles_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:21: } // namespace nit: Write this as: namespace autofill { class AuxiliaryProfileLoader; } // namespace (no newlines, no indentation, two spaces before the comment) https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:25: class AuxiliaryProfilesAndroid { Why do you need this class to be separate from the AuxiliaryProfileLoader class? It seems like you could just include a GetAutofillProfile() method in the latter class's API. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:30: // profile and injected into the vector. nit: Comment belongs above the class. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:33: // Constructor takes in an AuxiliaryProfileLoader object whicht provides nit: "whicht" -> "which" https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:33: // Constructor takes in an AuxiliaryProfileLoader object whicht provides nit: Drop "Constructor" https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:36: const autofill::AuxiliaryProfileLoader& profileLoader); nit: Use hacker_case. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:41: void LoadContactsProfile(std::vector<AutofillProfile*>* profiles); Why doesn't this method just return a scoped_ptr<AutofillProfile>? https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:46: string16 CollapseAddress(const string16& pobox, nit: Spell out post_office_box. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:47: const string16& neighborhood); nit: Can this be tucked into the anonymous namespace in the implementation file? https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:48: // inserts contact's address data into profile. nit: Start the comment with an uppercase letter. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/personal_data_manager_android.cc:7: #include <vector> nit: This doesn't seem to be used. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/personal_data_manager_android.cc:8: #include "base/logging.h" nit: This doesn't seem to be used. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/personal_data_manager_android.cc:9: #include "chrome/browser/autofill/android/auxiliary_profile_loader_android.h" nit: This doesn't seem to be used. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:62: std::vector<string16> phoneNumbers_; nit: hacker_case for all of these. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/chrome_brow... chrome/chrome_browser.gypi:73: 'browser/accessibility/accessibility_events.h', This doesn't seem to be related to your CL. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/chrome_brow... chrome/chrome_browser.gypi:128: 'browser/auto_launch_trial.cc', This doesn't seem to be related to your CL.
https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... File base/android/jni_local_ref_extensions.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... base/android/jni_local_ref_extensions.cc:20: std::vector<string16>* stringVector) { On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Indent these three lines two more spaces each. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... base/android/jni_local_ref_extensions.cc:20: std::vector<string16>* stringVector) { On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Indent these three lines two more spaces each. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... File base/android/jni_local_ref_extensions.h (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... base/android/jni_local_ref_extensions.h:15: // Returns empty string if java string is null, On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: "Returns and empty string if the Java string is null," Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... base/android/jni_local_ref_extensions.h:19: // Checks if java array is null and copies contents into stringVector On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Perhaps something like "Fills |string_vector| with the contents of > |jarray|. If the Java array is null, then |string_vector| is not modified." Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/base/android/jni_l... base/android/jni_local_ref_extensions.h:23: std::vector<string16>* stringVector); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Use hacker_case, not camelCase. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/android/tes... File chrome/android/testshell/java/AndroidManifest.xml (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/android/tes... chrome/android/testshell/java/AndroidManifest.xml:81: On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Remove this newline. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/OWNERS (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/OWNERS:1: apiccion@chromium.org On 2013/03/05 00:42:38, Ilya Sherman wrote: > Non-committers can't approve CLs, so it doesn't make sense for you to be in this > OWNERS file. I don't think this directory needs a separate OWNERS file for now. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:16: // contact information. On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Comment goes above the class. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:46: virtual void GetEmailAddresses(std::vector<string16>*) const = 0; On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Parameters should always be named. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:49: virtual void GetPhoneNumbers(std::vector<string16>*) const = 0; On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Parameters should always be named. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:26: : env_(base::android::AttachCurrentThread()), On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Indent this two more spaces. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:28: base::android::GetApplicationContext())) {} On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: This should either be on the same line as |env_|, or be aligned with it. Can't without going over 80 chars. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:35: return SafeJavaStringToUTF16(JAVA_METHOD(getStreet)); On 2013/03/05 00:42:38, Ilya Sherman wrote: > Did you mean to add "using base::android::SafeJavaStringToUTF16" at the top of > the file? Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:81: std::vector<string16>* strVector) const { On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: "strVector" -> "emails" Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.cc:89: std::vector<string16>* strVector) const { On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: "strVector" -> "phone_numbers" Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.h:11: bool RegisterAuxiliaryProfileLoader(JNIEnv* env); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: This should be defined within the "autofill" namespace. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.h:11: bool RegisterAuxiliaryProfileLoader(JNIEnv* env); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: This should be defined within the "autofill" namespace. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.h:11: bool RegisterAuxiliaryProfileLoader(JNIEnv* env); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: This should be defined within the "autofill" namespace. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.h:18: // |PersonalAutofillPopulator|. On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Comments go above the class. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_impl_android.h:20: explicit AuxiliaryProfileLoaderImplAndroid(); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: No need for explicit. 'explicit' is only needed for 1-arg constructors. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:14: namespace { On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: The test fixture class itself should not be in the anonymous namespace; > otherwise, it's possible to end up with identically-named test fixtures in > different files that fail with strange linker errors. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:18: AuxiliaryProfileAndroidTest() : profile_loader_( On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Break the line so that the colon is on the next line. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:33: std::vector<AutofillProfile*> profiles_; On 2013/03/05 00:42:38, Ilya Sherman wrote: > Shouldn't this be a ScopedVector? Who owns the memory for the profiles in this > vector? The profiles are owned by the caller to populator method. Fixed with scopedVector. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:55: AutofillProfile * profile = GetAndLoadProfile(); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: No space before the asterisk Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:109: * On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Use "// "-style comments. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profiles_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:27: // Randomly generated guid. Autofillprofile requires a consistent unique guid On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: "Autofillprofile" -> "The AutofillProfile class" Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:35: : profile_loader_(profileLoader) { On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Please fix the indentation here. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:42: // constructed from profileLoader. On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Comments like this belong in the header file, rather than in the > implementation file. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:44: std::vector<AutofillProfile*>* profiles) { On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Fix indentation. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:45: DCHECK(profiles); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: No need for this. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:57: // Chromium issue id # 178838 On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: "http://crbug.com/178838" Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:68: std::vector<string16> accVector; On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: hacker_case, spell out whatever "acc" is short for (or rename the variable > to something like "parts") Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:69: if (!pobox.empty()) accVector.push_back(pobox); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Move the body of the if-stmt onto a separate line. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:70: if (!neighborhood.empty()) accVector.push_back(neighborhood); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Move the body of the if-stmt onto a separate line. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:74: } else { On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: No need for an else stmt after a return. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:75: string16 acc; On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Spell out whatever "acc" is short for Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:82: } On 2013/03/05 00:42:38, Ilya Sherman wrote: > Can all of this code be replaced by just calling base::JoinString()? Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:87: string16 pobox = profile_loader_.GetPostOfficeBox(); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Spell out "pobox" Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:107: string16 lastName = profile_loader_.GetLastName(); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: hacker_case Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:115: std::vector<string16> emailsVector; On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Just name this "emails" Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:121: std::vector<string16> phoneNumbersVector; On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Name this "phone_numbers" Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profiles_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:21: } // namespace On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Write this as: > > namespace autofill { > class AuxiliaryProfileLoader; > } // namespace > > (no newlines, no indentation, two spaces before the comment) Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:25: class AuxiliaryProfilesAndroid { On 2013/03/05 00:42:38, Ilya Sherman wrote: > Why do you need this class to be separate from the AuxiliaryProfileLoader class? > It seems like you could just include a GetAutofillProfile() method in the > latter class's API. Basically, I want to separate the JNI stuff (AuxiliaryProfileLoaderImpl) from the task of composing that data into a profile (AuxiliaryProfilesAndroid). So that I can unit test the latter by stubbing out everything that hits JNI (and the Android OS contact's profile). The alternative was to write tests which would have destructive side effects to the user's contacts profile on the phone. I find this approach of separating the two makes sense for me as it's separating two concerns. If you have another alternative let me know. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:30: // profile and injected into the vector. On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Comment belongs above the class. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:33: // Constructor takes in an AuxiliaryProfileLoader object whicht provides On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: "whicht" -> "which" Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:36: const autofill::AuxiliaryProfileLoader& profileLoader); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Use hacker_case. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:41: void LoadContactsProfile(std::vector<AutofillProfile*>* profiles); On 2013/03/05 00:42:38, Ilya Sherman wrote: > Why doesn't this method just return a scoped_ptr<AutofillProfile>? Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:46: string16 CollapseAddress(const string16& pobox, On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Spell out post_office_box. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:47: const string16& neighborhood); On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Can this be tucked into the anonymous namespace in the implementation file? Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:48: // inserts contact's address data into profile. On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: Start the comment with an uppercase letter. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/personal_data_manager_android.cc:7: #include <vector> On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: This doesn't seem to be used. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/personal_data_manager_android.cc:8: #include "base/logging.h" On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: This doesn't seem to be used. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/browser/aut... chrome/browser/autofill/android/personal_data_manager_android.cc:9: #include "chrome/browser/autofill/android/auxiliary_profile_loader_android.h" On 2013/03/05 00:42:38, Ilya Sherman wrote: > nit: This doesn't seem to be used. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/chrome_brow... chrome/chrome_browser.gypi:73: 'browser/accessibility/accessibility_events.h', On 2013/03/05 00:42:38, Ilya Sherman wrote: > This doesn't seem to be related to your CL. Done. https://chromiumcodereview.appspot.com/12282004/diff/39002/chrome/chrome_brow... chrome/chrome_browser.gypi:128: 'browser/auto_launch_trial.cc', On 2013/03/05 00:42:38, Ilya Sherman wrote: > This doesn't seem to be related to your CL. Done.
https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_loader_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:5: // nit: Replaces lines 4 and 5 with a single blank line. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:18: populator_.obj()) nit: Please fix the indentation here. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. ultra nit: Please omit the "(c)" (applies to all of the files in this CL) https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:18: // contact information. Please update this comment. The class is no longer abstract. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:16: AuxiliaryProfileAndroidTest() : profile_loader_(){} nit: Just write this as "AuxiliaryProfileAndroidTest() {}" https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:94: AutofillProfile * profile = GetAndLoadProfile(); nit: No space before the asterisk. (Please fix this throughout.) https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:105: /* nit: If you must "comment out" a test, please use "#ifdef 0" rather than "/*" comments. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profiles_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:41: } nit: No need for curly braces. Ditto below. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:55: : profile_loader_(profileLoader) {} nit: Indent this line two fewer spaces. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profiles_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:18: class AuxiliaryProfileLoaderAndroid; nit: De-indent this line. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:35: // Returns autofill profile constructed from profileLoader. nit: Please update "profileLoader" -- probably to "profile_loader_" https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/personal_data_manager_android.cc:13: autofill::AuxiliaryProfileLoaderAndroid profileLoader; nit: hacker_case (please try to fix the variable name casing throughout the CL) https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... File chrome/browser/autofill/android/test_auxiliary_profile_loader_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.cc:58: std::vector<string16>* emailAddresses) const { nit: hacker_case for this and all other variables in this file. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.cc:111: void TestAuxiliaryProfileLoader::SetEmailAddresses(std::vector<string16> addr) { nit: Please spell out "address" https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.cc:115: void TestAuxiliaryProfileLoader::SetPhoneNumbers(std::vector<string16> pns) { nit: Please spell out "phone_numbers" https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... File chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:11: class TestAuxiliaryProfileLoader : public autofill::AuxiliaryProfileLoaderAndroid { nit: 80-col https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:17: virtual string16 GetFirstName() const; nit: This should still include the "OVERRIDE" annotation (applies throughout). https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:31: virtual void GetPhoneNumbers(std::vector<string16>* phoneNumbers) const; nit: hacker_case for both of these. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:36: void SetSuffix(string16); nit: For all of the SetFoo() methods, pass the parameter by const-reference, and include names for the parameters. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:47: void SetPhoneNumbers(std::vector<string16> emailAddresses); nit: hacker_case for both of these. https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:55: string16 postalCode_; nit: hacker_case https://chromiumcodereview.appspot.com/12282004/diff/43001/chrome/browser/aut... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:59: string16 lastName_; nit: hacker_case for these three...
https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... File chrome/browser/autofill/android/auxiliary_profile_loader_android.cc (right): https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:5: // On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: Replaces lines 4 and 5 with a single blank line. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:5: // On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: Replaces lines 4 and 5 with a single blank line. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:18: populator_.obj()) On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: Please fix the indentation here. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... File chrome/browser/autofill/android/auxiliary_profile_loader_android.h (right): https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/03/09 01:42:58, Ilya Sherman wrote: > ultra nit: Please omit the "(c)" (applies to all of the files in this CL) Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:18: // contact information. On 2013/03/09 01:42:58, Ilya Sherman wrote: > Please update this comment. The class is no longer abstract. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... File chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc (right): https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:16: AuxiliaryProfileAndroidTest() : profile_loader_(){} On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: Just write this as "AuxiliaryProfileAndroidTest() {}" Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:94: AutofillProfile * profile = GetAndLoadProfile(); On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: No space before the asterisk. (Please fix this throughout.) Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_unittest_android.cc:105: /* On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: If you must "comment out" a test, please use "#ifdef 0" rather than "/*" > comments. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... File chrome/browser/autofill/android/auxiliary_profiles_android.cc (right): https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profiles_android.cc:41: } On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: No need for curly braces. Ditto below. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profiles_android.cc:55: : profile_loader_(profileLoader) {} On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: Indent this line two fewer spaces. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... File chrome/browser/autofill/android/auxiliary_profiles_android.h (right): https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profiles_android.h:18: class AuxiliaryProfileLoaderAndroid; On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: De-indent this line. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profiles_android.h:35: // Returns autofill profile constructed from profileLoader. On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: Please update "profileLoader" -- probably to "profile_loader_" Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/personal_data_manager_android.cc:13: autofill::AuxiliaryProfileLoaderAndroid profileLoader; On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: hacker_case (please try to fix the variable name casing throughout the CL) Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... File chrome/browser/autofill/android/test_auxiliary_profile_loader_android.cc (right): https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.cc:58: std::vector<string16>* emailAddresses) const { On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: hacker_case for this and all other variables in this file. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.cc:111: void TestAuxiliaryProfileLoader::SetEmailAddresses(std::vector<string16> addr) { On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: Please spell out "address" Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.cc:115: void TestAuxiliaryProfileLoader::SetPhoneNumbers(std::vector<string16> pns) { On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: Please spell out "phone_numbers" Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... File chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h (right): https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:11: class TestAuxiliaryProfileLoader : public autofill::AuxiliaryProfileLoaderAndroid { On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: 80-col Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:17: virtual string16 GetFirstName() const; On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: This should still include the "OVERRIDE" annotation (applies throughout). Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:31: virtual void GetPhoneNumbers(std::vector<string16>* phoneNumbers) const; On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: hacker_case for both of these. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:36: void SetSuffix(string16); On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: For all of the SetFoo() methods, pass the parameter by const-reference, and > include names for the parameters. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:47: void SetPhoneNumbers(std::vector<string16> emailAddresses); On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: hacker_case for both of these. Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:55: string16 postalCode_; On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: hacker_case Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:55: string16 postalCode_; On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: hacker_case Done. https://codereview.chromium.org/12282004/diff/43001/chrome/browser/autofill/a... chrome/browser/autofill/android/test_auxiliary_profile_loader_android.h:59: string16 lastName_; On 2013/03/09 01:42:58, Ilya Sherman wrote: > nit: hacker_case for these three... Done.
Thanks. LGTM with a last few nits: https://codereview.chromium.org/12282004/diff/43005/base/android/jni_local_re... File base/android/jni_local_ref_extensions.cc (right): https://codereview.chromium.org/12282004/diff/43005/base/android/jni_local_re... base/android/jni_local_ref_extensions.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: Remove the (c) https://codereview.chromium.org/12282004/diff/43005/base/android/jni_local_re... File base/android/jni_local_ref_extensions.h (right): https://codereview.chromium.org/12282004/diff/43005/base/android/jni_local_re... base/android/jni_local_ref_extensions.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: Remove the (c) https://codereview.chromium.org/12282004/diff/43005/base/android/jni_local_re... base/android/jni_local_ref_extensions.h:17: string16 SafeJavaStringToUTF16(ScopedJavaLocalRef<jstring> jstring); nit: Should this be passed by const-ref? https://codereview.chromium.org/12282004/diff/43005/base/android/jni_local_re... base/android/jni_local_ref_extensions.h:22: ScopedJavaLocalRef<jobjectArray> jarray, nit: Should this be passed by const-ref? https://codereview.chromium.org/12282004/diff/43005/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/12282004/diff/43005/base/base.gypi#newcode64 base/base.gypi:64: 'android/scoped_java_ref.h', nit: This doesn't seem to be related to your CL. Please break this out into a separate CL; or at least remove the incorrectly sorted lines 43 and 44. https://codereview.chromium.org/12282004/diff/43005/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://codereview.chromium.org/12282004/diff/43005/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: Remove the (c) https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... File chrome/browser/autofill/android/auxiliary_profile_loader_android.cc (right): https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:15: #define JAVA_METHOD(__jmethod__) Java_PersonalAutofillPopulator_##__jmethod__(\ nit: Please leave a space before the backslash. https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:16: env_,\ nit: Please leave a space before the backslash. https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:30: } nit: Please be consistent about "{}" on the same line or on split lines (compare to the constructor). https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... File chrome/browser/autofill/android/auxiliary_profile_loader_android.h (right): https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:25: void Init(JNIEnv* env, const jobject& context); nit: Docs. https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:25: void Init(JNIEnv* env, const jobject& context); nit: Please leave a blank line after this one. https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... File chrome/browser/autofill/android/auxiliary_profiles_android.cc (right): https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profiles_android.cc:84: CollapseAddress(post_office_box, neighborhood); nit: Looks like this fits on the previous line. https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... File chrome/browser/autofill/android/auxiliary_profiles_android.h (right): https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profiles_android.h:30: // contact information methods. nit: Line wrapping seems off. https://codereview.chromium.org/12282004/diff/43005/chrome/browser/autofill/a... chrome/browser/autofill/android/auxiliary_profiles_android.h:39: // inserts contact's address data into profile. nit: Begin sentences with uppercase letters (applies throughout).
https://chromiumcodereview.appspot.com/12282004/diff/43005/base/android/jni_l... File base/android/jni_local_ref_extensions.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/43005/base/android/jni_l... base/android/jni_local_ref_extensions.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Remove the (c) Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/base/android/jni_l... File base/android/jni_local_ref_extensions.h (right): https://chromiumcodereview.appspot.com/12282004/diff/43005/base/android/jni_l... base/android/jni_local_ref_extensions.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Remove the (c) Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/base/android/jni_l... base/android/jni_local_ref_extensions.h:17: string16 SafeJavaStringToUTF16(ScopedJavaLocalRef<jstring> jstring); On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Should this be passed by const-ref? Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/base/android/jni_l... base/android/jni_local_ref_extensions.h:22: ScopedJavaLocalRef<jobjectArray> jarray, On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Should this be passed by const-ref? Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Remove the (c) Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_loader_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:15: #define JAVA_METHOD(__jmethod__) Java_PersonalAutofillPopulator_##__jmethod__(\ On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Please leave a space before the backslash. Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:16: env_,\ On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Please leave a space before the backslash. Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.cc:30: } On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Please be consistent about "{}" on the same line or on split lines (compare > to the constructor). Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profile_loader_android.h:25: void Init(JNIEnv* env, const jobject& context); On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Docs. Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profiles_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.cc:84: CollapseAddress(post_office_box, neighborhood); On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Looks like this fits on the previous line. Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... File chrome/browser/autofill/android/auxiliary_profiles_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:30: // contact information methods. On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Line wrapping seems off. Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:39: // inserts contact's address data into profile. On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Begin sentences with uppercase letters (applies throughout). Done. https://chromiumcodereview.appspot.com/12282004/diff/43005/chrome/browser/aut... chrome/browser/autofill/android/auxiliary_profiles_android.h:39: // inserts contact's address data into profile. On 2013/03/11 22:39:44, Ilya Sherman wrote: > nit: Begin sentences with uppercase letters (applies throughout). Done.
It looks like there's still a bit more to move over to the component. Namely the java file and the gypi definitions. https://chromiumcodereview.appspot.com/12282004/diff/72001/base/android/jni_l... File base/android/jni_local_ref_extensions.h (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/base/android/jni_l... base/android/jni_local_ref_extensions.h:17: string16 SafeJavaStringToUTF16(const ScopedJavaLocalRef<jstring>& jstring); To be honest, I don't see why we need these additional functions. It seems to me like the functionality in jni_string and jni_array are sufficient. ConvertJavaStringToUTF16 already does a null check on the passed in |str| https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:7: package org.chromium.chrome.browser.autofill; Should move to the component and update the package https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/browser/and... File chrome/browser/android/chrome_jni_registrar.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/browser/and... chrome/browser/android/chrome_jni_registrar.cc:70: { "RegisterAuxiliaryProfileLoader", autofill::RegisterAuxiliaryProfileLoader } Nit: move above the "components" section. See L37 https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_brow... chrome/chrome_browser.gypi:2193: '../components/autofill/browser/android/auxiliary_profile_loader_android.h', These should all be in the components gyp https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_brow... chrome/chrome_browser.gypi:3123: 'target_name': 'chrome_browser_jni_headers', You'll have to define a new jni generator target with the one file (PersonalAutofillPopulator.java) in components/autofill.gypi You can see an example of this in components/web_contents_delegate_android.gypi https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_test... File chrome/chrome_tests_unit.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_test... chrome/chrome_tests_unit.gypi:274: '../components/autofill/browser/android/test_auxiliary_profile_loader_android.cc', I don't see why these are here. THey don't depend on chrome, do they? Move co the component gypi. https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_test... chrome/chrome_tests_unit.gypi:1627: '../components/autofill/browser/android/auxiliary_profile_unittest_android.cc', same here https://chromiumcodereview.appspot.com/12282004/diff/72001/components/autofil... File components/autofill/browser/android/test_auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/components/autofil... components/autofill/browser/android/test_auxiliary_profile_loader_android.h:5: #ifndef CHROME_BROWSER_AUTOFILL_ANDROID_TEST_AUXILIARY_PROFILE_LOADER_ANDROID_H_ Update header guard
https://chromiumcodereview.appspot.com/12282004/diff/72001/base/android/jni_l... File base/android/jni_local_ref_extensions.h (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/base/android/jni_l... base/android/jni_local_ref_extensions.h:17: string16 SafeJavaStringToUTF16(const ScopedJavaLocalRef<jstring>& jstring); On 2013/03/13 21:23:23, Yaron wrote: > To be honest, I don't see why we need these additional functions. It seems to me > like the functionality in jni_string and jni_array are sufficient. > ConvertJavaStringToUTF16 already does a null check on the passed in |str| Done. https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalAutofillPopulator.java:7: package org.chromium.chrome.browser.autofill; On 2013/03/13 21:23:23, Yaron wrote: > Should move to the component and update the package Done. https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_brow... chrome/chrome_browser.gypi:2193: '../components/autofill/browser/android/auxiliary_profile_loader_android.h', On 2013/03/13 21:23:23, Yaron wrote: > These should all be in the components gyp Done. https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_brow... chrome/chrome_browser.gypi:3123: 'target_name': 'chrome_browser_jni_headers', On 2013/03/13 21:23:23, Yaron wrote: > You'll have to define a new jni generator target with the one file > (PersonalAutofillPopulator.java) in components/autofill.gypi > > You can see an example of this in components/web_contents_delegate_android.gypi Done. https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_test... File chrome/chrome_tests_unit.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_test... chrome/chrome_tests_unit.gypi:274: '../components/autofill/browser/android/test_auxiliary_profile_loader_android.cc', On 2013/03/13 21:23:23, Yaron wrote: > I don't see why these are here. THey don't depend on chrome, do they? Move co > the component gypi. Done. https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_test... chrome/chrome_tests_unit.gypi:274: '../components/autofill/browser/android/test_auxiliary_profile_loader_android.cc', On 2013/03/13 21:23:23, Yaron wrote: > I don't see why these are here. THey don't depend on chrome, do they? Move co > the component gypi. Done. https://chromiumcodereview.appspot.com/12282004/diff/72001/chrome/chrome_test... chrome/chrome_tests_unit.gypi:1627: '../components/autofill/browser/android/auxiliary_profile_unittest_android.cc', On 2013/03/13 21:23:23, Yaron wrote: > same here Done. https://chromiumcodereview.appspot.com/12282004/diff/72001/components/autofil... File components/autofill/browser/android/test_auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/72001/components/autofil... components/autofill/browser/android/test_auxiliary_profile_loader_android.h:5: #ifndef CHROME_BROWSER_AUTOFILL_ANDROID_TEST_AUXILIARY_PROFILE_LOADER_ANDROID_H_ On 2013/03/13 21:23:23, Yaron wrote: > Update header guard Done.
Why was the test code moved back to unit_tests? I think with my suggestion it should be fine unless you discovered actual dependencies on chrome? https://chromiumcodereview.appspot.com/12282004/diff/80001/chrome/android/tes... File chrome/android/testshell/java/AndroidManifest.xml (right): https://chromiumcodereview.appspot.com/12282004/diff/80001/chrome/android/tes... chrome/android/testshell/java/AndroidManifest.xml:75: <uses-permission android:name="android.permission.READ_CONTACTS"/> Can you update the CL description and specify that it adds these permissions and which API calls require them. https://chromiumcodereview.appspot.com/12282004/diff/80001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://chromiumcodereview.appspot.com/12282004/diff/80001/chrome/chrome.gyp#... chrome/chrome.gyp:1088: Nit: remove blank line https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... File components/autofill.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... components/autofill.gypi:94: 'target_name': 'autofill_android_java', the android is redundant in target_naems and the like. Please call it autofill_java https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... components/autofill.gypi:106: 'target_name': 'autofill_android_jni_headers', autofill_jni_headers https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... components/autofill.gypi:112: 'jni_gen_package': 'autofill_android', autofill https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... File components/autofill/browser/android/auxiliary_profile_loader_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... components/autofill/browser/android/auxiliary_profile_loader_android.h:61: JNIEnv* env_; Hmm. We generally don't cache the env pointer or a local ref especially if can be accessed from multiple threads. In this case it looks like you're doing it in a single thread of execution and throwing it away after so I guess it's ok. https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... File components/autofill/browser/android/auxiliary_profile_unittest_android.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... components/autofill/browser/android/auxiliary_profile_unittest_android.cc:100: // Android user's profile contact does not prase its address s/prase/parse https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... File components/autofill/browser/android/auxiliary_profiles_android.h (right): https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... components/autofill/browser/android/auxiliary_profiles_android.h:19: } // namespace Why close the namespace and then re-open below? Just use one, right? Or you're separating forward declares? https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... File components/autofill/browser/android/component_jni_registrar.cc (right): https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... components/autofill/browser/android/component_jni_registrar.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. 2013 (throughout) https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... components/autofill/browser/android/component_jni_registrar.cc:5: #include "components/autofill/browser/android//component_jni_registrar.h" Nit: only one "/" https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... File components/autofill/browser/android/component_jni_registrar.h (right): https://chromiumcodereview.appspot.com/12282004/diff/80001/components/autofil... components/autofill/browser/android/component_jni_registrar.h:12: // Register all JNI bindings necessary for the autofill_android s/autofill_android/autofill/ https://chromiumcodereview.appspot.com/12282004/diff/80001/components/compone... File components/components_tests.gypi (right): https://chromiumcodereview.appspot.com/12282004/diff/80001/components/compone... components/components_tests.gypi:14: 'autofill/browser/android/test_auxiliary_profile_loader_android.cc', Instead of re-listing files from the binary, just add a dependency to the library in the "dependencies" list below https://chromiumcodereview.appspot.com/12282004/diff/80001/components/compone... components/components_tests.gypi:16: 'browser/android/auxiliary_profile_unittest_android.cc', missing "autofill/" prefix. Is this test being built? Also: swap "unittest" and "android" in the filename
@Yaron, The tests depend on autofill_profile.cc which is built in chrome_browser.gypi hence the dependency. https://codereview.chromium.org/12282004/diff/80001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/12282004/diff/80001/chrome/chrome.gyp#newcode... chrome/chrome.gyp:1088: On 2013/03/16 00:15:30, Yaron wrote: > Nit: remove blank line Done. https://codereview.chromium.org/12282004/diff/80001/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/12282004/diff/80001/components/autofill.gypi#... components/autofill.gypi:94: 'target_name': 'autofill_android_java', On 2013/03/16 00:15:30, Yaron wrote: > the android is redundant in target_naems and the like. Please call it > autofill_java Done. https://codereview.chromium.org/12282004/diff/80001/components/autofill.gypi#... components/autofill.gypi:106: 'target_name': 'autofill_android_jni_headers', On 2013/03/16 00:15:30, Yaron wrote: > autofill_jni_headers Done. https://codereview.chromium.org/12282004/diff/80001/components/autofill.gypi#... components/autofill.gypi:112: 'jni_gen_package': 'autofill_android', On 2013/03/16 00:15:30, Yaron wrote: > autofill Done. https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... File components/autofill/browser/android/auxiliary_profile_loader_android.h (right): https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... components/autofill/browser/android/auxiliary_profile_loader_android.h:61: JNIEnv* env_; Thanks. I will keep this in mind for the future. On 2013/03/16 00:15:30, Yaron wrote: > Hmm. We generally don't cache the env pointer or a local ref especially if can > be accessed from multiple threads. In this case it looks like you're doing it in > a single thread of execution and throwing it away after so I guess it's ok. https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... File components/autofill/browser/android/auxiliary_profile_unittest_android.cc (right): https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... components/autofill/browser/android/auxiliary_profile_unittest_android.cc:100: // Android user's profile contact does not prase its address On 2013/03/16 00:15:30, Yaron wrote: > s/prase/parse Done. https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... File components/autofill/browser/android/auxiliary_profiles_android.h (right): https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... components/autofill/browser/android/auxiliary_profiles_android.h:19: } // namespace Was separating forward declares. I'll change it. On 2013/03/16 00:15:30, Yaron wrote: > Why close the namespace and then re-open below? Just use one, right? Or you're > separating forward declares? Done. https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... File components/autofill/browser/android/component_jni_registrar.cc (right): https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... components/autofill/browser/android/component_jni_registrar.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2013/03/16 00:15:30, Yaron wrote: > 2013 (throughout) Done. https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... components/autofill/browser/android/component_jni_registrar.cc:5: #include "components/autofill/browser/android//component_jni_registrar.h" On 2013/03/16 00:15:30, Yaron wrote: > Nit: only one "/" Done. https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... File components/autofill/browser/android/component_jni_registrar.h (right): https://codereview.chromium.org/12282004/diff/80001/components/autofill/brows... components/autofill/browser/android/component_jni_registrar.h:12: // Register all JNI bindings necessary for the autofill_android On 2013/03/16 00:15:30, Yaron wrote: > s/autofill_android/autofill/ Done.
native android bits lgtm https://codereview.chromium.org/12282004/diff/96001/components/autofill/brows... File components/autofill/browser/android/auxiliary_profiles_android.cc (right): https://codereview.chromium.org/12282004/diff/96001/components/autofill/brows... components/autofill/browser/android/auxiliary_profiles_android.cc:115: } // namespace nit: namespace autofill https://codereview.chromium.org/12282004/diff/96001/components/autofill/brows... File components/autofill/browser/android/auxiliary_profiles_android.h (right): https://codereview.chromium.org/12282004/diff/96001/components/autofill/brows... components/autofill/browser/android/auxiliary_profiles_android.h:26: // Takes in an AuxiliaryProfileLoader object which provides contact Nit: indentation is off on the public section. They should be indented once from "public:"
https://codereview.chromium.org/12282004/diff/96001/components/autofill/brows... File components/autofill/browser/android/auxiliary_profiles_android.cc (right): https://codereview.chromium.org/12282004/diff/96001/components/autofill/brows... components/autofill/browser/android/auxiliary_profiles_android.cc:115: } // namespace On 2013/03/18 20:23:05, Yaron wrote: > nit: namespace autofill Done. https://codereview.chromium.org/12282004/diff/96001/components/autofill/brows... File components/autofill/browser/android/auxiliary_profiles_android.h (right): https://codereview.chromium.org/12282004/diff/96001/components/autofill/brows... components/autofill/browser/android/auxiliary_profiles_android.h:26: // Takes in an AuxiliaryProfileLoader object which provides contact On 2013/03/18 20:23:05, Yaron wrote: > Nit: indentation is off on the public section. They should be indented once from > "public:" Done.
So it occurs to me that give we're holding the feature launch for now, we can land it but flag-guarded. Can you add that? A separate issue that I was just reminded of is that even though personal_data_manager_android.cc is in the component dir, it still shares the name with the one in chrome which will break the x86 compile. I'll rename the other one to open the door for you.
Done. Please have a look. On 2013/03/19 20:15:51, Yaron wrote: > So it occurs to me that give we're holding the feature launch for now, we can > land it but flag-guarded. Can you add that? > > A separate issue that I was just reminded of is that even though > personal_data_manager_android.cc is in the component dir, it still shares the > name with the one in chrome which will break the x86 compile. I'll rename the > other one to open the door for you.
lgtm I think I was mistaken and this should be fine even with the duplicate basename. These are in separate static libraries so it should be ok. https://chromiumcodereview.appspot.com/12282004/diff/109001/chrome/android/te... File chrome/android/testshell/java/AndroidManifest.xml (right): https://chromiumcodereview.appspot.com/12282004/diff/109001/chrome/android/te... chrome/android/testshell/java/AndroidManifest.xml:76: <uses-permission android:name="android.permission.READ_CONTACTS"/> Let's remove these perms for now. We're hoping to soon merge manifests so that we'll inherit them as we go (and not have to duplicate) and so we'd end up pulling these into chrome for android
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/12282004/124001
Presubmit check for 12282004-124001 failed and returned exit status 1.
INFO:root:Found 21 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
components/autofill.gypi
chrome/chrome.gyp
Presubmit checks took 2.1s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/12282004/124001
Presubmit check for 12282004-124001 failed and returned exit status 1.
INFO:root:Found 21 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
components/autofill.gypi
chrome/chrome.gyp
Presubmit checks took 2.2s to calculate.
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
thakis: chrome/chrome.gyp joi: components/autofill.gypi
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/12282004/124001
chrome.gyp lgtm Please wrap your commit messages at 80 columns for people using `git log`. If you had said "TBR=" instead of "TBR:", the presubmit would've let this through. (But TBR= doesn't mean "committed without review", you still have to cc owners and send out a "please review" message. Also see today's chromium-dev thread on the topic.)
Retried try job too often on linux_aura for step(s) aura_unittests, browser_tests, compositor_unittests, content_browsertests, content_unittests, interactive_ui_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/12282004/149001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/12282004/163001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/12282004/175001
Message was sent while issue was closed.
Change committed as 190842 |
