Initial UI structure for autofill dialog

Initial UI structure for autofill dialog

This is the basic UI structure that has some of the trnasitions and
animations build in. The dialog is only initialized only in related
autofill code and this is mainly to get a UI structure committed to
build around to. The placeholders will be removed and some animations
and control structures may eventually change.

BUG=177380
NOTRY=true

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

[Ted C, 2013-03-06 01:41:56]

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File chrome/android/java/res/layout/autofill_address_spinner_dropdown.xml
(right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_address_spinner_dropdown.xml:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
Hmm...does this work?

I thought we needed to do the XML comment style:
<!-- Copyright (c) 2013 The Chromium Authors. All rights reserved.

   Use of this source code is governed by a BSD-style license that can be
   found in the LICENSE file.
-->

Might want to do it that way just to be consistent.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_address_spinner_dropdown.xml:4: <?xml
version="1.0" encoding="utf-8"?>
Also, this has gone above the copyright in all other files, so we should do it
here too.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File chrome/android/java/res/layout/autofill_dialog_content.xml (right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:13: <TextView
All of the children of this gridlayout should be intended.  Same for below.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:53: android:text="By
clicking the below button, you agree to the terms of service. And also some
other text that makes sure this is not one line"
add this to strings.xml

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:74:
android:layout_columnSpan="4"/>
You'll also want to see how this behaves in accessibility mode as you might need
description strings for all these.  Maybe just a TODO for now.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:80:
android:hint="MM"/>
these should probably be in strings.xml as well

applies to all the hints.  Unless these come from native in which case you
should remove them.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:146: <GridLayout
I would pull these three grid layouts into separate layout files if possible.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:191: </GridLayout>
indent messed up.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File chrome/android/java/res/layout/autofill_dialog_title.xml (right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_title.xml:6: <FrameLayout
this block should be indented 4 and it's 2 here and 3 for the closing tag

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_title.xml:22: </merge>
decrease indent.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java
(right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:27:
public class AutofillDialog extends AlertDialog implements OnClickListener,
I would put all the implements on the second line to make it easier to read.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:28:
OnItemSelectedListener {
+4 indent

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:33:
AutofillDialogContentView mContentView;
private?

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:34:
private int mCurrentLayout = LAYOUT_STEADY;
I would probably move this logic to the content view as it is the one more
impacted by the layout.  Maybe the constants as well.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:40:
accountNames.add("placeholder@gmail.com");
where are these account names going to come from in the long run?

I would extract all placeholders into the places you'll eventually replace with
valid functionality.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:46:
mContentView = (AutofillDialogContentView)getLayoutInflater().
space after cast

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:50:
setButton(AlertDialog.BUTTON_NEGATIVE,
I would add some blank lines in this constructor to split of the logical blocks
a bit more

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:80:
mCurrentLayout != LAYOUT_STEADY) return;
braces needed as it doesn't fit all on one line

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:83:
LAYOUT_EDITING_BILLING;
+4 indent

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:90:
public void onNothingSelected(AdapterView<?> arg0) {
arg0 isn't the best name :-)

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java
(right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:38:
private static int PLACEHOLDER_ANIMATION_DISTANCE = 500;
what unit is this distance in?  Px?  Dp?

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:41:
private static final List<CreditCardItem> mConstantBillingItems =
why are these static?

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:55:
if (mConstantShippingItems.size() == 0) {
Does list have isEmpty()?

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:67:
mTopViewGroup = new ArrayList<View>();
these can probably be final since they're not reset.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:78:
mCreditCardSpinner = (Spinner)findViewById(R.id.cc_spinner);
space after case (double check all your casts)

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:81:
cards.add(new CreditCardItem(R.drawable.visa,
again, move the placeholders to a function that can be replaced with working
functionality easily.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:100:
mCreditCardSpinner.setDropDownVerticalOffset(-1 *
mCreditCardSpinner.getMeasuredHeight());
you can just do:

-mCreditCardSpinner.getMeasuredHeight()

same below

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:129:
public void changeLayoutTo(int mode) {
this function is too long, look into splitting it up.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:130:
mEditBillingLayout.setVisibility(GONE);
if you're already in the current mode, should you exit early?

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:172:
int i;
I would declare i in each of the loops to prevent value leakage unintentionally

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:191:
postDelayed(new Runnable() {
should you be caching this runnable in case you need to remove it before it
executes?  (i.e. if you change layouts quickly).

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:199:
private void setViewGroups() {
I would move this up near the constructor.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:215:
String mName;
private final

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:217:
public AddressItem(String name, String address) {
blank line above this

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:236:
ViewGroup parent) {
java indenting instead of c++

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:241:
if(convertView == null)
space after if, braces needed, and java indenting

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:247:
if (nameView != null) nameView.setText(getItem(position).mName);
I would cache getItem(position) before this.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:253:
private static class CreditCardItem {
Do these inner classes map to native counterparts?  might be worth documenting
that so we can figure out what the values relate to.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:254:
int mCardIconID;
private final here too

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:257:
public CreditCardItem(int icon, String lastDigits, String info) {
blank line above it.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:264:
private static class CreditCardAdapter extends ArrayAdapter<CreditCardItem> {
same comments as the above in regards to style.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogTitleView.java
(right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogTitleView.java:34:
if (mConstantItems.size() == 0) {
isEmpty?

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogTitleView.java:47:
public AutofillDialogTitleView(Context context, List<String> accountNames) {
javadoc

[Yusuf, 2013-03-06 19:50:00]

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File chrome/android/java/res/layout/autofill_address_spinner_dropdown.xml
(right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_address_spinner_dropdown.xml:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/03/06 01:41:56, Ted C wrote:
> Hmm...does this work?
> 
> I thought we needed to do the XML comment style:
> <!-- Copyright (c) 2013 The Chromium Authors. All rights reserved.
> 
>    Use of this source code is governed by a BSD-style license that can be
>    found in the LICENSE file.
> -->
> 
> Might want to do it that way just to be consistent.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_address_spinner_dropdown.xml:4: <?xml
version="1.0" encoding="utf-8"?>
On 2013/03/06 01:41:56, Ted C wrote:
> Also, this has gone above the copyright in all other files, so we should do it
> here too.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File chrome/android/java/res/layout/autofill_dialog_content.xml (right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:13: <TextView
On 2013/03/06 01:41:56, Ted C wrote:
> All of the children of this gridlayout should be intended.  Same for below.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:74:
android:layout_columnSpan="4"/>
Adding this as a TODO.

On 2013/03/06 01:41:56, Ted C wrote:
> You'll also want to see how this behaves in accessibility mode as you might
need
> description strings for all these.  Maybe just a TODO for now.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:80:
android:hint="MM"/>
They are going to come from native. Removing them, then



On 2013/03/06 01:41:56, Ted C wrote:
> these should probably be in strings.xml as well
> 
> applies to all the hints.  Unless these come from native in which case you
> should remove them.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:146: <GridLayout
On 2013/03/06 01:41:56, Ted C wrote:
> I would pull these three grid layouts into separate layout files if possible.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:191: </GridLayout>
On 2013/03/06 01:41:56, Ted C wrote:
> indent messed up.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_content.xml:191: </GridLayout>
On 2013/03/06 01:41:56, Ted C wrote:
> indent messed up.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File chrome/android/java/res/layout/autofill_dialog_title.xml (right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_title.xml:6: <FrameLayout
On 2013/03/06 01:41:56, Ted C wrote:
> this block should be indented 4 and it's 2 here and 3 for the closing tag

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/res/layout/autofill_dialog_title.xml:22: </merge>
On 2013/03/06 01:41:56, Ted C wrote:
> decrease indent.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java
(right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:27:
public class AutofillDialog extends AlertDialog implements OnClickListener,
On 2013/03/06 01:41:56, Ted C wrote:
> I would put all the implements on the second line to make it easier to read.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:28:
OnItemSelectedListener {
On 2013/03/06 01:41:56, Ted C wrote:
> +4 indent

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:33:
AutofillDialogContentView mContentView;
On 2013/03/06 01:41:56, Ted C wrote:
> private?

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:34:
private int mCurrentLayout = LAYOUT_STEADY;
On 2013/03/06 01:41:56, Ted C wrote:
> I would probably move this logic to the content view as it is the one more
> impacted by the layout.  Maybe the constants as well.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:40:
accountNames.add("placeholder@gmail.com");
On 2013/03/06 01:41:56, Ted C wrote:
> where are these account names going to come from in the long run?
> 
> I would extract all placeholders into the places you'll eventually replace
with
> valid functionality.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:46:
mContentView = (AutofillDialogContentView)getLayoutInflater().
On 2013/03/06 01:41:56, Ted C wrote:
> space after cast

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:50:
setButton(AlertDialog.BUTTON_NEGATIVE,
On 2013/03/06 01:41:56, Ted C wrote:
> I would add some blank lines in this constructor to split of the logical
blocks
> a bit more

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:80:
mCurrentLayout != LAYOUT_STEADY) return;
On 2013/03/06 01:41:56, Ted C wrote:
> braces needed as it doesn't fit all on one line

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:83:
LAYOUT_EDITING_BILLING;
On 2013/03/06 01:41:56, Ted C wrote:
> +4 indent

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:90:
public void onNothingSelected(AdapterView<?> arg0) {
On 2013/03/06 01:41:56, Ted C wrote:
> arg0 isn't the best name :-)

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java
(right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:38:
private static int PLACEHOLDER_ANIMATION_DISTANCE = 500;
On 2013/03/06 01:41:56, Ted C wrote:
> what unit is this distance in?  Px?  Dp?

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:41:
private static final List<CreditCardItem> mConstantBillingItems =
On 2013/03/06 01:41:56, Ted C wrote:
> why are these static?

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:55:
if (mConstantShippingItems.size() == 0) {
On 2013/03/06 01:41:56, Ted C wrote:
> Does list have isEmpty()?

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:67:
mTopViewGroup = new ArrayList<View>();
On 2013/03/06 01:41:56, Ted C wrote:
> these can probably be final since they're not reset.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:78:
mCreditCardSpinner = (Spinner)findViewById(R.id.cc_spinner);
On 2013/03/06 01:41:56, Ted C wrote:
> space after case (double check all your casts) 

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:81:
cards.add(new CreditCardItem(R.drawable.visa,
On 2013/03/06 01:41:56, Ted C wrote:
> again, move the placeholders to a function that can be replaced with working
> functionality easily.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:100:
mCreditCardSpinner.setDropDownVerticalOffset(-1 *
mCreditCardSpinner.getMeasuredHeight());
On 2013/03/06 01:41:56, Ted C wrote:
> you can just do:
> 
> -mCreditCardSpinner.getMeasuredHeight()
> 
> same below

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:130:
mEditBillingLayout.setVisibility(GONE);
On 2013/03/06 01:41:56, Ted C wrote:
> if you're already in the current mode, should you exit early?

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:172:
int i;
On 2013/03/06 01:41:56, Ted C wrote:
> I would declare i in each of the loops to prevent value leakage
unintentionally

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:191:
postDelayed(new Runnable() {
On 2013/03/06 01:41:56, Ted C wrote:
> should you be caching this runnable in case you need to remove it before it
> executes?  (i.e. if you change layouts quickly).

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:199:
private void setViewGroups() {
On 2013/03/06 01:41:56, Ted C wrote:
> I would move this up near the constructor.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:215:
String mName;
On 2013/03/06 01:41:56, Ted C wrote:
> private final

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:217:
public AddressItem(String name, String address) {
On 2013/03/06 01:41:56, Ted C wrote:
> blank line above this

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:236:
ViewGroup parent) {
On 2013/03/06 01:41:56, Ted C wrote:
> java indenting instead of c++

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:241:
if(convertView == null)
On 2013/03/06 01:41:56, Ted C wrote:
> space after if, braces needed, and java indenting

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:247:
if (nameView != null) nameView.setText(getItem(position).mName);
On 2013/03/06 01:41:56, Ted C wrote:
> I would cache getItem(position) before this.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:253:
private static class CreditCardItem {
On 2013/03/06 01:41:56, Ted C wrote:
> Do these inner classes map to native counterparts?  might be worth documenting
> that so we can figure out what the values relate to.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:254:
int mCardIconID;
No. See the added TODO to createAndAddPlaceholders(). They will be used just for
Android UI, so will be constructed on Java. (Maybe in here or in
AutofillDialog).


On 2013/03/06 01:41:56, Ted C wrote:
> private final here too

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:257:
public CreditCardItem(int icon, String lastDigits, String info) {
On 2013/03/06 01:41:56, Ted C wrote:
> blank line above it.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:264:
private static class CreditCardAdapter extends ArrayAdapter<CreditCardItem> {
On 2013/03/06 01:41:56, Ted C wrote:
> same comments as the above in regards to style.

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogTitleView.java
(right):

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogTitleView.java:34:
if (mConstantItems.size() == 0) {
On 2013/03/06 01:41:56, Ted C wrote:
> isEmpty?

Done.

https://chromiumcodereview.appspot.com/12412003/diff/19001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogTitleView.java:47:
public AutofillDialogTitleView(Context context, List<String> accountNames) {
On 2013/03/06 01:41:56, Ted C wrote:
> javadoc

Done.

[Ted C, 2013-03-06 21:57:11]

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/res/l...
File chrome/android/java/res/layout/autofill_dialog_content.xml (right):

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/res/l...
chrome/android/java/res/layout/autofill_dialog_content.xml:12:
layout="@layout/autofill_general_layout"/>
The indenting is off for this line.  I would probably put the layout attributes
on the same line as the include.

And the indenting looks kind of wonky for the rest of this file.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/res/l...
File chrome/android/java/res/layout/autofill_editing_layout_billing.xml (right):

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/res/l...
chrome/android/java/res/layout/autofill_editing_layout_billing.xml:13:
android:layout_height="48dp"
I would probably extract the 48dp height and 2dp margin and textsize of 18sp
into a dimens file so it can be shared/updated easily.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java
(right):

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:101:
private void updateAccountChooserAndAddTitle(List<String> accounts) {
if this will eventually be what is called, should you make it public/protected
w/ javadoc?  Or do you think the interface will change so that won't be worth
it?

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java
(right):

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:39:
private static int PLACEHOLDER_ANIMATION_DISTANCE_PX = 500;
should this be in DP?  or is PX what you want for all densities?

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:42:
private final List<CreditCardItem> mConstantBillingItems =
instead of Constant, maybe Default?

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:193:
View centeredLayout = mode == LAYOUT_EDITING_BILLING ? mEditBillingLayout :
personal style nit -- I would put ? mEditBillingLayout on the second line so you
can see both conditions next to each other (assuming they both fit on the same
line).

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:194:
mEditShippingLayout;
should be indented 8.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:207:
mAddressSpinner;
same comment as above.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:284:
if(convertView == null) {
space between if and (

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:326:
if(convertView == null) {
missing space here too

[Yusuf, 2013-03-07 18:16:58]

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/res/l...
File chrome/android/java/res/layout/autofill_dialog_content.xml (right):

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/res/l...
chrome/android/java/res/layout/autofill_dialog_content.xml:12:
layout="@layout/autofill_general_layout"/>
On 2013/03/06 21:57:11, Ted C wrote:
> The indenting is off for this line.  I would probably put the layout
attributes
> on the same line as the include.
> 
> And the indenting looks kind of wonky for the rest of this file.

Done.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/res/l...
File chrome/android/java/res/layout/autofill_editing_layout_billing.xml (right):

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/res/l...
chrome/android/java/res/layout/autofill_editing_layout_billing.xml:13:
android:layout_height="48dp"
On 2013/03/06 21:57:11, Ted C wrote:
> I would probably extract the 48dp height and 2dp margin and textsize of 18sp
> into a dimens file so it can be shared/updated easily.

Done.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java
(right):

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:101:
private void updateAccountChooserAndAddTitle(List<String> accounts) {
On 2013/03/06 21:57:11, Ted C wrote:
> if this will eventually be what is called, should you make it public/protected
> w/ javadoc?  Or do you think the interface will change so that won't be worth
> it?

Done.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java
(right):

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:39:
private static int PLACEHOLDER_ANIMATION_DISTANCE_PX = 500;
I think it will end up having a percentage value, but since I am not even sure
we will have the translation I am adding it as dp for now.


On 2013/03/06 21:57:11, Ted C wrote:
> should this be in DP?  or is PX what you want for all densities?

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:42:
private final List<CreditCardItem> mConstantBillingItems =
On 2013/03/06 21:57:11, Ted C wrote:
> instead of Constant, maybe Default?

Done.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:193:
View centeredLayout = mode == LAYOUT_EDITING_BILLING ? mEditBillingLayout :
On 2013/03/06 21:57:11, Ted C wrote:
> personal style nit -- I would put ? mEditBillingLayout on the second line so
you
> can see both conditions next to each other (assuming they both fit on the same
> line).

Done.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:194:
mEditShippingLayout;
On 2013/03/06 21:57:11, Ted C wrote:
> should be indented 8.

Done.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:207:
mAddressSpinner;
On 2013/03/06 21:57:11, Ted C wrote:
> same comment as above.

Done.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:284:
if(convertView == null) {
On 2013/03/06 21:57:11, Ted C wrote:
> space between if and (

Done.

https://codereview.chromium.org/12412003/diff/33001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:326:
if(convertView == null) {
On 2013/03/06 21:57:11, Ted C wrote:
> missing space here too

Done.

[Ted C, 2013-03-07 18:28:09]

lgtm w/ a few final comments

https://codereview.chromium.org/12412003/diff/44001/chrome/android/java/res/l...
File chrome/android/java/res/layout/autofill_dialog_content.xml (right):

https://codereview.chromium.org/12412003/diff/44001/chrome/android/java/res/l...
chrome/android/java/res/layout/autofill_dialog_content.xml:23:
android:layout_width="48dp"
is this meant to match @dimen/autofill_edit_height?  Or is it just coincidence? 
Just wondering if you should use the constant or not.

https://codereview.chromium.org/12412003/diff/44001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java
(right):

https://codereview.chromium.org/12412003/diff/44001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:70:
PLACEHOLDER_ANIMATION_DISTANCE_DP = getResources().
if you're loading this from resources, I think it's better off to be a local
variable instead of a constant (mPlaceholderAnimationDistanceDp)

https://codereview.chromium.org/12412003/diff/44001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:73:
if (mDefaultShippingItems.isEmpty()) {
now that these aren't statics, you probably don't need these isEmpty() checks as
they'll always be empty at construction time.

[Yusuf, 2013-03-07 18:53:20]

https://codereview.chromium.org/12412003/diff/44001/chrome/android/java/res/l...
File chrome/android/java/res/layout/autofill_dialog_content.xml (right):

https://codereview.chromium.org/12412003/diff/44001/chrome/android/java/res/l...
chrome/android/java/res/layout/autofill_dialog_content.xml:23:
android:layout_width="48dp"
It is a coincidence. I think we will end up getting a different spec for this.

On 2013/03/07 18:28:09, Ted C wrote:
> is this meant to match @dimen/autofill_edit_height?  Or is it just
coincidence? 
> Just wondering if you should use the constant or not.

https://codereview.chromium.org/12412003/diff/44001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java
(right):

https://codereview.chromium.org/12412003/diff/44001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:70:
PLACEHOLDER_ANIMATION_DISTANCE_DP = getResources().
On 2013/03/07 18:28:09, Ted C wrote:
> if you're loading this from resources, I think it's better off to be a local
> variable instead of a constant (mPlaceholderAnimationDistanceDp)

Done.

https://codereview.chromium.org/12412003/diff/44001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java:73:
if (mDefaultShippingItems.isEmpty()) {
On 2013/03/07 18:28:09, Ted C wrote:
> now that these aren't statics, you probably don't need these isEmpty() checks
as
> they'll always be empty at construction time.

Done.

[Yusuf, 2013-03-08 23:02:20]

Change is Android only and it has passed Android tests, turning NOTRY=true

[commit-bot: I haz the power, 2013-03-08 23:02:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusufo@chromium.org/12412003/74001

[commit-bot: I haz the power, 2013-03-08 23:11:37]

Change committed as 187071