[Autofill] Uses uniform margin for icon, label and sublabel in popup

Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout for credit card after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymMGxWUkI5aThVNFU/view?usp=sharing

New layout for user name and address that uses same margin as for expanded credit card popup layout.
https://screenshot.googleplex.com/HoJLy2bT2BO.png
BUG=

Review-Url: https://codereview.chromium.org/2649623002
Cr-Commit-Position: refs/heads/master@{#446743}
Committed: https://chromium.googlesource.com/chromium/src/+/8029467edee0bb882544d16c53c1efbd96e1de67

[csashi, 2017-01-20 19:53:22]

Description was changed from

==========
Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymS0lrRUx2eWVqSW8/view?usp=sharing
BUG=
==========

to

==========
Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymS0lrRUx2eWVqSW8/view?usp=sharing
BUG=
==========

[csashi, 2017-01-20 19:53:22]

csashi@google.com changed reviewers:
+ dtrainor@chromium.org

[csashi, 2017-01-20 19:53:39]

On 2017/01/20 19:53:22, csashi wrote:
> mailto:csashi@google.com changed reviewers:
> + mailto:dtrainor@chromium.org

Hi David,
Can you take a look? Thanks,
-sashi.

[csashi, 2017-01-20 19:59:24]

Description was changed from

==========
Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymS0lrRUx2eWVqSW8/view?usp=sharing
BUG=
==========

to

==========
Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymbUM0OWwzaFlwLU0/view?usp=sharing
BUG=
==========

[csashi, 2017-01-20 21:06:30]

Description was changed from

==========
Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymbUM0OWwzaFlwLU0/view?usp=sharing
BUG=
==========

to

==========
Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymMGxWUkI5aThVNFU/view?usp=sharing
BUG=
==========

[csashi, 2017-01-24 17:22:19]

On 2017/01/20 19:53:39, csashi wrote:
> On 2017/01/20 19:53:22, csashi wrote:
> > mailto:csashi@google.com changed reviewers:
> > + mailto:dtrainor@chromium.org
> 
> Hi David,
> Can you take a look? Thanks,
> -sashi.

Hi David,
Can you please take a look or reassign to someone else if you don't have
bandwidth.
Thanks,
-sashi.

[csashi, 2017-01-25 03:53:44]

csashi@google.com changed reviewers:
+ tedchoc@chromium.org
- dtrainor@chromium.org

[csashi, 2017-01-25 03:53:58]

On 2017/01/24 17:22:19, csashi wrote:
> On 2017/01/20 19:53:39, csashi wrote:
> > On 2017/01/20 19:53:22, csashi wrote:
> > > mailto:csashi@google.com changed reviewers:
> > > + mailto:dtrainor@chromium.org
> > 
> > Hi David,
> > Can you take a look? Thanks,
> > -sashi.
> 
> Hi David,
> Can you please take a look or reassign to someone else if you don't have
> bandwidth.
> Thanks,
> -sashi.

Hi Ted,
Can you please take a look? Thanks,
-sashi.

[David Trainor- moved to gerrit, 2017-01-25 04:00:14]

dtrainor@chromium.org changed reviewers:
+ dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-01-25 04:00:15]

Sorry for the delay!  Thanks for the ping :).

https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:149:
ApiCompatibilityUtils.setMarginStart(layoutParams, labelMargin + iconMargin -
1);
1 dp or 1 px?  This looks like 1 px right?

https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:150:
ApiCompatibilityUtils.setMarginEnd(layoutParams, labelMargin + iconMargin);
Should the end margin always be labelMargin even if there is no icon?  That way
the margin is consistent on the tail end of the drawable.  LMK if I'm picturing
this incorrectly though!

[csashi, 2017-01-25 08:16:27]

https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:149:
ApiCompatibilityUtils.setMarginStart(layoutParams, labelMargin + iconMargin -
1);
On 2017/01/25 04:00:15, David Trainor wrote:
> 1 dp or 1 px?  This looks like 1 px right?

Done.

https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:150:
ApiCompatibilityUtils.setMarginEnd(layoutParams, labelMargin + iconMargin);
On 2017/01/25 04:00:15, David Trainor wrote:
> Should the end margin always be labelMargin even if there is no icon?  That
way
> the margin is consistent on the tail end of the drawable.  LMK if I'm
picturing
> this incorrectly though!

