Turn Autofill country text input into a Spinner.

Turn Autofill country text input into a Spinner.

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

[Evan Stade, 2014-08-01 01:21:57]

This and the src/clank patch have to land at the same time or things will break.
Is that possible or do I have to do some weird dance like with Webkit?

https://codereview.chromium.org/436563005/diff/1/chrome/browser/android/chrom...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/android/chrom...
chrome/browser/android/chrome_jni_registrar.cc:92: {"DomDistiller",
dom_distiller::android::RegisterDomDistiller},
git cl format got over-zealous here... I can revert if it bothers you

[aurimas (slooooooooow), 2014-08-01 15:18:21]

Since this is a two sided patch, you will first have get both patches approved,
then land this one, as soon as it does you have to roll clank deps and land your
internal patch. I would really appreciate if you could wait until wednesday when
everyone in MTV is back in the office.

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
(right):

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:27:
LayoutInflater mInflater;
Can this be private?

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:104:
if (countryCode.equals(getItem(i)))
Doesn't this result in getCount() of JNI calls? JNI calls are fairly expensive
and we try to avoid call it unnecessarily. Can we get around that either passing
in the country codes in the constructor of this class or doing all this checking
in c++?

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:105:
return i;
move "return i;" to the same line with if statement. In java one line statements
either go on the same like with the if statement or you need to put curly braces
around it.

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:112:
public static CountryAdapter getInstance() {
Can you explain why are we using a static instance of the Country adapter
instead of just returning the real object? The static variable does not seem to
be unset anywhere either.

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:120:
private final long mCountryAdapterAndroid;
In java we put all member variable definitions to the top of the class.

https://codereview.chromium.org/436563005/diff/1/chrome/browser/autofill/andr...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/autofill/andr...
chrome/browser/autofill/android/personal_data_manager_android.cc:168:
ScopedJavaLocalRef<jobject> CreateJavaCountryFromNative(
Can you remove this?

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
File chrome/browser/ui/android/autofill/country_adapter_android.cc (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
chrome/browser/ui/android/autofill/country_adapter_android.cc:1: // Copyright
2013 The Chromium Authors. All rights reserved.
s/2013/2014

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
chrome/browser/ui/android/autofill/country_adapter_android.cc:20:
ScopedJavaLocalRef<jobject> CreateJavaCountryFromNative(
Can this be deleted?

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
File chrome/browser/ui/android/autofill/country_adapter_android.h (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
chrome/browser/ui/android/autofill/country_adapter_android.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
s/2013/2014

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
chrome/browser/ui/android/autofill/country_adapter_android.h:23:
base::android::ScopedJavaLocalRef<jstring>
I know its not really used in Chromium much, but we tend to set "using
base::android::ScopedJavaLocalRef;" [1] for easier readability. It is really up
to you though.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/net/android/keysto...

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/autofill/c...
File chrome/browser/ui/autofill/country_combobox_model.cc (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/autofill/c...
chrome/browser/ui/autofill/country_combobox_model.cc:42: #if
!defined(OS_ANDROID)
We don't cant separators in Android UI?

[Evan Stade, 2014-08-01 18:59:23]

OK, I'll wait till next week.

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
(right):

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:27:
LayoutInflater mInflater;
On 2014/08/01 15:18:21, aurimas wrote:
> Can this be private?

yes

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:104:
if (countryCode.equals(getItem(i)))
On 2014/08/01 15:18:21, aurimas wrote:
> Doesn't this result in getCount() of JNI calls? JNI calls are fairly expensive
> and we try to avoid call it unnecessarily. Can we get around that either
passing
> in the country codes in the constructor of this class or doing all this
checking
> in c++?

I was wondering about that. I figured JNI wasn't expensive because
PersonalDataManager calls nativeGetProfileByIndex repeatedly instead of having a
nativeGetProfilesByIndex that's called just once. I will rejigger this class to
reduce the number of native calls.

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:105:
return i;
On 2014/08/01 15:18:21, aurimas wrote:
> move "return i;" to the same line with if statement. In java one line
statements
> either go on the same like with the if statement or you need to put curly
braces
> around it.

Acknowledged.

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:112:
public static CountryAdapter getInstance() {
On 2014/08/01 15:18:21, aurimas wrote:
> Can you explain why are we using a static instance of the Country adapter
> instead of just returning the real object? The static variable does not seem
to
> be unset anywhere either.

What do you mean by the "real object"?

The reason we're using a singleton is because we don't actually need more than
one. There is no state that would vary from instance to instance. This follows
the example of PersonalDataManager.

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:120:
private final long mCountryAdapterAndroid;
On 2014/08/01 15:18:21, aurimas wrote:
> In java we put all member variable definitions to the top of the class.

OK. (I was basically copying PersonalDataManager.)

https://codereview.chromium.org/436563005/diff/1/chrome/browser/autofill/andr...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/autofill/andr...
chrome/browser/autofill/android/personal_data_manager_android.cc:168:
ScopedJavaLocalRef<jobject> CreateJavaCountryFromNative(
On 2014/08/01 15:18:21, aurimas wrote:
> Can you remove this?

oops. Yes.

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
File chrome/browser/ui/android/autofill/country_adapter_android.h (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
chrome/browser/ui/android/autofill/country_adapter_android.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2014/08/01 15:18:21, aurimas wrote:
> s/2013/2014

Acknowledged.

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
chrome/browser/ui/android/autofill/country_adapter_android.h:23:
base::android::ScopedJavaLocalRef<jstring>
On 2014/08/01 15:18:21, aurimas wrote:
> I know its not really used in Chromium much, but we tend to set "using
> base::android::ScopedJavaLocalRef;" [1] for easier readability. It is really
up
> to you though.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/android/keysto...

Only in .cc files

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/autofill/c...
File chrome/browser/ui/autofill/country_combobox_model.cc (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/autofill/c...
chrome/browser/ui/autofill/country_combobox_model.cc:42: #if
!defined(OS_ANDROID)
On 2014/08/01 15:18:21, aurimas wrote:
> We don't cant separators in Android UI?

there are already separators between every item in the Android UI.

[aurimas (slooooooooow), 2014-08-04 15:37:04]

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
(right):

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:112:
public static CountryAdapter getInstance() {
On 2014/08/01 18:59:23, Evan Stade wrote:
> On 2014/08/01 15:18:21, aurimas wrote:
> > Can you explain why are we using a static instance of the Country adapter
> > instead of just returning the real object? The static variable does not seem
> to
> > be unset anywhere either.
> 
> What do you mean by the "real object"?
> 
> The reason we're using a singleton is because we don't actually need more than
> one. There is no state that would vary from instance to instance. This follows
> the example of PersonalDataManager.

The way you are creating the CountryAdapter will make it stick around and use up
memory until chrome is killed since it is a static member variable.

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/autofill/c...
File chrome/browser/ui/autofill/country_combobox_model.cc (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/autofill/c...
chrome/browser/ui/autofill/country_combobox_model.cc:42: #if
!defined(OS_ANDROID)
On 2014/08/01 18:59:23, Evan Stade wrote:
> On 2014/08/01 15:18:21, aurimas wrote:
> > We don't cant separators in Android UI?
> 
> there are already separators between every item in the Android UI.

Can you add a comment explaining that?

[Evan Stade, 2014-08-04 20:36:05]

reduced the number of jni calls to 1.

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
(right):

https://codereview.chromium.org/436563005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:112:
public static CountryAdapter getInstance() {
On 2014/08/04 15:37:03, aurimas wrote:
> On 2014/08/01 18:59:23, Evan Stade wrote:
> > On 2014/08/01 15:18:21, aurimas wrote:
> > > Can you explain why are we using a static instance of the Country adapter
> > > instead of just returning the real object? The static variable does not
seem
> > to
> > > be unset anywhere either.
> > 
> > What do you mean by the "real object"?
> > 
> > The reason we're using a singleton is because we don't actually need more
than
> > one. There is no state that would vary from instance to instance. This
follows
> > the example of PersonalDataManager.
> 
> The way you are creating the CountryAdapter will make it stick around and use
up
> memory until chrome is killed since it is a static member variable.

That is how singletons tend to work. One benefit is that we avoid repeated
initialization.

https://codereview.chromium.org/436563005/diff/1/chrome/browser/autofill/andr...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/autofill/andr...
chrome/browser/autofill/android/personal_data_manager_android.cc:168:
ScopedJavaLocalRef<jobject> CreateJavaCountryFromNative(
On 2014/08/01 15:18:21, aurimas wrote:
> Can you remove this?

Done.

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
File chrome/browser/ui/android/autofill/country_adapter_android.cc (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
chrome/browser/ui/android/autofill/country_adapter_android.cc:20:
ScopedJavaLocalRef<jobject> CreateJavaCountryFromNative(
On 2014/08/01 15:18:21, aurimas wrote:
> Can this be deleted?

Done.

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
File chrome/browser/ui/android/autofill/country_adapter_android.h (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/android/au...
chrome/browser/ui/android/autofill/country_adapter_android.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2014/08/01 15:18:21, aurimas wrote:
> s/2013/2014

Done.

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/autofill/c...
File chrome/browser/ui/autofill/country_combobox_model.cc (right):

https://codereview.chromium.org/436563005/diff/1/chrome/browser/ui/autofill/c...
chrome/browser/ui/autofill/country_combobox_model.cc:42: #if
!defined(OS_ANDROID)
On 2014/08/04 15:37:03, aurimas wrote:
> On 2014/08/01 18:59:23, Evan Stade wrote:
> > On 2014/08/01 15:18:21, aurimas wrote:
> > > We don't cant separators in Android UI?
> > 
> > there are already separators between every item in the Android UI.
> 
> Can you add a comment explaining that?

Done.

[Evan Stade, 2014-08-06 22:30:02]

ping

[aurimas (slooooooooow), 2014-08-06 22:34:08]

lgtm

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/res/...
File chrome/android/java/res/layout/country_item.xml (right):

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/res/...
chrome/android/java/res/layout/country_item.xml:13: android:paddingBottom="10dp"
Since padding value is the same for every direction you can just use
android:padding="10dp"

[aurimas (slooooooooow), 2014-08-06 22:41:23]

Just saw few more things.

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
(right):

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:113:
if (countryCode.equals(getItem(i)))
Move return i; to the same line with the if statement.

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:121:
if (sAdapter == null) {
Chrome supports multi-instance mode (for samsung devices). Using a static
adapter would break if user would go to setting on two instances stuff would
break as convertViews would not be matching up correctly. Please make this into
non-static class.

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:122:
sAdapter = new CountryAdapter();
Indenting is wrong +2 spaces.

[Evan Stade, 2014-08-09 00:22:16]

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/res/...
File chrome/android/java/res/layout/country_item.xml (right):

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/res/...
chrome/android/java/res/layout/country_item.xml:13: android:paddingBottom="10dp"
On 2014/08/06 22:34:08, aurimas wrote:
> Since padding value is the same for every direction you can just use
> android:padding="10dp"

Done.

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
(right):

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:113:
if (countryCode.equals(getItem(i)))
On 2014/08/06 22:41:23, aurimas wrote:
> Move return i; to the same line with the if statement.

what rule governs the placement of the return? In AutofillProfileEditor.java, I
see:

if (extras != null) {
    mGUID = extras.getString(AutofillPreferences.AUTOFILL_GUID);
}

if (mGUID == null)
    mGUID = "";

if (profile == null) {
    return;
}

According to the style guide, it seems the middle one ought to have braces.
Otherwise, I can't find a reference to sharing a line between condition and
conditional body. The canonical Java file at [1] has an opportunity for this
near the bottom, but doesn't do it.

[1] https://x20web.corp.google.com/~kotzmann/java-style-codewalk/index.html

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:121:
if (sAdapter == null) {
On 2014/08/06 22:41:23, aurimas wrote:
> Chrome supports multi-instance mode (for samsung devices). Using a static
> adapter would break if user would go to setting on two instances stuff would
> break as convertViews would not be matching up correctly. Please make this
into
> non-static class.

Done.

https://codereview.chromium.org/436563005/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:122:
sAdapter = new CountryAdapter();
On 2014/08/06 22:41:23, aurimas wrote:
> Indenting is wrong +2 spaces.

fn removed

[aurimas (slooooooooow), 2014-08-12 00:01:34]

LGTM.

You will still need OWNERS review.

[Evan Stade, 2014-08-12 00:04:37]

Hey Ruslan and Ted, could I get OWNERS reviews for the following files:

aruslan:
  chrome/android/java/res/layout/country_item.xml
 
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
 
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java
 
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java
  chrome/browser/ui/android/autofill/country_adapter_android.cc
  chrome/browser/ui/android/autofill/country_adapter_android.h

tedchoc:
  chrome/browser/android/chrome_jni_registrar.cc

[Ted C, 2014-08-12 00:11:07]

On 2014/08/12 00:04:37, Evan Stade wrote:
> Hey Ruslan and Ted, could I get OWNERS reviews for the following files:
> 
> aruslan:
>   chrome/android/java/res/layout/country_item.xml
>  
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
>  
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java
>  
>
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java
>   chrome/browser/ui/android/autofill/country_adapter_android.cc
>   chrome/browser/ui/android/autofill/country_adapter_android.h
> 
> tedchoc:
>   chrome/browser/android/chrome_jni_registrar.cc

chrome_jni_registrar.cc - lgtm

[aruslan, 2014-08-12 00:18:39]

lgtm modulo seemingly incorrect ID of the country_item view and passing tests.

Please make sure you can build-n-run Chrome with this change.
I think the downstream AutofillProfileEditor saveProfile() is going to be
affected with this change.

https://codereview.chromium.org/436563005/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
(right):

https://codereview.chromium.org/436563005/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:21:
*
nit: unnecessary empty line.

https://codereview.chromium.org/436563005/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:66:
textView = (TextView) mInflater.inflate(R.layout.country_text, parent, false);
country_item?

[Evan Stade, 2014-08-12 00:33:36]

> Please make sure you can build-n-run Chrome with this change.
> I think the downstream AutofillProfileEditor saveProfile() is going to be
> affected with this change.

indeed --- see first response on this CL. The plan is to git cl dcommit this,
then land the clank patch and roll DEPS.

https://codereview.chromium.org/436563005/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
(right):

https://codereview.chromium.org/436563005/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:21:
*
On 2014/08/12 00:18:39, aruslan wrote:
> nit: unnecessary empty line.

removed

https://codereview.chromium.org/436563005/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:66:
textView = (TextView) mInflater.inflate(R.layout.country_text, parent, false);
On 2014/08/12 00:18:39, aruslan wrote:
> country_item?

oops, forgot to git add a file

[Evan Stade, 2014-08-12 15:24:10]

On 2014/08/12 00:33:36, Evan Stade wrote:
> > Please make sure you can build-n-run Chrome with this change.
> > I think the downstream AutofillProfileEditor saveProfile() is going to be
> > affected with this change.
> 
> indeed --- see first response on this CL. The plan is to git cl dcommit this,
> then land the clank patch and roll DEPS.

After talking to mariakhomenko@, s/git cl dcommit/CQ

> 
>
https://codereview.chromium.org/436563005/diff/60001/chrome/android/java/src/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java
> (right):
> 
>
https://codereview.chromium.org/436563005/diff/60001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:21:
> *
> On 2014/08/12 00:18:39, aruslan wrote:
> > nit: unnecessary empty line.
> 
> removed
> 
>
https://codereview.chromium.org/436563005/diff/60001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/CountryAdapter.java:66:
> textView = (TextView) mInflater.inflate(R.layout.country_text, parent, false);
> On 2014/08/12 00:18:39, aruslan wrote:
> > country_item?
> 
> oops, forgot to git add a file

[Evan Stade, 2014-08-12 15:24:15]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-12 15:28:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/436563005/80001

[Evan Stade, 2014-08-12 16:20:11]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-12 16:21:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/436563005/100001

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

Change committed as 289021