Redo Autofill wallet preferences on Android.

Redo Autofill wallet preferences on Android.

BUG=446700
Committed: https://crrev.com/ade5db8c29aa985c5dc5d1b00ce243448153ec3b
Cr-Commit-Position: refs/heads/master@{#321508}

[Evan Stade, 2015-03-16 23:26:33]

estade@chromium.org changed reviewers:
+ newt@chromium.org

[Evan Stade, 2015-03-16 23:26:33]

mocks:
https://folio.googleplex.com/payments-ux/Instant%20Buy%20UX/Zero%20integratio...

[newt (away), 2015-03-18 04:31:45]

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
File chrome/android/java/res/layout/autofill_server_card_clear_copy_button.xml
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
chrome/android/java/res/layout/autofill_server_card_clear_copy_button.xml:13:
android:minHeight="40dp"
By default, buttons have a minHeight of 36dp. We use this in other parts of
settings, so it'd be nice not to deviate here.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
File chrome/android/java/res/layout/autofill_server_data_label.xml (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
chrome/android/java/res/layout/autofill_server_data_label.xml:7: <TextView
xmlns:android="http://schemas.android.com/apk/res/android"
I'd check that this looks correct on both an L and a pre-L device. The
preference layout is pretty different in L, so it's not a given.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
File chrome/android/java/res/xml/autofill_server_card_preferences.xml (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
chrome/android/java/res/xml/autofill_server_card_preferences.xml:8:
<org.chromium.chrome.browser.preferences.ChromeBasePreference
FYI, you can use Preference instead of ChromeBasePreference if you don't need a
multi-line title or the option to make the preference managed.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/TextAndButtonPreference.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/TextAndButtonPreference.java:32:
View button = ((ViewGroup)
view.findViewById(android.R.id.widget_frame)).getChildAt(0);
A better way to get the button would be to call view.findViewById(R.id.button)
and require that the widget layout contain a view with that ID. Making
assumptions about the view hierarchy of a preference has caused a number of
problems in the past. e.g. on a Nook, there might be an extra layer between the
widget_frame and the actual widget.

Or, you could call setWidgetLayoutResource() in the constructor to set the
widget layout to a button, get the desired button text from a custom attribute
in the constructor, and in onBindView() set the button text. That'd be a cleaner
API, but is a bit more work.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/TextAndButtonPreference.java:33:
assert button != null;
I'd probably remove this assert. asserts are only enabled on developer builds,
and we'll get an equally helpful NPE on the next line anyway if button is null.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java:193:
CreditCard card = new CreditCard(mGUID, AutofillPreferences.SETTINGS_ORIGIN,
true, false,
I'd add /* */ comments to document what true and false mean here:

    true /* enableLasers */, etc

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java:195:
cardNumber, // obfuscated number, it will be obfuscated by native code.
?? is this comment right?

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillPreferences.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillPreferences.java:105:
PreferenceScreen screen =
getPreferenceManager().createPreferenceScreen(getActivity());
Does this need to be a PreferenceScreen? Seems like a Preference would suffice.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillServerCardPreferences.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillServerCardPreferences.java:47:
cardDescription.setSummary(card.getMonth() + "/" + card.getYear());
You should use Android's DateUtils or a similar class to format this date. For
one thing, string concatenation will convert integers to 0123456789, but some
languages use different numerals. I'd wouldn't be surprised if the date
separator varies by locale as well.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillWalletPreferences.java
(left):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillWalletPreferences.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
This file was deleted right? rietveld shows it as modified but not deleted for
some reason.

https://codereview.chromium.org/997713004/diff/120001/chrome/app/BUILD.gn
File chrome/app/BUILD.gn (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/app/BUILD.gn#new...
chrome/app/BUILD.gn:56: android_google_chrome_strings = [
You'll need a GYP change too for this. Also, the dependencies here don't look
complete to me. What will cause chrome_java to be compiled when
google_chrome_strings.grd changes?

Though I'm not sure that it's worth adding this additional build complexity to
be able to share a couple strings with chromium/google_chrome_strings.grd.
Unless we plan to share a lot of strings in the future, I'd probably avoid this.

https://codereview.chromium.org/997713004/diff/120001/chrome/browser/autofill...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.cc:307: //
personal_data_manager_->ResetFullServerCard(ConvertJavaStringToUTF8(env,
TODO?

https://codereview.chromium.org/997713004/diff/120001/chrome/browser/autofill...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.h:87: // Rests all
unmasked cards back to the masked state.
"Resets"

[Evan Stade, 2015-03-19 00:24:23]

haven't tried on K yet, working on doing so.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
File chrome/android/java/res/layout/autofill_server_card_clear_copy_button.xml
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
chrome/android/java/res/layout/autofill_server_card_clear_copy_button.xml:13:
android:minHeight="40dp"
On 2015/03/18 04:31:44, newt wrote:
> By default, buttons have a minHeight of 36dp. We use this in other parts of
> settings, so it'd be nice not to deviate here.

This is actually copied from preference_button.xml. As you can see, I forgot to
delete android:id="@+id/button_preference" (or I might should leave it to use it
in findViewById as you suggetsed).

The reason I didn't just reuse preference_button.xml is because I couldn't
figure out how to set the text in an elegant way (also the extra LinearLayout
wrapper was not needed).

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
File chrome/android/java/res/layout/autofill_server_data_label.xml (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
chrome/android/java/res/layout/autofill_server_data_label.xml:7: <TextView
xmlns:android="http://schemas.android.com/apk/res/android"
On 2015/03/18 04:31:45, newt wrote:
> I'd check that this looks correct on both an L and a pre-L device. The
> preference layout is pretty different in L, so it's not a given.

I don't have a pre-L device. What kind should I request?

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
File chrome/android/java/res/xml/autofill_server_card_preferences.xml (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
chrome/android/java/res/xml/autofill_server_card_preferences.xml:8:
<org.chromium.chrome.browser.preferences.ChromeBasePreference
On 2015/03/18 04:31:45, newt wrote:
> FYI, you can use Preference instead of ChromeBasePreference if you don't need
a
> multi-line title or the option to make the preference managed.

Done.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/TextAndButtonPreference.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/TextAndButtonPreference.java:32:
View button = ((ViewGroup)
view.findViewById(android.R.id.widget_frame)).getChildAt(0);
On 2015/03/18 04:31:45, newt wrote:
> A better way to get the button would be to call view.findViewById(R.id.button)
> and require that the widget layout contain a view with that ID. Making
> assumptions about the view hierarchy of a preference has caused a number of
> problems in the past. e.g. on a Nook, there might be an extra layer between
the
> widget_frame and the actual widget.

Done.

> 
> Or, you could call setWidgetLayoutResource() in the constructor to set the
> widget layout to a button, get the desired button text from a custom attribute
> in the constructor, and in onBindView() set the button text. That'd be a
cleaner
> API, but is a bit more work.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/TextAndButtonPreference.java:33:
assert button != null;
On 2015/03/18 04:31:45, newt wrote:
> I'd probably remove this assert. asserts are only enabled on developer builds,
> and we'll get an equally helpful NPE on the next line anyway if button is
null.

Done.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java:193:
CreditCard card = new CreditCard(mGUID, AutofillPreferences.SETTINGS_ORIGIN,
true, false,
On 2015/03/18 04:31:45, newt wrote:
> I'd add /* */ comments to document what true and false mean here:
> 
>     true /* enableLasers */, etc

Done.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java:195:
cardNumber, // obfuscated number, it will be obfuscated by native code.
On 2015/03/18 04:31:45, newt wrote:
> ?? is this comment right?

I think the point is actually that native code doesn't read this value, so it
doesn't matter what we put here. Updated the comment.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillPreferences.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillPreferences.java:105:
PreferenceScreen screen =
getPreferenceManager().createPreferenceScreen(getActivity());
On 2015/03/18 04:31:45, newt wrote:
> Does this need to be a PreferenceScreen? Seems like a Preference would
suffice.

I don't know why it was a screen. Normal pref seems to work.

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillServerCardPreferences.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillServerCardPreferences.java:47:
cardDescription.setSummary(card.getMonth() + "/" + card.getYear());
On 2015/03/18 04:31:45, newt wrote:
> You should use Android's DateUtils or a similar class to format this date. For
> one thing, string concatenation will convert integers to 0123456789, but some
> languages use different numerals.

I have never heard that other locales don't use Arabic numerals --- can you give
an example? I looked at DateUtils and SimpleDateFormat and neither seemed like
they had something useful. getBestDateTimePattern only has examples for month +
day varying by locale.

I have actually looked into this before because it's relevant to how Chrome
fills in the webpage or shows expiration dates in the suggestion dropdown. I
couldn't find any evidence that an expiration date such as "12/2016" was invalid
in some locale.

> I'd wouldn't be surprised if the date
> separator varies by locale as well.

yea there is a string we can use here --- done. I'm still skeptical/curious if
it's going to be translated to something other than "/".

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillWalletPreferences.java
(left):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillWalletPreferences.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/03/18 04:31:44, newt wrote:
> This file was deleted right? rietveld shows it as modified but not deleted for
> some reason.

yes

https://codereview.chromium.org/997713004/diff/120001/chrome/app/BUILD.gn
File chrome/app/BUILD.gn (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/app/BUILD.gn#new...
chrome/app/BUILD.gn:56: android_google_chrome_strings = [
On 2015/03/18 04:31:45, newt wrote:
> You'll need a GYP change too for this. Also, the dependencies here don't look
> complete to me. What will cause chrome_java to be compiled when
> google_chrome_strings.grd changes?
> 
> Though I'm not sure that it's worth adding this additional build complexity to
> be able to share a couple strings with chromium/google_chrome_strings.grd.
> Unless we plan to share a lot of strings in the future, I'd probably avoid
this.

Hmm, I guess I can remove these changes, but I don't understand why we seem to
use the Chrome terminology even in Chromium on android.

https://codereview.chromium.org/997713004/diff/120001/chrome/browser/autofill...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.cc:307: //
personal_data_manager_->ResetFullServerCard(ConvertJavaStringToUTF8(env,
On 2015/03/18 04:31:45, newt wrote:
> TODO?

I have no recollection of commenting this out

https://codereview.chromium.org/997713004/diff/120001/chrome/browser/autofill...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.h:87: // Rests all
unmasked cards back to the masked state.
On 2015/03/18 04:31:45, newt wrote:
> "Resets"

Done.

[Evan Stade, 2015-03-19 00:24:58]

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
File chrome/android/java/res/layout/autofill_server_data_label.xml (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
chrome/android/java/res/layout/autofill_server_data_label.xml:7: <TextView
xmlns:android="http://schemas.android.com/apk/res/android"
On 2015/03/19 00:24:22, Evan Stade wrote:
> On 2015/03/18 04:31:45, newt wrote:
> > I'd check that this looks correct on both an L and a pre-L device. The
> > preference layout is pretty different in L, so it's not a given.
> 
> I don't have a pre-L device. What kind should I request?

ignore this

[newt (away), 2015-03-19 01:41:43]

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
File chrome/android/java/res/layout/autofill_server_card_clear_copy_button.xml
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/res...
chrome/android/java/res/layout/autofill_server_card_clear_copy_button.xml:13:
android:minHeight="40dp"
On 2015/03/19 00:24:22, Evan Stade wrote:
> On 2015/03/18 04:31:44, newt wrote:
> > By default, buttons have a minHeight of 36dp. We use this in other parts of
> > settings, so it'd be nice not to deviate here.
> 
> This is actually copied from preference_button.xml. As you can see, I forgot
to
> delete android:id="@+id/button_preference" (or I might should leave it to use
it
> in findViewById as you suggetsed).
> 
> The reason I didn't just reuse preference_button.xml is because I couldn't
> figure out how to set the text in an elegant way (also the extra LinearLayout
> wrapper was not needed).

Ah, my bad. Didn't realize we override the height in preference_button.xml too

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillServerCardPreferences.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillServerCardPreferences.java:47:
cardDescription.setSummary(card.getMonth() + "/" + card.getYear());
On 2015/03/19 00:24:22, Evan Stade wrote:
> On 2015/03/18 04:31:45, newt wrote:
> > You should use Android's DateUtils or a similar class to format this date.
For
> > one thing, string concatenation will convert integers to 0123456789, but
some
> > languages use different numerals.
> 
> I have never heard that other locales don't use Arabic numerals --- can you
give
> an example? I looked at DateUtils and SimpleDateFormat and neither seemed like
> they had something useful. getBestDateTimePattern only has examples for month
+
> day varying by locale.

http://en.wikipedia.org/wiki/Eastern_Arabic_numerals

Also, some less common languages: Bengali, Marathi, Nepalese

Arabic: ١٨/‏٣
Bengali: ১৮/৩
Marathi: १८/३
Nepali: ०३-१८

> 
> I have actually looked into this before because it's relevant to how Chrome
> fills in the webpage or shows expiration dates in the suggestion dropdown. I
> couldn't find any evidence that an expiration date such as "12/2016" was
invalid
> in some locale.
> 
> > I'd wouldn't be surprised if the date
> > separator varies by locale as well.
> 
> yea there is a string we can use here --- done. I'm still skeptical/curious if
> it's going to be translated to something other than "/".

I tried formatting dates for all the available locales on my device. There are
lots of different separators used:

    Filipino: 3/18
     Faroese: 03-18
      French: 18/3
       Irish: 18/03
    Galician: 18-3
Swiss German: 18.3.
    Gujarati: 18/3
       Gusii: 3/18
        Manx: 03-18
       Hausa: 3/18
    Hawaiian: 18/3
      Hebrew: 18/3
       Hindi: 18/3
    Croatian: 18. 3.
   Hungarian: 3. 18.
    Armenian: 18.03
  Indonesian: 18/3
        Igbo: 3/18

I was using DateUtils.formatDateTime(context, System.currentTimeMillis(),
DateUtils.FORMAT_SHOW_DATE | DateUtils.FORMAT_NUMERIC_DATE). You should be able
to use that or maybe SimpleDateFormat.

https://codereview.chromium.org/997713004/diff/120001/chrome/app/BUILD.gn
File chrome/app/BUILD.gn (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/app/BUILD.gn#new...
chrome/app/BUILD.gn:56: android_google_chrome_strings = [
On 2015/03/19 00:24:23, Evan Stade wrote:
> On 2015/03/18 04:31:45, newt wrote:
> > You'll need a GYP change too for this. Also, the dependencies here don't
look
> > complete to me. What will cause chrome_java to be compiled when
> > google_chrome_strings.grd changes?
> > 
> > Though I'm not sure that it's worth adding this additional build complexity
to
> > be able to share a couple strings with chromium/google_chrome_strings.grd.
> > Unless we plan to share a lot of strings in the future, I'd probably avoid
> this.
> 
> Hmm, I guess I can remove these changes, but I don't understand why we seem to
> use the Chrome terminology even in Chromium on android.

I'd like to change this, but do so in a way that doesn't require duplicating
each string that contains "Chrome".

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/res...
File chrome/android/java/res/layout/autofill_server_data_edit_link.xml (right):

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/res...
chrome/android/java/res/layout/autofill_server_data_edit_link.xml:8:
android:id="@+id/preference_click_target"
call this edit_link?

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java
(right):

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java:195:
cardNumber, new String() /* obfuscatedNumber */,
You could pass "" instead of new String() to avoid unneeded object creation.

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillPreferences.java
(right):

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillPreferences.java:129:
pref.setSummary(card.getMonth()
This still won't work. In Arabic, this would result in: 3.15, when we really
want ٥١.٣, or something like that. DateUtils is your friend :)

[Evan Stade, 2015-03-20 00:14:57]

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillServerCardPreferences.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillServerCardPreferences.java:47:
cardDescription.setSummary(card.getMonth() + "/" + card.getYear());
On 2015/03/19 01:41:42, newt wrote:
> On 2015/03/19 00:24:22, Evan Stade wrote:
> > On 2015/03/18 04:31:45, newt wrote:
> > > You should use Android's DateUtils or a similar class to format this date.
> For
> > > one thing, string concatenation will convert integers to 0123456789, but
> some
> > > languages use different numerals.
> > 
> > I have never heard that other locales don't use Arabic numerals --- can you
> give
> > an example? I looked at DateUtils and SimpleDateFormat and neither seemed
like
> > they had something useful. getBestDateTimePattern only has examples for
month
> +
> > day varying by locale.
> 
> http://en.wikipedia.org/wiki/Eastern_Arabic_numerals
> 
> Also, some less common languages: Bengali, Marathi, Nepalese
> 
> Arabic: ١٨/‏٣
> Bengali: ১৮/৩
> Marathi: १८/३
> Nepali: ०३-१८

getMonth and getYear are strings so the responsibility of converting from number
to locale-aware string is upstream from here.

> 
> > 
> > I have actually looked into this before because it's relevant to how Chrome
> > fills in the webpage or shows expiration dates in the suggestion dropdown. I
> > couldn't find any evidence that an expiration date such as "12/2016" was
> invalid
> > in some locale.
> > 
> > > I'd wouldn't be surprised if the date
> > > separator varies by locale as well.
> > 
> > yea there is a string we can use here --- done. I'm still skeptical/curious
if
> > it's going to be translated to something other than "/".
> 
> I tried formatting dates for all the available locales on my device. There are
> lots of different separators used:
> 
>     Filipino: 3/18
>      Faroese: 03-18
>       French: 18/3
>        Irish: 18/03
>     Galician: 18-3
> Swiss German: 18.3.
>     Gujarati: 18/3
>        Gusii: 3/18
>         Manx: 03-18
>        Hausa: 3/18
>     Hawaiian: 18/3
>       Hebrew: 18/3
>        Hindi: 18/3
>     Croatian: 18. 3.
>    Hungarian: 3. 18.
>     Armenian: 18.03
>   Indonesian: 18/3
>         Igbo: 3/18
> 
> I was using DateUtils.formatDateTime(context, System.currentTimeMillis(),
> DateUtils.FORMAT_SHOW_DATE | DateUtils.FORMAT_NUMERIC_DATE). You should be
able
> to use that or maybe SimpleDateFormat.

This example is month and day, not month and year. The order of month/day
differs by locale, I still think the order of month/year is the same everywhere
(except in RTL languages of course, but that doesn't affect the order of
characters in the string).

I still believe we want mm(separator)yyyy everywhere. It's what the rest of
Chrome does, which is not to say it's right, but I've never seen a bug filed
about this.

https://codereview.chromium.org/997713004/diff/120001/chrome/app/BUILD.gn
File chrome/app/BUILD.gn (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/app/BUILD.gn#new...
chrome/app/BUILD.gn:56: android_google_chrome_strings = [
On 2015/03/19 01:41:42, newt wrote:
> On 2015/03/19 00:24:23, Evan Stade wrote:
> > On 2015/03/18 04:31:45, newt wrote:
> > > You'll need a GYP change too for this. Also, the dependencies here don't
> look
> > > complete to me. What will cause chrome_java to be compiled when
> > > google_chrome_strings.grd changes?
> > > 
> > > Though I'm not sure that it's worth adding this additional build
complexity
> to
> > > be able to share a couple strings with chromium/google_chrome_strings.grd.
> > > Unless we plan to share a lot of strings in the future, I'd probably avoid
> > this.
> > 
> > Hmm, I guess I can remove these changes, but I don't understand why we seem
to
> > use the Chrome terminology even in Chromium on android.
> 
> I'd like to change this, but do so in a way that doesn't require duplicating
> each string that contains "Chrome".

If you're planning to use placeholders, I think that's fraught with peril in
terms of translation quality. Desktop Chrome does duplicate strings with
"Chrome"/"Chromium" after moving away from the use of placeholders.

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/res...
File chrome/android/java/res/layout/autofill_server_data_edit_link.xml (right):

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/res...
chrome/android/java/res/layout/autofill_server_data_edit_link.xml:8:
android:id="@+id/preference_click_target"
On 2015/03/19 01:41:43, newt wrote:
> call this edit_link?

TextAndButtonPreference requires this id

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java
(right):

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java:195:
cardNumber, new String() /* obfuscatedNumber */,
On 2015/03/19 01:41:43, newt wrote:
> You could pass "" instead of new String() to avoid unneeded object creation.

Done.

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillPreferences.java
(right):

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillPreferences.java:129:
pref.setSummary(card.getMonth()
On 2015/03/19 01:41:43, newt wrote:
> This still won't work. In Arabic, this would result in: 3.15, when we really
> want ٥١.٣, or something like that. DateUtils is your friend :)

In Arabic text is displayed RTL, which handles the ordering. getMonth is a
string which should already be ٣ or whatever.

[newt (away), 2015-03-20 00:53:45]

lgtm

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillServerCardPreferences.java
(right):

https://codereview.chromium.org/997713004/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillServerCardPreferences.java:47:
cardDescription.setSummary(card.getMonth() + "/" + card.getYear());
On 2015/03/20 00:14:56, Evan Stade wrote:
> On 2015/03/19 01:41:42, newt wrote:
> > On 2015/03/19 00:24:22, Evan Stade wrote:
> > > On 2015/03/18 04:31:45, newt wrote:
> > > > You should use Android's DateUtils or a similar class to format this
date.
> > For
> > > > one thing, string concatenation will convert integers to 0123456789, but
> > some
> > > > languages use different numerals.
> > > 
> > > I have never heard that other locales don't use Arabic numerals --- can
you
> > give
> > > an example? I looked at DateUtils and SimpleDateFormat and neither seemed
> like
> > > they had something useful. getBestDateTimePattern only has examples for
> month
> > +
> > > day varying by locale.
> > 
> > http://en.wikipedia.org/wiki/Eastern_Arabic_numerals
> > 
> > Also, some less common languages: Bengali, Marathi, Nepalese
> > 
> > Arabic: ١٨/‏٣
> > Bengali: ১৮/৩
> > Marathi: १८/३
> > Nepali: ०३-१८
> 
> getMonth and getYear are strings so the responsibility of converting from
number
> to locale-aware string is upstream from here.

Gotcha.

> > 
> > > 
> > > I have actually looked into this before because it's relevant to how
Chrome
> > > fills in the webpage or shows expiration dates in the suggestion dropdown.
I
> > > couldn't find any evidence that an expiration date such as "12/2016" was
> > invalid
> > > in some locale.
> > > 
> > > > I'd wouldn't be surprised if the date
> > > > separator varies by locale as well.
> > > 
> > > yea there is a string we can use here --- done. I'm still
skeptical/curious
> if
> > > it's going to be translated to something other than "/".
> > 
> > I tried formatting dates for all the available locales on my device. There
are
> > lots of different separators used:
> > 
> >     Filipino: 3/18
> >      Faroese: 03-18
> >       French: 18/3
> >        Irish: 18/03
> >     Galician: 18-3
> > Swiss German: 18.3.
> >     Gujarati: 18/3
> >        Gusii: 3/18
> >         Manx: 03-18
> >        Hausa: 3/18
> >     Hawaiian: 18/3
> >       Hebrew: 18/3
> >        Hindi: 18/3
> >     Croatian: 18. 3.
> >    Hungarian: 3. 18.
> >     Armenian: 18.03
> >   Indonesian: 18/3
> >         Igbo: 3/18
> > 
> > I was using DateUtils.formatDateTime(context, System.currentTimeMillis(),
> > DateUtils.FORMAT_SHOW_DATE | DateUtils.FORMAT_NUMERIC_DATE). You should be
> able
> > to use that or maybe SimpleDateFormat.
> 
> This example is month and day, not month and year. The order of month/day
> differs by locale, I still think the order of month/year is the same
everywhere
> (except in RTL languages of course, but that doesn't affect the order of
> characters in the string).
> 
> I still believe we want mm(separator)yyyy everywhere. It's what the rest of
> Chrome does, which is not to say it's right, but I've never seen a bug filed
> about this.
> 

Ok, I'll buy that.

https://codereview.chromium.org/997713004/diff/120001/chrome/app/BUILD.gn
File chrome/app/BUILD.gn (right):

https://codereview.chromium.org/997713004/diff/120001/chrome/app/BUILD.gn#new...
chrome/app/BUILD.gn:56: android_google_chrome_strings = [
On 2015/03/20 00:14:56, Evan Stade wrote:
> On 2015/03/19 01:41:42, newt wrote:
> > On 2015/03/19 00:24:23, Evan Stade wrote:
> > > On 2015/03/18 04:31:45, newt wrote:
> > > > You'll need a GYP change too for this. Also, the dependencies here don't
> > look
> > > > complete to me. What will cause chrome_java to be compiled when
> > > > google_chrome_strings.grd changes?
> > > > 
> > > > Though I'm not sure that it's worth adding this additional build
> complexity
> > to
> > > > be able to share a couple strings with
chromium/google_chrome_strings.grd.
> > > > Unless we plan to share a lot of strings in the future, I'd probably
avoid
> > > this.
> > > 
> > > Hmm, I guess I can remove these changes, but I don't understand why we
seem
> to
> > > use the Chrome terminology even in Chromium on android.
> > 
> > I'd like to change this, but do so in a way that doesn't require duplicating
> > each string that contains "Chrome".
> 
> If you're planning to use placeholders, I think that's fraught with peril in
> terms of translation quality. Desktop Chrome does duplicate strings with
> "Chrome"/"Chromium" after moving away from the use of placeholders.

The placeholder solution could be done in a way that doesn't affect
translations, by e.g. replacing the placeholder with "Chrome" before sending it
to the translators, so the translators would see a placeholder-less string. This
wouldn't provide translations for Chromium, but those could be provided by
anyone who wanted to provide them.

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillPreferences.java
(right):

https://codereview.chromium.org/997713004/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillPreferences.java:129:
pref.setSummary(card.getMonth()
On 2015/03/20 00:14:57, Evan Stade wrote:
> On 2015/03/19 01:41:43, newt wrote:
> > This still won't work. In Arabic, this would result in: 3.15, when we really
> > want ٥١.٣, or something like that. DateUtils is your friend :)
> 
> In Arabic text is displayed RTL, which handles the ordering. getMonth is a
> string which should already be ٣ or whatever.

Didn't realize it was already a string. Sounds OK then.

[Evan Stade, 2015-03-20 02:14:57]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-03-20 02:15:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/997713004/160001

[commit-bot: I haz the power, 2015-03-20 03:28:10]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-20 03:28:51]

Patchset 9 (id:??) landed as
https://crrev.com/ade5db8c29aa985c5dc5d1b00ce243448153ec3b
Cr-Commit-Position: refs/heads/master@{#321508}