The UX reviewer wanted the larger margin at the end of the label - i.e. he
wanted to change the green distances at the end of "Chrome Settings ..." equal
to the orange distance in
this picture:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

[csashi, 2017-01-25 08:49:25]

On 2017/01/25 08:16:27, csashi wrote:
>
https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
> File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):
> 
>
https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
> ui/android/java/src/org/chromium/ui/DropdownAdapter.java:149:
> ApiCompatibilityUtils.setMarginStart(layoutParams, labelMargin + iconMargin -
> 1);
> On 2017/01/25 04:00:15, David Trainor wrote:
> > 1 dp or 1 px?  This looks like 1 px right?
> 
> Done.
> 
>
https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
> ui/android/java/src/org/chromium/ui/DropdownAdapter.java:150:
> ApiCompatibilityUtils.setMarginEnd(layoutParams, labelMargin + iconMargin);
> On 2017/01/25 04:00:15, David Trainor wrote:
> > Should the end margin always be labelMargin even if there is no icon?  That
> way
> > the margin is consistent on the tail end of the drawable.  LMK if I'm
> picturing
> > this incorrectly though!
> 
> The UX reviewer wanted the larger margin at the end of the label - i.e. he
> wanted to change the green distances at the end of "Chrome Settings ..." equal
> to the orange distance in
> this picture:
> https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

Hi David,
Thanks for getting back. I added additional comments. Please take a look.
-sashi.

[csashi, 2017-01-25 08:51:03]

https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:150:
ApiCompatibilityUtils.setMarginEnd(layoutParams, labelMargin + iconMargin);
On 2017/01/25 08:16:27, csashi wrote:
> On 2017/01/25 04:00:15, David Trainor wrote:
> > Should the end margin always be labelMargin even if there is no icon?  That
> way
> > the margin is consistent on the tail end of the drawable.  LMK if I'm
> picturing
> > this incorrectly though!
> 
> The UX reviewer wanted the larger margin at the end of the label - i.e. he
> wanted to change the green distances at the end of "Chrome Settings ..." equal
> to the orange distance in
> this picture:
> https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

I fixed the endMargin for the label when there is an icon. I didn't catch it in
my testing because the label text did not exceed "Chrome Autofill Settings...".

[csashi, 2017-01-25 08:52:42]

On 2017/01/25 08:51:03, csashi wrote:
>
https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
> File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):
> 
>
https://codereview.chromium.org/2649623002/diff/20001/ui/android/java/src/org...
> ui/android/java/src/org/chromium/ui/DropdownAdapter.java:150:
> ApiCompatibilityUtils.setMarginEnd(layoutParams, labelMargin + iconMargin);
> On 2017/01/25 08:16:27, csashi wrote:
> > On 2017/01/25 04:00:15, David Trainor wrote:
> > > Should the end margin always be labelMargin even if there is no icon? 
That
> > way
> > > the margin is consistent on the tail end of the drawable.  LMK if I'm
> > picturing
> > > this incorrectly though!
> > 
> > The UX reviewer wanted the larger margin at the end of the label - i.e. he
> > wanted to change the green distances at the end of "Chrome Settings ..."
equal
> > to the orange distance in
> > this picture:
> >
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing
> 
> I fixed the endMargin for the label when there is an icon. I didn't catch it
in
> my testing because the label text did not exceed "Chrome Autofill
Settings...".

The code will become much simpler if we either make this the default (i.e. not
experiment) or 
remove the experiment. It is this way now because I didn't want to add
additional experiment parameters
that I would have to plumb through from the native autofill code to this
function.

[Ted C, 2017-01-25 17:50:23]

https://codereview.chromium.org/2649623002/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:146: if
(item.isIconAtStart()) {
IMO, I think this should be whether any item in the dropdown has an icon and
isIconAtStart

This avoid having to special case autofill/pass in new variables, but wouldn't
affect things where icons aren't present.

for (int i = 0; i < getCount(); i++) {
    DropdownItem item = getItem(i);
    if (item.isIconAtStart() && item.getIconId() != DropdownItem.NO_ICON) {
        return true;
    }
}
return false;

We might eventually change the logic of DropdownItemBase and this would insulate
this change from that.

https://codereview.chromium.org/2649623002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:151: labelMargin +
iconMargin - 1);
this 1px "smells" to me like it is going to be very device/font/android version
specific.

Is it because the font glyph has an inherent 1px offset from the beginning?  I
feel this is likely a rounding artifact that will not be consistent.  Maybe test
on a few different devices/locales to makes sure this is consistent?

