Expanded Autofill Credit Card Popup Layout Experiment in Android.

ui/android/java/src/org/chromium/ui/DropdownAdapter.java

Index: ui/android/java/src/org/chromium/ui/DropdownAdapter.java
diff --git a/ui/android/java/src/org/chromium/ui/DropdownAdapter.java b/ui/android/java/src/org/chromium/ui/DropdownAdapter.java
index d8ad3f3fdeed237e8c817613c839f37363e7c0c5..e41340d26540be1bf1afed46fde81546b98c30f5 100644
--- a/ui/android/java/src/org/chromium/ui/DropdownAdapter.java
+++ b/ui/android/java/src/org/chromium/ui/DropdownAdapter.java
@@ -30,14 +30,24 @@ public class DropdownAdapter extends ArrayAdapter<DropdownItem> {
     private final Context mContext;
     private final Set<Integer> mSeparators;
     private final boolean mAreAllItemsEnabled;
-
-    public DropdownAdapter(
-            Context context, List<? extends DropdownItem> items, Set<Integer> separators) {
+    // Keep comparison with default values (i.e. not overriden by experiment value) consistent with
+    // native code. 0 == SK_ColorTRANSPARENT is default value if not overriden by experiment.

[Theresa, 2016/12/08 18:26:20]

I'm a little confused by this comment. It looks like if these are set to 0, the
values in colors.xml will be used for the divider line and the background will
be transparent?

[csashi, 2016/12/08 20:09:01]

Done.

+    private final int mBackgroundColor;
+    private final int mDividerColor;
+    // 0 => use the default value for |dropdown_item_height| specified in XML resource file.

[Theresa, 2016/12/08 18:26:20]

This comment should go in the JavaDoc for the constructor -- something like: The
height of the drop down items in dp(?). If this is 0, the
@dimens/dropdown_item_height will be used.


Alternatively, instead of using 0 to indicate "use the default" you could
introduce a second constructor. See comment below.

[csashi, 2016/12/08 20:09:01]

Done.

+    private final int mDropdownItemHeight;
+
+    public DropdownAdapter(Context context, List<? extends DropdownItem> items,
+            Set<Integer> separators, int backgroundColor, int dividerColor,
+            int dropdownItemHeight) {
         super(context, R.layout.dropdown_item);
+        mContext = context;
         addAll(items);
         mSeparators = separators;
-        mContext = context;
         mAreAllItemsEnabled = checkAreAllItemsEnabled();
+        mBackgroundColor = backgroundColor;
+        mDividerColor = dividerColor;
+        mDropdownItemHeight = dropdownItemHeight;
     }
 
     private boolean checkAreAllItemsEnabled() {
@@ -57,25 +67,35 @@ public class DropdownAdapter extends ArrayAdapter<DropdownItem> {
             LayoutInflater inflater =
                     (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
             layout = inflater.inflate(R.layout.dropdown_item, null);
-            layout.setBackground(new DropdownDividerDrawable());
+            layout.setBackground(new DropdownDividerDrawable(mBackgroundColor));
         }
-
         DropdownDividerDrawable divider = (DropdownDividerDrawable) layout.getBackground();
-        int height = mContext.getResources().getDimensionPixelSize(R.dimen.dropdown_item_height);
+        int height;
+        if (mDropdownItemHeight == 0) {
+            height = mContext.getResources().getDimensionPixelSize(R.dimen.dropdown_item_height);
+        } else {
+            height = (int) TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP,
+                    mDropdownItemHeight, mContext.getResources().getDisplayMetrics());
+        }
+
         if (position == 0) {
-            divider.setColor(Color.TRANSPARENT);
+            divider.setDividerColor(Color.TRANSPARENT);
         } else {
             int dividerHeight = mContext.getResources().getDimensionPixelSize(
                     R.dimen.dropdown_item_divider_height);
             height += dividerHeight;
             divider.setHeight(dividerHeight);
+            int dividerColor;
             if (mSeparators != null && mSeparators.contains(position)) {
-                divider.setColor(ApiCompatibilityUtils.getColor(mContext.getResources(),
-                        R.color.dropdown_dark_divider_color));
+                dividerColor = ApiCompatibilityUtils.getColor(mContext.getResources(),
+                        R.color.dropdown_dark_divider_color);
+            } else if (mDividerColor == 0) {

[Theresa, 2016/12/08 18:26:20]

Since 0 is a valid color (transparent), would it make sense to use -1 to
indicate that the default dropdown_divider_color should be used? Or (probably
better) there could be two constructors, one that takes a dividerColor and one
that passes R.color.dropdown_divider_color as the dividerColor to the other
constructor.

[csashi, 2016/12/08 20:09:01]

I was concerned -1 could also be interpreted as a color. Also, for consistency,
I wanted to use 0 as the default value for all the experiment parameters.

I considered using different constructors but we would need at least 4
constructors.
1. All defaults (i.e. no experiment)
2. Only specify the height (this is a variation we want to try).
3. Change background color and divider color.
4. Change divider color only.

If I had multiple constructors I would push this logic down to
AutofillPopup.java which would need to decide which constructor to use for
DropdownAdapter. Seemed simpler to have the experiment logic in 1 place. 

I added some clarifying comments. Please let me know if that helps.

[Theresa, 2016/12/08 21:38:17]

Good point. It looks like -1 is the value for Color.WHITE.
Ah, I see. The values are coming from native which makes this a little more
challenging.
Since this is a ui component class, if the background and divider line color are
configurable Color.Transparent should be allowed. Could these param's be
Integer's with null meaning "use the default"? The autofill code could can map a
native value of 0 -> null.

[csashi, 2016/12/08 23:03:29]

Done.

+                dividerColor = ApiCompatibilityUtils.getColor(mContext.getResources(),
+                        R.color.dropdown_divider_color);
             } else {
-                divider.setColor(ApiCompatibilityUtils.getColor(mContext.getResources(),
-                        R.color.dropdown_divider_color));
+                dividerColor = mDividerColor;
             }
+            divider.setDividerColor(dividerColor);
         }
 
         DropdownItem item = getItem(position);
@@ -113,7 +133,7 @@ public class DropdownAdapter extends ArrayAdapter<DropdownItem> {
         }
 
         labelView.setEnabled(item.isEnabled());
-        if (item.isGroupHeader()) {
+        if (item.isGroupHeader() || item.isBoldLabel()) {
             labelView.setTypeface(null, Typeface.BOLD);
         } else {
             labelView.setTypeface(null, Typeface.NORMAL);