Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
872023002:
Use floating labels for preference forms (Closed)
DescriptionUse floating labels for preference forms
Refactor AutofillProfileEditor to use cpp third_party/libaddressinput
and custom Java UI (rather than libaddressinput's Java AddressWidget).
Add FloatLabelLayout.java and change autofill profile and credit card forms
to use floating labels.
BUG=414438
Committed: https://crrev.com/0bc6149395ed645213e16fa62ae3321dc0880537
Cr-Commit-Position: refs/heads/master@{#313973}
Patch Set 1 #Patch Set 2 : Fix chrome/browser/BUILD.gn #
Total comments: 2
Patch Set 3 : Change repack_locales condition to just enable_autofill_dialog #
Total comments: 58
Patch Set 4 : Use shared Java/C++ enum, other changes from newt review #Patch Set 5 : A few more tweaks #Patch Set 6 : Manually focus first field for empty form/on country change #Patch Set 7 : Replace autogenerated AddressField.java with manually generated; order countries by display name #Patch Set 8 : Actually commit AddressField.java #
Total comments: 38
Patch Set 9 : Get application language from C++ #
Total comments: 20
Patch Set 10 : Do more on cpp side, cache field data when form is reset, etc #Patch Set 11 : Minor tweaks #Patch Set 12 : Fix findbugs #Patch Set 13 : Re-fix findbugs #
Total comments: 42
Patch Set 14 : Getting closer... #
Total comments: 4
Patch Set 15 : Minor changes #Messages
Total messages: 33 (3 generated)
twellington@chromium.org changed reviewers: + kerz@chromium.org, newt@chromium.org, thestig@chromium.org
Including reviewers: newt@ for chrome/android & chrome/android/browser changes kerz@ for chrome/tools/build/repack_locales.py thestig@ for chrome/chrome.gyp, chrome/chrome_browser.gypi, chrome/chrome_repack_locales.gni & chrome/browser/BUILD.gn
lgtm
https://codereview.chromium.org/872023002/diff/20001/chrome/chrome_repack_loc... File chrome/chrome_repack_locales.gni (right): https://codereview.chromium.org/872023002/diff/20001/chrome/chrome_repack_loc... chrome/chrome_repack_locales.gni:65: if ((enable_autofill_dialog && !is_ios) || is_android) { I think this should be just enable_autofill_dialog. As is, android + webview will have this extra dependecy that it doesn't need.
https://codereview.chromium.org/872023002/diff/20001/chrome/chrome_repack_loc... File chrome/chrome_repack_locales.gni (right): https://codereview.chromium.org/872023002/diff/20001/chrome/chrome_repack_loc... chrome/chrome_repack_locales.gni:65: if ((enable_autofill_dialog && !is_ios) || is_android) { On 2015/01/23 20:53:18, Lei Zhang wrote: > I think this should be just enable_autofill_dialog. As is, android + webview > will have this extra dependecy that it doesn't need. You are correct. Done.
gn/gyp files lgtm
Overall design looks pretty good. Lots of comments inline though :) Could you add a reviewer who's familiar with autofill to look at autofill_profile_bridge.cc (and maybe AutofillProfileEditor.java too)? https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... File chrome/android/java/res/layout/autofill_credit_card_editor.xml (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... chrome/android/java/res/layout/autofill_credit_card_editor.xml:34: android:contentDescription="@string/accessibility_autofill_cc_name_textbox" I believe that EditTexts shouldn't have contentDescription, since the hint text is already used for that purpose (and the contentDescription may override the actual content of the text field when read aloud, which is definitely not what we want) https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... File chrome/android/java/res/layout/homepage_preferences.xml (left): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... chrome/android/java/res/layout/homepage_preferences.xml:23: android:layout_margin="6dp" This margin applies in all directions. Does this still look OK after removing it? https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... File chrome/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... chrome/android/java/res/values-v17/styles.xml:70: <style name="PreferenceFloatLabelTextAppearance"> In PreferencesTheme, you can set PreferenceFloatLabelTextAppearance as the default value for floatLabelTextAppearance, and it'll automatically be applied to all FloatLabelLayouts: <item name="floatLabelTextAppearance">@style/PreferenceFloatLabelTextAppearance</item> https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... chrome/android/java/res/values-v17/styles.xml:74: <style name="PreferenceTextFieldLabel" parent="@style/BoldTextFieldLabel"> When is this style used vs the other? Could you post screenshots on the bug? https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015, here and elsewhere https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:21: public static List<String> getAvailableCountries() { javadoc for all public methods https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:27: public static List<Pair<String, String>> getAddressUiComponents(String countryCode, javadoc. *especially* for a method that returns a mysterious value of type List<Pair<String, String>>. I can't read your mind :) https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:101: public void onTextChanged(CharSequence s, int start, int before, int count) { The current logic seems unnecessarily tricky. How about: boolean empty = mNoCountryItemIsSelected && TextUtils.isEmpty(s) && allFieldsEmpty(); setSaveButtonEnabled(!empty); https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:116: for (String fieldId : mAddressFields.keySet()) { You can iterate directly over the HashMap values: http://stackoverflow.com/a/1066607/598094 https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:130: mWidgetRoot.requestFocus(); what happens if you omit mWidgetRoot.requestFocus()? https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:161: resetFormFields(mCurrentCountryPos); what happens if mCurrentCountryPos is -1? https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:163: setFieldText("admin_area", profile.getRegion()); These strings should be turned into constants. Ideally, they could be shared between Java and C++. One option is to turn these into integer constants and use the java_cpp_enum build rule to generate an enum that's shared between Java and C++. In that case, these constants could be integer indices into an array of EditTexts, which would allow you to replace the HashMap with a simple array or list. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:192: EditText fieldEditText = new EditText(getActivity()); You could add this EditText to the preference_float_label_layout.xml, then it would be inflated for you and already have some attributes set. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:198: | InputType.TYPE_TEXT_VARIATION_POSTAL_ADDRESS Should all fields really be treated as postal addresses? Is that what we currently do? https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:215: getFieldText("orgnaization"), typo :/ That's why these constants should be defined only once. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java:29: * Layout which an {@link android.widget.EditText} to show a floating label when the hint is hidden Not quite a sentence https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java:30: * due to the user inputting text. It would be helpful to include a little snippet showing how to use a FloatLabelLayout. E.g. an excerpt of an XML layout. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java:32: * @see <a href="https://dribbble.com/shots/1254439--GIF-Mobile-Form-Interaction">Matt D. Smith on Dribble</a> This is from Chris Banes, right? Could you link to the original source and add a comment about using the floating label from the support library if/when it's available? https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/stri... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/stri... chrome/android/java/strings/android_chrome_strings.grd:146: <message name="IDS_AUTOFILL_PROFILE_EDITOR_COUNTRY" desc="Label for a spinner input field containg a list of countires or regions [CHAR-LIMIT=32]"> spelling: "countries" https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... File chrome/browser/android/preferences/autofill/DEPS (right): https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/DEPS:2: "+third_party/libaddressinput/src/cpp/include/libaddressinput/address_ui.h", It's probably fine to use a single rule here ("+third_party/libaddressinput/src/cpp/include/libaddressinput"). I'm not sure whether that's preferred, though. https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:23: using ::i18n::addressinput::GetRegionCodes; If you're only calling a method once, I'd skip using "using". https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:42: jstring ui_language_tag, I'd call this j_language_tag (and j_country_code) for consistency https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:45: std::string best_language_tag; We never use this value. Should we? / why does it exist? https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:47: std::vector<std::string> componentIds; Use hacker_case not camelCase (it's easy to forget this when switching between Java and C++) https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:54: std::string language_tag = ConvertJavaStringToUTF8(env, ui_language_tag); language_tag is never used. Did you mean to use it on line 59? https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:62: // TODO(twellington): debug localization problem: You can remove this comment. I filed a bug to track this: https://code.google.com/p/chromium/issues/detail?id=452293 https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:74: componentIds.push_back("country"); Is there not a string constant we could use here instead of hardcoding "country", "admin_area", etc? See my comment in AutofillProfileEditor about sharing these constants between C++ and Java using the java_cpp_enum build rule. https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:100: } Adding a default case seems like a good idea: default: NOTREACHED(); https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.h (right): https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.h:10: namespace autofill { consensus on chromium-dev@ is not to use namespaces in src/chrome.
twellington@chromium.org changed reviewers: + estade@chromium.org
newt@ PTAL -- addressed review comments and fixed a few things I noticed in testing today estade@ -- added for review of: - chrome/browser/android/preferences/autofill/DEPS (requires an lgtm from a libaddressinput owner) - autofill_profile_bridge.cc (requires knowledge of autofill profiles) - AutofillProfileEditor.java (if time permits, requires knowledge of autofill profiles) https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... File chrome/android/java/res/layout/autofill_credit_card_editor.xml (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... chrome/android/java/res/layout/autofill_credit_card_editor.xml:34: android:contentDescription="@string/accessibility_autofill_cc_name_textbox" On 2015/01/27 01:45:05, newt wrote: > I believe that EditTexts shouldn't have contentDescription, since the hint text > is already used for that purpose (and the contentDescription may override the > actual content of the text field when read aloud, which is definitely not what > we want) It still reads the actual contents of the text field. FloatLabelLayout sets the hint text to null when the label is shown, so without content descriptions, when the EditText box is focused but has no content, the text read aloud is just "edit box." With the content description set, it reads "edit box organization" (or whatever) when the field is empty; if the field has content it says "edit box Google" (or whatever the content is). https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... File chrome/android/java/res/layout/homepage_preferences.xml (left): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... chrome/android/java/res/layout/homepage_preferences.xml:23: android:layout_margin="6dp" On 2015/01/27 01:45:05, newt wrote: > This margin applies in all directions. Does this still look OK after removing > it? Yes, I'll attach screenshots. Upon retesting, I need to tweak it a little for pre-L. On L with this patchset, the homepage screen has the same padding as the other preference screens (I forgot to look at homepage when I changed the L+ dividers to full width). https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... File chrome/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... chrome/android/java/res/values-v17/styles.xml:70: <style name="PreferenceFloatLabelTextAppearance"> On 2015/01/27 01:45:05, newt wrote: > In PreferencesTheme, you can set PreferenceFloatLabelTextAppearance as the > default value for floatLabelTextAppearance, and it'll automatically be applied > to all FloatLabelLayouts: > > <item > name="floatLabelTextAppearance">@style/PreferenceFloatLabelTextAppearance</item> Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/res/... chrome/android/java/res/values-v17/styles.xml:74: <style name="PreferenceTextFieldLabel" parent="@style/BoldTextFieldLabel"> On 2015/01/27 01:45:05, newt wrote: > When is this style used vs the other? Could you post screenshots on the bug? It was being used for the labels above the spinners, but there isn't really a difference. I'll drop this style and just use PreferenceFloatLabelTextAppearance for those as well. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/27 01:45:05, newt wrote: > 2015, here and elsewhere Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:21: public static List<String> getAvailableCountries() { On 2015/01/27 01:45:05, newt wrote: > javadoc for all public methods Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:27: public static List<Pair<String, String>> getAddressUiComponents(String countryCode, On 2015/01/27 01:45:05, newt wrote: > javadoc. *especially* for a method that returns a mysterious value of type > List<Pair<String, String>>. I can't read your mind :) Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:101: public void onTextChanged(CharSequence s, int start, int before, int count) { On 2015/01/27 01:45:06, newt wrote: > The current logic seems unnecessarily tricky. How about: > > boolean empty = mNoCountryItemIsSelected && TextUtils.isEmpty(s) && > allFieldsEmpty(); > setSaveButtonEnabled(!empty); Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:116: for (String fieldId : mAddressFields.keySet()) { On 2015/01/27 01:45:06, newt wrote: > You can iterate directly over the HashMap values: > http://stackoverflow.com/a/1066607/598094 Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:130: mWidgetRoot.requestFocus(); On 2015/01/27 01:45:06, newt wrote: > what happens if you omit mWidgetRoot.requestFocus()? The first EditText box doesn't get focused if this line is omitted. Since we focus the first field when a new address is being added (near the end of addProfileDataToEditFields), I thought it made sense to do it here as well. In playing around with this today, I noticed that the auto-focus looks weird with the floating label animation because you only see the tail end of the animation and it looks like the text is randomly shrinking, so I tweaked how this works. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:161: resetFormFields(mCurrentCountryPos); On 2015/01/27 01:45:06, newt wrote: > what happens if mCurrentCountryPos is -1? Currently, it would probably throw errors. I don't think we can get in a situation where the country saved with a profile isn't in mCountries because users should only be able to set the country to one of the CLDR country codes available in the drop down. Just in case, I added a check; if it's -1, mCountryPos is changed to mCountries.indexOf(Locale.getDefault().getCountry()); https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:163: setFieldText("admin_area", profile.getRegion()); On 2015/01/27 01:45:06, newt wrote: > These strings should be turned into constants. Ideally, they could be shared > between Java and C++. One option is to turn these into integer constants and use > the java_cpp_enum build rule to generate an enum that's shared between Java and > C++. > > In that case, these constants could be integer indices into an array of > EditTexts, which would allow you to replace the HashMap with a simple array or > list. Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:192: EditText fieldEditText = new EditText(getActivity()); On 2015/01/27 01:45:06, newt wrote: > You could add this EditText to the preference_float_label_layout.xml, then it > would be inflated for you and already have some attributes set. Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:198: | InputType.TYPE_TEXT_VARIATION_POSTAL_ADDRESS On 2015/01/27 01:45:06, newt wrote: > Should all fields really be treated as postal addresses? Is that what we > currently do? Yes, it is (see inputType on this file: https://cs.corp.google.com/#clankium/src/third_party/libaddressinput/src/java...) https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:215: getFieldText("orgnaization"), On 2015/01/27 01:45:06, newt wrote: > typo :/ That's why these constants should be defined only once. Acknowledged. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java:29: * Layout which an {@link android.widget.EditText} to show a floating label when the hint is hidden On 2015/01/27 01:45:06, newt wrote: > Not quite a sentence Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java:30: * due to the user inputting text. On 2015/01/27 01:45:06, newt wrote: > It would be helpful to include a little snippet showing how to use a > FloatLabelLayout. E.g. an excerpt of an XML layout. Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java:32: * @see <a href="https://dribbble.com/shots/1254439--GIF-Mobile-Form-Interaction">Matt D. Smith on Dribble</a> On 2015/01/27 01:45:06, newt wrote: > This is from Chris Banes, right? Could you link to the original source and add a > comment about using the floating label from the support library if/when it's > available? Done. https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/stri... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/872023002/diff/40001/chrome/android/java/stri... chrome/android/java/strings/android_chrome_strings.grd:146: <message name="IDS_AUTOFILL_PROFILE_EDITOR_COUNTRY" desc="Label for a spinner input field containg a list of countires or regions [CHAR-LIMIT=32]"> On 2015/01/27 01:45:06, newt wrote: > spelling: "countries" Done. https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... File chrome/browser/android/preferences/autofill/DEPS (right): https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/DEPS:2: "+third_party/libaddressinput/src/cpp/include/libaddressinput/address_ui.h", On 2015/01/27 01:45:06, newt wrote: > It's probably fine to use a single rule here > ("+third_party/libaddressinput/src/cpp/include/libaddressinput"). I'm not sure > whether that's preferred, though. > I think it's fine too, since it's included like that in src/chrome/browser/ui/autofill/DEPS. Possibly even preferred because then if we want to modify autofill_profile_bridge to use another class in libaddressinput, we don't have to remember to update this DEPS file (not properly updating this is only noticeable when tryjobs fail to compile). https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:23: using ::i18n::addressinput::GetRegionCodes; On 2015/01/27 01:45:07, newt wrote: > If you're only calling a method once, I'd skip using "using". Done. https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:42: jstring ui_language_tag, On 2015/01/27 01:45:06, newt wrote: > I'd call this j_language_tag (and j_country_code) for consistency Done. https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:45: std::string best_language_tag; On 2015/01/27 01:45:06, newt wrote: > We never use this value. Should we? / why does it exist? It's passed in to the ::i18n::addressinput::BuildComponents function as a required argument -- BuildComponents starts with this assertion: assert(best_address_language_tag != NULL); https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:47: std::vector<std::string> componentIds; On 2015/01/27 01:45:07, newt wrote: > Use hacker_case not camelCase (it's easy to forget this when switching between > Java and C++) Done. https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:54: std::string language_tag = ConvertJavaStringToUTF8(env, ui_language_tag); On 2015/01/27 01:45:06, newt wrote: > language_tag is never used. Did you mean to use it on line 59? Yep. Done. https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:62: // TODO(twellington): debug localization problem: On 2015/01/27 01:45:06, newt wrote: > You can remove this comment. I filed a bug to track this: > https://code.google.com/p/chromium/issues/detail?id=452293 Done. https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:74: componentIds.push_back("country"); On 2015/01/27 01:45:07, newt wrote: > Is there not a string constant we could use here instead of hardcoding > "country", "admin_area", etc? > > See my comment in AutofillProfileEditor about sharing these constants between > C++ and Java using the java_cpp_enum build rule. Done. https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:100: } On 2015/01/27 01:45:06, newt wrote: > Adding a default case seems like a good idea: > > default: > NOTREACHED(); Done. https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.h (right): https://codereview.chromium.org/872023002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/autofill/autofill_profile_bridge.h:10: namespace autofill { On 2015/01/27 01:45:07, newt wrote: > consensus on chromium-dev@ is not to use namespaces in src/chrome. Done.
https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... File chrome/android/java/res/layout/preference_float_label_layout.xml (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/preference_float_label_layout.xml:13: android:id="@+id/address_edit_text" this file seems generic but this id is "address_edit_text"? https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/preference_float_label_layout.xml:14: android:singleLine="true" street address must be multiline https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:22: namespace autofill https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:48: // from Android are in the format en_US and the C++ equivalent is en-US It's not Java vs. C++. Hypens are used in the IETF language tag. It looks like the underscores are coming from ::i18n::addressinput::GetRegionCodes(), not Android. Why are these not using hypens? Why does ::i18n::addressinput::BuildComponents expect something different from what ::i18n::addressinput::GetRegionCodes returns? We must have run into this somewhere else in the code.
https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:48: // from Android are in the format en_US and the C++ equivalent is en-US On 2015/01/28 03:07:47, Evan Stade wrote: > It's not Java vs. C++. Hypens are used in the IETF language tag. > > It looks like the underscores are coming from > ::i18n::addressinput::GetRegionCodes(), not Android. Why are these not using > hypens? Why does ::i18n::addressinput::BuildComponents expect something > different from what ::i18n::addressinput::GetRegionCodes returns? We must have > run into this somewhere else in the code. Also, beware the Java's Locale.getDefault() returns different values for a couple special locales (e.g. Locale.getDefault() returns "iw" for Hebrew, instead of the standard "he"). You'll need to be careful to compensate for this (we *might* have an existing function for this) if libaddressinput doesn't understand "iw" and the other non-standard language codes.
https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... File chrome/android/java/res/layout/preference_float_label_layout.xml (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/preference_float_label_layout.xml:13: android:id="@+id/address_edit_text" On 2015/01/28 03:07:47, Evan Stade wrote: > this file seems generic but this id is "address_edit_text"? The file name is poor, sorry. I'll change it. This layout is only intended to be used specifically for address fields since the inputType for the EditText is set to textPostalAddress|textCapWords. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/preference_float_label_layout.xml:14: android:singleLine="true" On 2015/01/28 03:07:47, Evan Stade wrote: > street address must be multiline The mocks call for a single street address line, and multiline street addresses are deprecated. They're marked as such in the libaddressinput's AddressField.java and the AddressField enum on the C++ side doesn't even include a placeholder for the deprecated address_line_1 and address_line_2 fields. I tested with multiline street addresses, and if there's a line break in the street_address, it's displayed as a space in the single line EditText box. https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:22: On 2015/01/28 03:07:47, Evan Stade wrote: > namespace autofill Re comment from newt@: consensus on chromium-dev@ is not to use namespaces in src/chrome. https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:48: // from Android are in the format en_US and the C++ equivalent is en-US On 2015/01/28 03:15:26, newt wrote: > On 2015/01/28 03:07:47, Evan Stade wrote: > > It's not Java vs. C++. Hypens are used in the IETF language tag. > > > > It looks like the underscores are coming from > > ::i18n::addressinput::GetRegionCodes(), not Android. Why are these not using > > hypens? Why does ::i18n::addressinput::BuildComponents expect something > > different from what ::i18n::addressinput::GetRegionCodes returns? We must have > > run into this somewhere else in the code. > > Also, beware the Java's Locale.getDefault() returns different values for a > couple special locales (e.g. Locale.getDefault() returns "iw" for Hebrew, > instead of the standard "he"). You'll need to be careful to compensate for this > (we *might* have an existing function for this) if libaddressinput doesn't > understand "iw" and the other non-standard language codes. estade@ - This string does come from Android, around line 214 in AutofillProfileEditor.java; it passes Locale.getDefault().toString() as an argument. I'll spend some more time looking at libaddressinput's language codes and Android's Locale and it's BCP-47 compliance (it looks like new methods were added in API level 21 for BCP-47 support. First, I'll try dropping the argument and just getting the current language tag from some c++ class; I hadn't realized Android was using non-standard (or a different standard) language tags.
PTAL - changes: get application language from C++ instead of Java, rename preference_float_label_layout.xml -> preference_address_float_label_layout.xml
https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... File chrome/android/java/res/layout/autofill_profile_editor.xml (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/autofill_profile_editor.xml:44: android:id="@+id/autofill_profile_editor_phone_number_label" Shorter IDs are OK too, e.g. autofill_phone_number_label or phone_number_label. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/autofill_profile_editor.xml:47: <EditText So, do you need to set the content description here and in other EditTexts? https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... File chrome/android/java/res/layout/homepage_preferences.xml (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/homepage_preferences.xml:22: android:layout_marginStart="@dimen/pref_homepage_layout_margin" Why is this margin applied only to the label and the switch? Why not to the entire page? https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:94: <!-- TODO(twellington): Remove/update the button dimens after we switch the PreferencesTheme update this comment https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AddressField.java (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AddressField.java:14: * src/third_party/libaddressinput/src/cpp/include/libaddressinput/address_field.h Trying to stay in sync with a third_party file is dangerous, since there's no comment in address_field.h warning developers to update this file when they update address_field.h. I'd only consider this acceptable if you have a unit test (not just a TODO) that will break if these get out of sync. Another option is to create your own copy of the AddressField enum in C++ and Java, then map from libaddressinput's AddressField to your own AddressField in autofill_profile_bridge.cc (like you were doing before, but using ints instead of strings). You could use java_cpp_enum to make your own AddressField constants identical between C++ and Java, or, since these enums are private to AutofillProfileBridge, it would be OK to manually create an enum in C++ and a list of ints in Java that are matching, with a comment in each place explaining that they need to stay in sync. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AddressField.java:19: public class AddressField { This should probably be package private (or even an inner class inside AutofillProfileBridge) https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:20: * @return A list of supported CLDR region codes. s/A/The https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:22: public static List<String> getAvailableCountries() { for consistent terminology, how about calling this getCountryCodes() or getSupportedCountryCodes()? https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:39: public static List<Pair<Integer, String>> getAddressUiComponents(String countryCode, I'd make this package private instead of public. It's too ad-hoc to be public IMO. Here's another way you could write this method: static String[] getAddressUiComponenets(...) where the return value is an array with 9 elements: one for each constant in AddressField.java. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:52: private static void stringArrayToList(String[] array, List<String> list) { Similar methods actually already exist in jni_array.h, e.g. ToJavaArrayOfStrings https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:80: mAddressFields = new FloatLabelLayout[AddressField.class.getDeclaredFields().length]; Avoid reflection at all costs please :O (Why? One reason: this could break as a result of an extremely innocent looking change to AddressField. E.g. if someone added an int to AddressField called ADDRESS_MASK.) Standard procedure would be to add an extra value to AddressField called NUM_FIELDS (or something like that), then use NUM_FIELDS here. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java:42: * <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" I'd remove the XML declaration and LinearLayout from this example. They don't add anything and makes it harder to see the interesting bit. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java (right): https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:144: String countryName = new Locale("", countryCode).getDisplayCountry(Locale.getDefault()); Is it possible that two countries could have the same name in some (unfortunate) language? That would cause trouble here. Another option is to create a list of Pair<String, String> objects and sort that, or create a Country class and sort of a list of Country objects: final Collator collator = Collator.getInstance(Locale.getDefault()); class Country implements Comparable<Country> { String name; String code; @Override public int compareTo(Country other) { return collator.compare(name, other.name); } } List<Country> countries = new ArrayList<Country>(); for (String countryCode : countryCodes) { Country country = new Country(); country.code = countryCode; country.name = new Locale("", countryCode).getDisplayCountry(Locale.getDefault()); countries.add(country); } Collections.sort(countries); mCountryCodes = new ArrayList<String>(); List<String> countryNames = new ArrayList<String>(); for (Country country : countries) { mCountryCodes.add(country.code); countryNames.add(country.name); } https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:150: Collator collator = Collator.getInstance(Locale.getDefault()); Examples of using Collator typically call collator.setStrength(Collator.PRIMARY). https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:151: java.util.Collections.sort(countryNames, collator); any reason not to import Collections? https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:173: mPhoneLabel.getLabel().setVisibility(View.VISIBLE); These three lines are repeated several times throughout this file: show a label, set the text, and change the hint to null. How about extracting that into a method, perhaps in FloatingLabelLayout. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:188: mCurrentCountryPos = mCountryCodes.indexOf(Locale.getDefault().getCountry()); This could still return -1. In that case, maybe just set mCurrentCountryPos to 0. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:201: mCurrentCountryPos = mCountryCodes.indexOf(Locale.getDefault().getCountry()); I think this will be broken for users who speak Hebrew, Indonesian, and maybe Norwegian. In those languages, Locale.getDefault().getCountry() will return alternative country codes (iw, in, no), which probably won't be in mCountryCodes. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:219: Integer fieldId = field.first; s/Integer/int https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:267: private String getFieldText(Integer fieldId) { s/Integer/int Use Integer as little as possible. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:274: private void setFieldText(Integer fieldId, String text) { s/Integer/int https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:275: if (mAddressFields[fieldId] != null && !TextUtils.isEmpty(text)) { What if you want to set the field to have empty text? Seems like you should remove the "!TextUtils.isEmpty()" check. Also, it is expected that mAddressFields[fieldId] might be null under normal conditions? (e.g. is mAddressFields[SORTING_CODE] null for U.S. profiles?) If not, then it seems the null check should be removed too.
https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java (right): https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:144: String countryName = new Locale("", countryCode).getDisplayCountry(Locale.getDefault()); On 2015/01/28 20:20:19, newt wrote: > Is it possible that two countries could have the same name in some (unfortunate) > language? That would cause trouble here. > > Another option is to create a list of Pair<String, String> objects and sort > that, or create a Country class and sort of a list of Country objects: > > final Collator collator = Collator.getInstance(Locale.getDefault()); > class Country implements Comparable<Country> { > String name; > String code; > > @Override > public int compareTo(Country other) { > return collator.compare(name, other.name); > } > } > > List<Country> countries = new ArrayList<Country>(); > for (String countryCode : countryCodes) { > Country country = new Country(); > country.code = countryCode; > country.name = new Locale("", > countryCode).getDisplayCountry(Locale.getDefault()); > countries.add(country); > } > Collections.sort(countries); > > mCountryCodes = new ArrayList<String>(); > List<String> countryNames = new ArrayList<String>(); > for (Country country : countries) { > mCountryCodes.add(country.code); > countryNames.add(country.name); > } Done. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:150: Collator collator = Collator.getInstance(Locale.getDefault()); On 2015/01/28 20:20:19, newt wrote: > Examples of using Collator typically call > collator.setStrength(Collator.PRIMARY). Done. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:151: java.util.Collections.sort(countryNames, collator); On 2015/01/28 20:20:19, newt wrote: > any reason not to import Collections? Done. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:173: mPhoneLabel.getLabel().setVisibility(View.VISIBLE); On 2015/01/28 20:20:19, newt wrote: > These three lines are repeated several times throughout this file: show a label, > set the text, and change the hint to null. How about extracting that into a > method, perhaps in FloatingLabelLayout. Done. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:188: mCurrentCountryPos = mCountryCodes.indexOf(Locale.getDefault().getCountry()); On 2015/01/28 20:20:19, newt wrote: > This could still return -1. In that case, maybe just set mCurrentCountryPos to > 0. Done. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:201: mCurrentCountryPos = mCountryCodes.indexOf(Locale.getDefault().getCountry()); On 2015/01/28 20:20:19, newt wrote: > I think this will be broken for users who speak Hebrew, Indonesian, and maybe > Norwegian. In those languages, Locale.getDefault().getCountry() will return > alternative country codes (iw, in, no), which probably won't be in > mCountryCodes. Okay, in that case, I don't think Android can be trusted, and I'll grab this value from the C++ code :) https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:219: Integer fieldId = field.first; On 2015/01/28 20:20:19, newt wrote: > s/Integer/int Done. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:267: private String getFieldText(Integer fieldId) { On 2015/01/28 20:20:19, newt wrote: > s/Integer/int > > Use Integer as little as possible. Done. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:274: private void setFieldText(Integer fieldId, String text) { On 2015/01/28 20:20:19, newt wrote: > s/Integer/int Done. https://codereview.chromium.org/872023002/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:275: if (mAddressFields[fieldId] != null && !TextUtils.isEmpty(text)) { On 2015/01/28 20:20:19, newt wrote: > What if you want to set the field to have empty text? Seems like you should > remove the "!TextUtils.isEmpty()" check. > > Also, it is expected that mAddressFields[fieldId] might be null under normal > conditions? (e.g. is mAddressFields[SORTING_CODE] null for U.S. profiles?) If > not, then it seems the null check should be removed too. The !empty and !null checks are here because this code makes the label visible and sets the hint text to none. If the profile data for a given field is null or an empty string, the UI shouldn't make the label visible and erase the hint. What would we gain by setting the field to have empty text? I can add an else block that just sets the EditText text (and doesn't change the hint or label visibility), I'm just not understanding why we would want to.
https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... File chrome/android/java/res/layout/preference_float_label_layout.xml (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/preference_float_label_layout.xml:14: android:singleLine="true" On 2015/01/28 03:33:47, twellington wrote: > On 2015/01/28 03:07:47, Evan Stade wrote: > > street address must be multiline > > The mocks call for a single street address line, and multiline street addresses > are deprecated. They're marked as such in the libaddressinput's > AddressField.java and the AddressField enum on the C++ side doesn't even include > a placeholder for the deprecated address_line_1 and address_line_2 fields. I > tested with multiline street addresses, and if there's a line break in the > street_address, it's displayed as a space in the single line EditText box. This is incorrect. Multiline street addresses are not deprecated. address_line_1 and address_line_2 are old, single-line inputs, and are deprecated because they should be replaced by a single, multiline street address of arbitrarily many lines. They don't appear in the mocks because 1) street addresses with just one line should look like one line and expand as necessary, and/or 2) the designer didn't consider the multiline case. https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:22: On 2015/01/28 03:33:47, twellington wrote: > On 2015/01/28 03:07:47, Evan Stade wrote: > > namespace autofill > > Re comment from newt@: consensus on chromium-dev@ is not to use namespaces in > src/chrome. The convention in autofill code in src/chrome is to use namespace autofill because it usually relies heavily on stuff in components/autofill, which uses that namespace. If you still don't want to add this namespace, you don't need ::i18n everywhere, which is only necessary to distinguish ::i18n from ::autofill::i18n.
PTAL addressed comments from newt@ and estade@ reviews https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... File chrome/android/java/res/layout/autofill_profile_editor.xml (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/autofill_profile_editor.xml:44: android:id="@+id/autofill_profile_editor_phone_number_label" On 2015/01/28 20:20:18, newt wrote: > Shorter IDs are OK too, e.g. autofill_phone_number_label or phone_number_label. Done. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/autofill_profile_editor.xml:47: <EditText On 2015/01/28 20:20:18, newt wrote: > So, do you need to set the content description here and in other EditTexts? Yes, done. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... File chrome/android/java/res/layout/homepage_preferences.xml (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/homepage_preferences.xml:22: android:layout_marginStart="@dimen/pref_homepage_layout_margin" On 2015/01/28 20:20:18, newt wrote: > Why is this margin applied only to the label and the switch? Why not to the > entire page? It shouldn't be. Done. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... File chrome/android/java/res/layout/preference_float_label_layout.xml (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/layout/preference_float_label_layout.xml:14: android:singleLine="true" On 2015/01/29 01:07:22, Evan Stade wrote: > On 2015/01/28 03:33:47, twellington wrote: > > On 2015/01/28 03:07:47, Evan Stade wrote: > > > street address must be multiline > > > > The mocks call for a single street address line, and multiline street > addresses > > are deprecated. They're marked as such in the libaddressinput's > > AddressField.java and the AddressField enum on the C++ side doesn't even > include > > a placeholder for the deprecated address_line_1 and address_line_2 fields. I > > tested with multiline street addresses, and if there's a line break in the > > street_address, it's displayed as a space in the single line EditText box. > > This is incorrect. Multiline street addresses are not deprecated. address_line_1 > and address_line_2 are old, single-line inputs, and are deprecated because they > should be replaced by a single, multiline street address of arbitrarily many > lines. > > They don't appear in the mocks because > 1) street addresses with just one line should look like one line and expand as > necessary, and/or > 2) the designer didn't consider the multiline case. Done. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:94: <!-- TODO(twellington): Remove/update the button dimens after we switch the PreferencesTheme On 2015/01/28 20:20:18, newt wrote: > update this comment Done. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AddressField.java (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AddressField.java:14: * src/third_party/libaddressinput/src/cpp/include/libaddressinput/address_field.h On 2015/01/28 20:20:18, newt wrote: > Trying to stay in sync with a third_party file is dangerous, since there's no > comment in address_field.h warning developers to update this file when they > update address_field.h. I'd only consider this acceptable if you have a unit > test (not just a TODO) that will break if these get out of sync. > > Another option is to create your own copy of the AddressField enum in C++ and > Java, then map from libaddressinput's AddressField to your own AddressField in > autofill_profile_bridge.cc (like you were doing before, but using ints instead > of strings). You could use java_cpp_enum to make your own AddressField constants > identical between C++ and Java, or, since these enums are private to > AutofillProfileBridge, it would be OK to manually create an enum in C++ and a > list of ints in Java that are matching, with a comment in each place explaining > that they need to stay in sync. I went with creating a copy of the AddressField enum in C++ and Java and mapping from libaddressinput's AddressField to my own AddressField. I forwent using java_cpp_enum in part because I need the NUM_FIELDS class member on the Java side. We should only have to update these fields if address_field.h changes which should be infrequently, and there are notes in both autofill_profile_bridge.cc and AutofillProfileBridge.java about keeping the AddressField enum/class in sync. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AddressField.java:19: public class AddressField { On 2015/01/28 20:20:18, newt wrote: > This should probably be package private (or even an inner class inside > AutofillProfileBridge) Done. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:20: * @return A list of supported CLDR region codes. On 2015/01/28 20:20:18, newt wrote: > s/A/The Done. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:22: public static List<String> getAvailableCountries() { On 2015/01/28 20:20:19, newt wrote: > for consistent terminology, how about calling this getCountryCodes() or > getSupportedCountryCodes()? Done, kind of. I changed it to getSupportedCountries() because this returns country codes and names now. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:39: public static List<Pair<Integer, String>> getAddressUiComponents(String countryCode, On 2015/01/28 20:20:18, newt wrote: > I'd make this package private instead of public. It's too ad-hoc to be public > IMO. > > Here's another way you could write this method: > > static String[] getAddressUiComponenets(...) > > where the return value is an array with 9 elements: one for each constant in > AddressField.java. I made it package protected. Just returning a String[] doesn't work because the field ordering isn't maintained; the order of AddressField's in componentIds is the order items should be displayed in the UI. I'll make that more clear in the comment. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:52: private static void stringArrayToList(String[] array, List<String> list) { On 2015/01/28 20:20:18, newt wrote: > Similar methods actually already exist in jni_array.h, e.g. ToJavaArrayOfStrings Yes, I saw that method; it doesn't do what I want I don't think. The method is primarily useful for returning a jojectArray. Since I need two lists of things for getAvailableCountries() and getAddressUiComponents(), using ToJavaArrayOfStrings to convert a std::vector to a String[] and returning doesn't quite work. I could/should have used it before for getAvailableCountries, but that method does more now. There's also SetObjectArrayElement in jni.h that I could use to populate arrays passed in from Java. I think this is also close to what I want, but I don't think it quite works. For getAvailableCountries, since the number of available countries is unknown, I can't (properly) initialize the arrays on the Java side. For getAddressUiComponents(), I do know the max # of fields, so I could use SetObjectArrayElement and just keep appending items to the country_id and country_names lists, but since I already need this method for getAvailableCountries it seems simpler/more consistent to use it for getAddressUiComponents too. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:80: mAddressFields = new FloatLabelLayout[AddressField.class.getDeclaredFields().length]; On 2015/01/28 20:20:19, newt wrote: > Avoid reflection at all costs please :O > > (Why? One reason: this could break as a result of an extremely innocent looking > change to AddressField. E.g. if someone added an int to AddressField called > ADDRESS_MASK.) > > Standard procedure would be to add an extra value to AddressField called > NUM_FIELDS (or something like that), then use NUM_FIELDS here. Done. https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/FloatLabelLayout.java:42: * <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2015/01/28 20:20:19, newt wrote: > I'd remove the XML declaration and LinearLayout from this example. They don't > add anything and makes it harder to see the interesting bit. Done. https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/140001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:22: On 2015/01/29 01:07:22, Evan Stade wrote: > On 2015/01/28 03:33:47, twellington wrote: > > On 2015/01/28 03:07:47, Evan Stade wrote: > > > namespace autofill > > > > Re comment from newt@: consensus on chromium-dev@ is not to use namespaces in > > src/chrome. > > The convention in autofill code in src/chrome is to use namespace autofill > because it usually relies heavily on stuff in components/autofill, which uses > that namespace. > > If you still don't want to add this namespace, you don't need ::i18n everywhere, > which is only necessary to distinguish ::i18n from ::autofill::i18n. I am using a method from components/autofill now, so I'll follow convention and add the autofill namespace.
https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/872023002/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:52: private static void stringArrayToList(String[] array, List<String> list) { On 2015/01/29 05:22:13, twellington wrote: > On 2015/01/28 20:20:18, newt wrote: > > Similar methods actually already exist in jni_array.h, e.g. > ToJavaArrayOfStrings > > Yes, I saw that method; it doesn't do what I want I don't think. The method is > primarily useful for returning a jojectArray. Since I need two lists of things > for getAvailableCountries() and getAddressUiComponents(), using > ToJavaArrayOfStrings to convert a std::vector to a String[] and returning > doesn't quite work. I could/should have used it before for > getAvailableCountries, but that method does more now. > > There's also SetObjectArrayElement in jni.h that I could use to populate arrays > passed in from Java. I think this is also close to what I want, but I don't > think it quite works. For getAvailableCountries, since the number of available > countries is unknown, I can't (properly) initialize the arrays on the Java side. > For getAddressUiComponents(), I do know the max # of fields, so I could use > SetObjectArrayElement and just keep appending items to the country_id and > country_names lists, but since I already need this method for > getAvailableCountries it seems simpler/more consistent to use it for > getAddressUiComponents too. Agreed. Sadly, despite the plethora of methods in jni_array.h, none of them are applicable for this class now. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... File chrome/android/java/res/layout/preference_address_float_label_layout.xml (right): https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... chrome/android/java/res/layout/preference_address_float_label_layout.xml:2: <!-- Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... chrome/android/java/res/layout/preference_address_float_label_layout.xml:8: xmlns:app="http://schemas.android.com/apk/res-auto" remove app namespace https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... chrome/android/java/res/layout/preference_address_float_label_layout.xml:11: android:id="@+id/float_label_layout" > put id about layout_* attributes https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... chrome/android/java/res/layout/preference_address_float_label_layout.xml:14: android:singleLine="true" put singleLine after layout_* attributes. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... chrome/android/java/res/layout/preference_address_float_label_layout.xml:17: android:inputType = "textPostalAddress|textCapWords" /> remove spaces around "=" https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:24: static final Collator sCollator = Collator.getInstance(Locale.getDefault()); There are lots of good reasons to avoid statics when possible. One of them is that they can never be garbage collected. I have a suggestion below for how to remove this one. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:59: * @return A list containing pairs of strings, where the first element in the pair is the it's not pairs of strings anymore https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:60: * component id (one of the static int's in AddressField), and the second element s/static ints/constants https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:101: * Address field types, ordered by size, from largest to smallest. What do you mean "ordered by size"? Size of what? https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:104: class AddressField { Put these classes inside of AutofillProfileBridge. You should never have multiple top level class definitions in one file. http://stackoverflow.com/questions/2336692/java-multiple-class-declarations-i... https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:105: public static final int COUNTRY = 0; I'd remove "public" since the class itself isn't public https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:140: public boolean equals(Object other) { Let's not go overboard with implementing methods that aren't used just to satisfy findbugs :) I'd remove equals(), hashCode(). Also, instead of making Country implement Comparable, use a Comparator that knows how to compare Country objects. To sort a list of countries, use this: final Collator collator = Collator.getInstance(Locale.getDefault()); Collections.sort(countries, new Comparator<Country>() { @Override public int compare(Country lhs, Country rhs) { int result = collator.compare(lhs.mName, rhs.mName); if (result == 0) result = lhs.mCode.compareTo(rhs.mCode); return result; } }); This will also allow you to remove the sCollator static variable. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java (right): https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:81: mPhoneLabel = don't need to wrap anymore. same below. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:188: mCurrentCountryPos = mCountryCodes.indexOf( We need the same logic here in case the default country code isn't in mCountryCodes. Or, can we now guarantee that mCountryCodes will contain the default country code? https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:197: // Save field text and reset mAddressFields. Maybe expand this comment a bit: "Save field text so we can restore it after updating the fields for the current country" https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:31: enum AddressField { put this enum in an anonymous namespace so it can't be seen outside this file https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:63: l10n_util::GetDisplayNameForCountry(country_code, one line? https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:83: std::string language_code; unused
https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java (right): https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:262: getFieldText(AddressField.STREET_ADDRESS), just checking --- this is a string with embedded newlines for the multiline case? https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:31: enum AddressField { sorry if you already explained it, but why does this exist? seems like it's duplicated
PTAL https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... File chrome/android/java/res/layout/preference_address_float_label_layout.xml (right): https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... chrome/android/java/res/layout/preference_address_float_label_layout.xml:2: <!-- Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/29 20:00:37, newt wrote: > 2015 Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... chrome/android/java/res/layout/preference_address_float_label_layout.xml:8: xmlns:app="http://schemas.android.com/apk/res-auto" On 2015/01/29 20:00:37, newt wrote: > remove app namespace Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... chrome/android/java/res/layout/preference_address_float_label_layout.xml:11: android:id="@+id/float_label_layout" > On 2015/01/29 20:00:37, newt wrote: > put id about layout_* attributes Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... chrome/android/java/res/layout/preference_address_float_label_layout.xml:14: android:singleLine="true" On 2015/01/29 20:00:37, newt wrote: > put singleLine after layout_* attributes. Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/res... chrome/android/java/res/layout/preference_address_float_label_layout.xml:17: android:inputType = "textPostalAddress|textCapWords" /> On 2015/01/29 20:00:37, newt wrote: > remove spaces around "=" Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:24: static final Collator sCollator = Collator.getInstance(Locale.getDefault()); On 2015/01/29 20:00:37, newt wrote: > There are lots of good reasons to avoid statics when possible. One of them is > that they can never be garbage collected. I have a suggestion below for how to > remove this one. Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:59: * @return A list containing pairs of strings, where the first element in the pair is the On 2015/01/29 20:00:37, newt wrote: > it's not pairs of strings anymore Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:60: * component id (one of the static int's in AddressField), and the second element On 2015/01/29 20:00:37, newt wrote: > s/static ints/constants Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:101: * Address field types, ordered by size, from largest to smallest. On 2015/01/29 20:00:37, newt wrote: > What do you mean "ordered by size"? Size of what? I think it means country > region (e.g. state) > locality (e.g. city)... so ordered in descending geographical size. But I copied the description from address_fields.h, and the relative ordering in the enum isn't important for us so I'll just delete that part of the comment. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:104: class AddressField { On 2015/01/29 20:00:37, newt wrote: > Put these classes inside of AutofillProfileBridge. You should never have > multiple top level class definitions in one file. > > http://stackoverflow.com/questions/2336692/java-multiple-class-declarations-i... Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:105: public static final int COUNTRY = 0; On 2015/01/29 20:00:37, newt wrote: > I'd remove "public" since the class itself isn't public Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:140: public boolean equals(Object other) { On 2015/01/29 20:00:37, newt wrote: > Let's not go overboard with implementing methods that aren't used just to > satisfy findbugs :) > > I'd remove equals(), hashCode(). Also, instead of making Country implement > Comparable, use a Comparator that knows how to compare Country objects. To sort > a list of countries, use this: > > final Collator collator = Collator.getInstance(Locale.getDefault()); > Collections.sort(countries, new Comparator<Country>() { > @Override > public int compare(Country lhs, Country rhs) { > int result = collator.compare(lhs.mName, rhs.mName); > if (result == 0) result = lhs.mCode.compareTo(rhs.mCode); > return result; > } > }); > > This will also allow you to remove the sCollator static variable. Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java (right): https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:81: mPhoneLabel = On 2015/01/29 20:00:37, newt wrote: > don't need to wrap anymore. same below. Done. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:188: mCurrentCountryPos = mCountryCodes.indexOf( On 2015/01/29 20:00:37, newt wrote: > We need the same logic here in case the default country code isn't in > mCountryCodes. Or, can we now guarantee that mCountryCodes will contain the > default country code? We need the same logic here; I'll add it. https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:197: // Save field text and reset mAddressFields. On 2015/01/29 20:00:37, newt wrote: > Maybe expand this comment a bit: "Save field text so we can restore it after > updating the fields for the current country" Done. https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:31: enum AddressField { On 2015/01/29 20:00:37, newt wrote: > put this enum in an anonymous namespace so it can't be seen outside this file Done. https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:63: l10n_util::GetDisplayNameForCountry(country_code, On 2015/01/29 20:00:37, newt wrote: > one line? Done. https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:83: std::string language_code; On 2015/01/29 20:00:37, newt wrote: > unused Done.
https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java (right): https://codereview.chromium.org/872023002/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:262: getFieldText(AddressField.STREET_ADDRESS), On 2015/01/29 22:29:57, Evan Stade wrote: > just checking --- this is a string with embedded newlines for the multiline > case? Yep :) There's a few lines of code above in resetFormFields: if (fieldId == AddressField.STREET_ADDRESS) { fieldEditText.setSingleLine(false); } I tested and it behaves like we want it to e.g. I can enter multiple lines for an address & save that off to a profile; When I open the profile to edit, the multiple street address lines are displayed as multiple lines in the street address field. I also tested the string being returned from getFieldText (for sanity) and it does have "\n" for the line breaks. https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:31: enum AddressField { On 2015/01/29 22:29:57, Evan Stade wrote: > sorry if you already explained it, but why does this exist? seems like it's > duplicated It is duplicated. It exists because we need the exact same enum/constants in this class and in the Java autofill classes (so we can use int's as address field id's). A few CL's back, I had a java_cpp_enum rule autogenerating the Java constants from address_field.h (in the libaddressinput library). I dropped that because it required changes to address_field.h (there was that email thread discussing). A couple CL's back, I just had a Java copy (of static constants that mapped to the same int values), but there was (very legitimate) concern about keeping the Java version in-sync with the third-party address_field.h. So I had these choices: 1) create a copy of address_field.h, make the needed changes and use java_cpp_enum to autogenerate 2) keep the Java version and write a unit test ensuring it was in-sync with the third-party enum 3) duplicate the enum here and on the Java side with notes in both places about keeping them in-sync, then map the address_field.h values to the enum values here (below in GetAddressUiComponents) I went with #3 because it allowed me to add a NUM_FIELDS constant on the Java side (for use in creating arrays) and we will know if these two enums get out of sync with address_field.h because the compiler will complain that the switch statement in GetAddressUiComponents doesn't cover all cases.
https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:31: enum AddressField { On 2015/01/29 22:49:42, twellington wrote: > On 2015/01/29 22:29:57, Evan Stade wrote: > > sorry if you already explained it, but why does this exist? seems like it's > > duplicated > > It is duplicated. It exists because we need the exact same enum/constants in > this class and in the Java autofill classes (so we can use int's as address > field id's). A few CL's back, I had a java_cpp_enum rule autogenerating the Java > constants from address_field.h (in the libaddressinput library). I dropped that > because it required changes to address_field.h (there was that email thread > discussing). A couple CL's back, I just had a Java copy (of static constants > that mapped to the same int values), but there was (very legitimate) concern > about keeping the Java version in-sync with the third-party address_field.h. > > So I had these choices: > 1) create a copy of address_field.h, make the needed changes and use > java_cpp_enum to autogenerate > 2) keep the Java version and write a unit test ensuring it was in-sync with the > third-party enum > 3) duplicate the enum here and on the Java side with notes in both places about > keeping them in-sync, then map the address_field.h values to the enum values > here (below in GetAddressUiComponents) > > I went with #3 because it allowed me to add a NUM_FIELDS constant on the Java > side (for use in creating arrays) and we will know if these two enums get out of > sync with address_field.h because the compiler will complain that the switch > statement in GetAddressUiComponents doesn't cover all cases. I think you should use ::i18n::addressinput::AddressField and write a unit test that makes sure the enums in java and C++ are in sync. Whoever rolls libaddressinput can see this test break, and fix stuff before rolling.
https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc (right): https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:31: enum AddressField { On 2015/01/30 00:02:17, Evan Stade wrote: > On 2015/01/29 22:49:42, twellington wrote: > > On 2015/01/29 22:29:57, Evan Stade wrote: > > > sorry if you already explained it, but why does this exist? seems like it's > > > duplicated > > > > It is duplicated. It exists because we need the exact same enum/constants in > > this class and in the Java autofill classes (so we can use int's as address > > field id's). A few CL's back, I had a java_cpp_enum rule autogenerating the > Java > > constants from address_field.h (in the libaddressinput library). I dropped > that > > because it required changes to address_field.h (there was that email thread > > discussing). A couple CL's back, I just had a Java copy (of static constants > > that mapped to the same int values), but there was (very legitimate) concern > > about keeping the Java version in-sync with the third-party address_field.h. > > > > So I had these choices: > > 1) create a copy of address_field.h, make the needed changes and use > > java_cpp_enum to autogenerate > > 2) keep the Java version and write a unit test ensuring it was in-sync with > the > > third-party enum > > 3) duplicate the enum here and on the Java side with notes in both places > about > > keeping them in-sync, then map the address_field.h values to the enum values > > here (below in GetAddressUiComponents) > > > > I went with #3 because it allowed me to add a NUM_FIELDS constant on the Java > > side (for use in creating arrays) and we will know if these two enums get out > of > > sync with address_field.h because the compiler will complain that the switch > > statement in GetAddressUiComponents doesn't cover all cases. > > I think you should use ::i18n::addressinput::AddressField and write a unit test > that makes sure the enums in java and C++ are in sync. Whoever rolls > libaddressinput can see this test break, and fix stuff before rolling. Whoever rolls libaddress could also see the build break and fix stuff before rolling, right? Honestly, I'm hesitant to change it again because I've already spent a week and a half on this address form rework and there are other, higher priority things in my queue. But if it has to change, I can drop the enum here and add some sort of COMPILE_ASSERT that uses JNI to grab the values from Java and check that they match the C++ values.
On 2015/01/30 00:37:06, twellington wrote: > https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... > File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc > (right): > > https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... > chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:31: enum > AddressField { > On 2015/01/30 00:02:17, Evan Stade wrote: > > On 2015/01/29 22:49:42, twellington wrote: > > > On 2015/01/29 22:29:57, Evan Stade wrote: > > > > sorry if you already explained it, but why does this exist? seems like > it's > > > > duplicated > > > > > > It is duplicated. It exists because we need the exact same enum/constants in > > > this class and in the Java autofill classes (so we can use int's as address > > > field id's). A few CL's back, I had a java_cpp_enum rule autogenerating the > > Java > > > constants from address_field.h (in the libaddressinput library). I dropped > > that > > > because it required changes to address_field.h (there was that email thread > > > discussing). A couple CL's back, I just had a Java copy (of static constants > > > that mapped to the same int values), but there was (very legitimate) concern > > > about keeping the Java version in-sync with the third-party address_field.h. > > > > > > So I had these choices: > > > 1) create a copy of address_field.h, make the needed changes and use > > > java_cpp_enum to autogenerate > > > 2) keep the Java version and write a unit test ensuring it was in-sync with > > the > > > third-party enum > > > 3) duplicate the enum here and on the Java side with notes in both places > > about > > > keeping them in-sync, then map the address_field.h values to the enum values > > > here (below in GetAddressUiComponents) > > > > > > I went with #3 because it allowed me to add a NUM_FIELDS constant on the > Java > > > side (for use in creating arrays) and we will know if these two enums get > out > > of > > > sync with address_field.h because the compiler will complain that the switch > > > statement in GetAddressUiComponents doesn't cover all cases. > > > > I think you should use ::i18n::addressinput::AddressField and write a unit > test > > that makes sure the enums in java and C++ are in sync. Whoever rolls > > libaddressinput can see this test break, and fix stuff before rolling. > > > Whoever rolls libaddress could also see the build break and fix stuff before > rolling, right? Honestly, I'm hesitant to change it again because I've already > spent a week and a half on this address form rework and there are other, higher > priority things in my queue. But if it has to change, I can drop the enum here > and add some sort of COMPILE_ASSERT that uses JNI to grab the values from Java > and check that they match the C++ values. I'm sorry that it's been a longer than expected process. A compile time assertion sgtm.
On 2015/01/30 00:46:45, Evan Stade wrote: > On 2015/01/30 00:37:06, twellington wrote: > > > https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... > > File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc > > (right): > > > > > https://codereview.chromium.org/872023002/diff/240001/chrome/browser/android/... > > chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:31: > enum > > AddressField { > > On 2015/01/30 00:02:17, Evan Stade wrote: > > > On 2015/01/29 22:49:42, twellington wrote: > > > > On 2015/01/29 22:29:57, Evan Stade wrote: > > > > > sorry if you already explained it, but why does this exist? seems like > > it's > > > > > duplicated > > > > > > > > It is duplicated. It exists because we need the exact same enum/constants > in > > > > this class and in the Java autofill classes (so we can use int's as > address > > > > field id's). A few CL's back, I had a java_cpp_enum rule autogenerating > the > > > Java > > > > constants from address_field.h (in the libaddressinput library). I dropped > > > that > > > > because it required changes to address_field.h (there was that email > thread > > > > discussing). A couple CL's back, I just had a Java copy (of static > constants > > > > that mapped to the same int values), but there was (very legitimate) > concern > > > > about keeping the Java version in-sync with the third-party > address_field.h. > > > > > > > > So I had these choices: > > > > 1) create a copy of address_field.h, make the needed changes and use > > > > java_cpp_enum to autogenerate > > > > 2) keep the Java version and write a unit test ensuring it was in-sync > with > > > the > > > > third-party enum > > > > 3) duplicate the enum here and on the Java side with notes in both places > > > about > > > > keeping them in-sync, then map the address_field.h values to the enum > values > > > > here (below in GetAddressUiComponents) > > > > > > > > I went with #3 because it allowed me to add a NUM_FIELDS constant on the > > Java > > > > side (for use in creating arrays) and we will know if these two enums get > > out > > > of > > > > sync with address_field.h because the compiler will complain that the > switch > > > > statement in GetAddressUiComponents doesn't cover all cases. > > > > > > I think you should use ::i18n::addressinput::AddressField and write a unit > > test > > > that makes sure the enums in java and C++ are in sync. Whoever rolls > > > libaddressinput can see this test break, and fix stuff before rolling. > > > > > > Whoever rolls libaddress could also see the build break and fix stuff before > > rolling, right? Honestly, I'm hesitant to change it again because I've already > > spent a week and a half on this address form rework and there are other, > higher > > priority things in my queue. But if it has to change, I can drop the enum > here > > and add some sort of COMPILE_ASSERT that uses JNI to grab the values from Java > > and check that they match the C++ values. > > I'm sorry that it's been a longer than expected process. A compile time > assertion sgtm. I suggested the current approach because writing a test that uses JNI to compare C++ and Java constants is unfortunately tricky and verbose, and COMPILE_ASSERTs definitely won't work with JNI. However, here's another reasonable approach: - Copy the constants into Java (already done in this CL) - In autofill_preference_bridge.cc, add COMPILE_ASSERTS to verify the values of the AddressField enum: COMPILE_ASSERT(::i18n::addressinput::COUNTRY == 0, java_cpp_enum_out_of_sync); etc - Put comments in both places (C++ and Java) instructing that the Java constants must be updated if the C++ enum changes. I'm also fine with keeping what's already in this CL.
https://codereview.chromium.org/872023002/diff/260001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/872023002/diff/260001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:45: * A convenience class for storing a CLDR country code and it's corresponding s/it's/its https://codereview.chromium.org/872023002/diff/260001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:52: public Country(String name, String code) { remove "public"
re: the enum -- I don't want to hold up the review. I guess we just have a bunch of bad options. lgtm
lgtm modulo two nits from the latest patch set. Thanks for making this work!
Landing soon :D https://codereview.chromium.org/872023002/diff/260001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/872023002/diff/260001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:45: * A convenience class for storing a CLDR country code and it's corresponding On 2015/01/30 01:32:31, newt wrote: > s/it's/its Done. https://codereview.chromium.org/872023002/diff/260001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:52: public Country(String name, String code) { On 2015/01/30 01:32:31, newt wrote: > remove "public" Done.
The CQ bit was checked by twellington@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/872023002/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/0bc6149395ed645213e16fa62ae3321dc0880537 Cr-Commit-Position: refs/heads/master@{#313973} |