It could be that it also just "feels" nicer and gives a better perceived visual
look, which could be explained by a more verbose comment.

[csashi, 2017-01-25 18:59:04]

Hi Ted, David,
Can you please take a look? Thanks,
-sashi.

https://codereview.chromium.org/2649623002/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:146: if
(item.isIconAtStart()) {
On 2017/01/25 17:50:23, Ted C wrote:
> IMO, I think this should be whether any item in the dropdown has an icon and
> isIconAtStart
> 
> This avoid having to special case autofill/pass in new variables, but wouldn't
> affect things where icons aren't present.
> 
> for (int i = 0; i < getCount(); i++) {
>     DropdownItem item = getItem(i);
>     if (item.isIconAtStart() && item.getIconId() != DropdownItem.NO_ICON) {
>         return true;
>     }
> }
> return false;
> 
> We might eventually change the logic of DropdownItemBase and this would
insulate
> this change from that.

Done.

https://codereview.chromium.org/2649623002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:151: labelMargin +
iconMargin - 1);
On 2017/01/25 17:50:23, Ted C wrote:
> this 1px "smells" to me like it is going to be very device/font/android
version
> specific.
> 
> Is it because the font glyph has an inherent 1px offset from the beginning?  I
> feel this is likely a rounding artifact that will not be consistent.  Maybe
test
> on a few different devices/locales to makes sure this is consistent?
> 
> It could be that it also just "feels" nicer and gives a better perceived
visual
> look, which could be explained by a more verbose comment.

I agree and I am not sure why I needed to do that. Without the '-1', the label
is slightly off as seen in this picture:

https://drive.google.com/file/d/0B1qGSlvsjCymQlJ1RVdUbkcyODA/view?usp=sharing

[csashi, 2017-01-27 00:21:42]

Description was changed from

==========
Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymMGxWUkI5aThVNFU/view?usp=sharing
BUG=
==========

to

==========
Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout for credit card after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymMGxWUkI5aThVNFU/view?usp=sharing

New layout for user name and address that uses same margin as for expanded
credit card popup layout.
https://screenshot.googleplex.com/HoJLy2bT2BO.png
BUG=
==========

[csashi, 2017-01-27 00:21:43]

csashi@google.com changed reviewers:
+ mathp@chromium.org, sgurun@chromium.org

[csashi, 2017-01-27 00:25:48]

On 2017/01/25 18:59:04, csashi wrote:
> Hi Ted, David,
> Can you please take a look? Thanks,
> -sashi.
> 
>
https://codereview.chromium.org/2649623002/diff/60001/ui/android/java/src/org...
> File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):
> 
>
https://codereview.chromium.org/2649623002/diff/60001/ui/android/java/src/org...
> ui/android/java/src/org/chromium/ui/DropdownAdapter.java:146: if
> (item.isIconAtStart()) {
> On 2017/01/25 17:50:23, Ted C wrote:
> > IMO, I think this should be whether any item in the dropdown has an icon and
> > isIconAtStart
> > 
> > This avoid having to special case autofill/pass in new variables, but
wouldn't
> > affect things where icons aren't present.
> > 
> > for (int i = 0; i < getCount(); i++) {
> >     DropdownItem item = getItem(i);
> >     if (item.isIconAtStart() && item.getIconId() != DropdownItem.NO_ICON) {
> >         return true;
> >     }
> > }
> > return false;
> > 
> > We might eventually change the logic of DropdownItemBase and this would
> insulate
> > this change from that.
> 
> Done.
> 
>
https://codereview.chromium.org/2649623002/diff/60001/ui/android/java/src/org...
> ui/android/java/src/org/chromium/ui/DropdownAdapter.java:151: labelMargin +
> iconMargin - 1);
> On 2017/01/25 17:50:23, Ted C wrote:
> > this 1px "smells" to me like it is going to be very device/font/android
> version
> > specific.
> > 
> > Is it because the font glyph has an inherent 1px offset from the beginning? 
I
> > feel this is likely a rounding artifact that will not be consistent.  Maybe
> test
> > on a few different devices/locales to makes sure this is consistent?
> > 
> > It could be that it also just "feels" nicer and gives a better perceived
> visual
> > look, which could be explained by a more verbose comment.
> 
> I agree and I am not sure why I needed to do that. Without the '-1', the label
> is slightly off as seen in this picture:
> 
> https://drive.google.com/file/d/0B1qGSlvsjCymQlJ1RVdUbkcyODA/view?usp=sharing

Hi Ted, David
I removed the -1 px needed for the font glyph. I also made the margin an
explicit experiment parameter and
we consistently use the same margin for credit card, user name and address auto
fill popup. Please see updated
description for screenshots.

Hi Mathieu,
Can you review chrome/browser/ui/autofill/ and components/autofill/core/browser/
(and, of course, DropdownAdapter if you wish).

Hi Selim,
Can you approve for android_webview?

Thanks,
-sashi.

[Mathieu, 2017-01-27 00:40:44]

https://codereview.chromium.org/2649623002/diff/100001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillPopup.java
(right):

https://codereview.chromium.org/2649623002/diff/100001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillPopup.java:71:
* pixel units, 0 if not specified in experiment.
nit: remove "in experiment"

https://codereview.chromium.org/2649623002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_experiments.h (right):

https://codereview.chromium.org/2649623002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_experiments.h:92: unsigned int
GetPopupMargin();
nit: newline after this

https://codereview.chromium.org/2649623002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_experiments.h:92: unsigned int
GetPopupMargin();
if specific to credit card, I would rename the method to indicate so

https://codereview.chromium.org/2649623002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:57: Integer
dropdownItemHeight, Integer margin) {
@Nullable?

https://codereview.chromium.org/2649623002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:152: if (margin == 0) {
check for null?

https://codereview.chromium.org/2649623002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:216: int iconMargin =
margin == 0
same?

[csashi, 2017-01-27 01:21:32]

https://codereview.chromium.org/2649623002/diff/100001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillPopup.java
(right):

https://codereview.chromium.org/2649623002/diff/100001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillPopup.java:71:
* pixel units, 0 if not specified in experiment.
On 2017/01/27 00:40:43, Mathieu Perreault wrote:
> nit: remove "in experiment"

Done.

https://codereview.chromium.org/2649623002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_experiments.h (right):

https://codereview.chromium.org/2649623002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_experiments.h:92: unsigned int
GetPopupMargin();
On 2017/01/27 00:40:43, Mathieu Perreault wrote:
> if specific to credit card, I would rename the method to indicate so

Not specific to credit card.

https://codereview.chromium.org/2649623002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_experiments.h:92: unsigned int
GetPopupMargin();
On 2017/01/27 00:40:44, Mathieu Perreault wrote:
> nit: newline after this

Done.

https://codereview.chromium.org/2649623002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:57: Integer
dropdownItemHeight, Integer margin) {
On 2017/01/27 00:40:44, Mathieu Perreault wrote:
> @Nullable?

Done.

https://codereview.chromium.org/2649623002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:152: if (margin == 0) {
On 2017/01/27 00:40:44, Mathieu Perreault wrote:
> check for null?

Done.

https://codereview.chromium.org/2649623002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:216: int iconMargin =
margin == 0
On 2017/01/27 00:40:44, Mathieu Perreault wrote:
> same?

Done.

[Mathieu, 2017-01-27 01:31:06]

lgtm

https://codereview.chromium.org/2649623002/diff/120001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/120001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:155: if (mMargin ==
null) {
Actually I may have misled, since |margin| is set to 0 on line 145, maybe it's
best to use that.

Sorry!

[csashi, 2017-01-27 01:35:36]

https://codereview.chromium.org/2649623002/diff/120001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/120001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:155: if (mMargin ==
null) {
On 2017/01/27 01:31:06, Mathieu Perreault wrote:
> Actually I may have misled, since |margin| is set to 0 on line 145, maybe it's
> best to use that.
> 
> Sorry!

Done.

[Ted C, 2017-01-27 04:31:52]

https://codereview.chromium.org/2649623002/diff/160001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:144: int margin =
mMargin == null ? 0 : (int) TypedValue.applyDimension(
I wonder if we should precalculate margin in the constructor to make this code
more consistent.

maybe

mLabelHorzMargin = <...>;
mLabelVertMargin = <...>;
mImageHorzMargin = <...>;

I guess in general, do we expect the margins to be consistent for all items in
the adapter or to change per item type?  if they don't change, the constructor
seems nicer as we reduce the codepaths here (or even if we need to do it per
item, maybe even storing in a local var here would be better).

https://codereview.chromium.org/2649623002/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:160: }
if this is else, do you need to set the margin to 0?

https://codereview.chromium.org/2649623002/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:218: int iconMargin =
margin == 0
Hmm...this seems to be a change in behavior.  Should this be margin != 0 ?
margin : mContext.getResources(...);

margin == 0 is the default case before your change right?

[csashi, 2017-01-27 18:12:55]

https://codereview.chromium.org/2649623002/diff/160001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:144: int margin =
mMargin == null ? 0 : (int) TypedValue.applyDimension(
On 2017/01/27 04:31:52, Ted C wrote:
> I wonder if we should precalculate margin in the constructor to make this code
> more consistent.
> 
> maybe
> 
> mLabelHorzMargin = <...>;
> mLabelVertMargin = <...>;
> mImageHorzMargin = <...>;
> 
> I guess in general, do we expect the margins to be consistent for all items in
> the adapter or to change per item type?  if they don't change, the constructor
> seems nicer as we reduce the codepaths here (or even if we need to do it per
> item, maybe even storing in a local var here would be better).

Done for label. Icon uses different margins depending on the icon - see
getIconMarginResId. I wanted to preserve that behavior when we are not in the
experiment.

https://codereview.chromium.org/2649623002/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:160: }
On 2017/01/27 04:31:52, Ted C wrote:
> if this is else, do you need to set the margin to 0?

I didn't want to add "ApiCompatibilityUtils.setMarginStart(layoutParams,  0)"
because I expected default Android behavior will be margin start = 0. And, it
worked as expected so didn't want to add an unnecessary line.

https://codereview.chromium.org/2649623002/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:218: int iconMargin =
margin == 0
On 2017/01/27 04:31:52, Ted C wrote:
> Hmm...this seems to be a change in behavior.  Should this be margin != 0 ?
> margin : mContext.getResources(...);
> 
> margin == 0 is the default case before your change right?

Correct. margin == 0 is the default case. But your code looks the same to me
(except the condition is margin != 0).

[sgurun-gerrit only, 2017-01-27 18:21:35]

aw lgtm

[Ted C, 2017-01-27 18:41:11]

lgtm

https://codereview.chromium.org/2649623002/diff/160001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2649623002/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:160: }
On 2017/01/27 18:12:55, csashi wrote:
> On 2017/01/27 04:31:52, Ted C wrote:
> > if this is else, do you need to set the margin to 0?
> 
> I didn't want to add "ApiCompatibilityUtils.setMarginStart(layoutParams,  0)"
> because I expected default Android behavior will be margin start = 0. And, it
> worked as expected so didn't want to add an unnecessary line.

Ah...we are constructing new layoutparams each time.  I was thinking that we
were reusing the existing one and then recylcing would do weird things...indeed
not needed.

[csashi, 2017-01-27 18:43:14]

The CQ bit was checked by csashi@google.com

[csashi, 2017-01-27 18:43:15]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2649623002/#ps180001
(title: "Moves label margin calculation to constructor.")

[commit-bot: I haz the power, 2017-01-27 18:43:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 20:10:45]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485542594981750,
"parent_rev": "72d0e68daf64b2c1a18fedb4be520c2fa183c734", "commit_rev":
"8029467edee0bb882544d16c53c1efbd96e1de67"}

[commit-bot: I haz the power, 2017-01-27 20:11:18]

Description was changed from

==========
Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout for credit card after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymMGxWUkI5aThVNFU/view?usp=sharing

New layout for user name and address that uses same margin as for expanded
credit card popup layout.
https://screenshot.googleplex.com/HoJLy2bT2BO.png
BUG=
==========

to

==========
Uses uniform margin for icon, label and sublabel in order to
make all of the green distances here equal to the orange distance in
the picture below:
https://drive.google.com/file/d/0B1qGSlvsjCymNVE1eGJVWW40TWM/view?usp=sharing

New layout for credit card after this change:
https://drive.google.com/file/d/0B1qGSlvsjCymMGxWUkI5aThVNFU/view?usp=sharing

New layout for user name and address that uses same margin as for expanded
credit card popup layout.
https://screenshot.googleplex.com/HoJLy2bT2BO.png
BUG=

Review-Url: https://codereview.chromium.org/2649623002
Cr-Commit-Position: refs/heads/master@{#446743}
Committed:
https://chromium.googlesource.com/chromium/src/+/8029467edee0bb882544d16c53c1...
==========

[commit-bot: I haz the power, 2017-01-27 20:11:19]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/8029467edee0bb882544d16c53c1...