Fix styling of Form-Not-Secure warnings on Android

Fix styling of Form-Not-Secure warnings on Android

This CL updates the styling of the Android form-not-secure warning to
match the specs (which are at go/fns-ui-spec).

The changes are:
- Make the "Payment autofilling disabled" a single-line label so that it
  has the same padding as the other autofill suggestions.
- Change the color of "Payment autofilling disabled" to #646464.
- Make both the label and sublabel of the "Payment not secure" item font
  size 18sp, adjust the icon size to 24, and change horizontal spacings
  between elements to 10.

These changes are implemented by allowing AutofillSuggestion to override
the necessary layout values and font sizes for the "Payment autofilling
disabled" and "Payment/Login not secure" dropdown items.

Screenshots are in:
https://bugs.chromium.org/p/chromium/issues/detail?id=678479#c13

BUG=678479
Review-Url: https://codereview.chromium.org/2627153007
Cr-Commit-Position: refs/heads/master@{#444790}
Committed: https://chromium.googlesource.com/chromium/src/+/7723c111843d5e749daaf879c9df8205b17fe737

[estark, 2017-01-13 02:18:01]

Description was changed from

==========
Fix styling of Form-Not-Secure warnings on Android

This CL updates the styling of the Android form-not-secure warning to
match the specs (which are at go/fns-ui-spec).

The changes are:
- Make the "Payment autofilling disabled" a single-line label so that it
  has the same padding as the other autofill suggestions.
- Change the color of "Payment autofilling disabled" to #646464.
- Make both the label and sublabel of the "Payment not secure" item font
  size 18sp, adjust the icon size to 24, and change horizontal spacings
  between elements to 16.

These changes are implemented by allowing AutofillSuggestion to override
the necessary layout values and font sizes for the "Payment autofilling
disabled" and "Payment/Login not secure" dropdown items.

BUG=678479
==========

to

==========
Fix styling of Form-Not-Secure warnings on Android

This CL updates the styling of the Android form-not-secure warning to
match the specs (which are at go/fns-ui-spec).

The changes are:
- Make the "Payment autofilling disabled" a single-line label so that it
  has the same padding as the other autofill suggestions.
- Change the color of "Payment autofilling disabled" to #646464.
- Make both the label and sublabel of the "Payment not secure" item font
  size 18sp, adjust the icon size to 24, and change horizontal spacings
  between elements to 16.

These changes are implemented by allowing AutofillSuggestion to override
the necessary layout values and font sizes for the "Payment autofilling
disabled" and "Payment/Login not secure" dropdown items.

Screenshots are in:
https://bugs.chromium.org/p/chromium/issues/detail?id=678479#c13

BUG=678479
==========

[estark, 2017-01-13 02:25:28]

Patchset #1 (id:1) has been deleted

[estark, 2017-01-13 02:28:36]

estark@chromium.org changed reviewers:
+ mathp@chromium.org, miguelg@chromium.org

[estark, 2017-01-13 02:28:36]

PTAL?
mathp for components/autofill
miguelg for ui/android and chrome/browser/ui

I'm brand new to Android UI so apologies if I've done anything silly. Thanks!

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

https://codereview.chromium.org/2627153007/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:188: int iconSize =
item.getIconSizeResId() == LayoutParams.WRAP_CONTENT
This seems a little clunky -- my goal is for items to be WRAP_CONTENT by
default, but in a way that AutofillSuggestion can override with a size for
specific suggestions. Please let me know if there's a better way.

[estark, 2017-01-13 02:39:19]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 02:39:53]

Dry run: 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-13 03:07:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 03:07:22]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[estark, 2017-01-13 03:11:58]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 03:12:35]

Dry run: 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-13 04:02:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 04:03:01]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-01-13 16:45:10]

On 2017/01/13 02:28:36, estark wrote:
> PTAL?
> mathp for components/autofill
> miguelg for ui/android and chrome/browser/ui
> 
> I'm brand new to Android UI so apologies if I've done anything silly. Thanks!
> 
>
https://codereview.chromium.org/2627153007/diff/20001/ui/android/java/src/org...
> File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):
> 
>
https://codereview.chromium.org/2627153007/diff/20001/ui/android/java/src/org...
> ui/android/java/src/org/chromium/ui/DropdownAdapter.java:188: int iconSize =
> item.getIconSizeResId() == LayoutParams.WRAP_CONTENT
> This seems a little clunky -- my goal is for items to be WRAP_CONTENT by
> default, but in a way that AutofillSuggestion can override with a size for
> specific suggestions. Please let me know if there's a better way.

Hi Emily! I see some comments on the bug by hwi@. Should I wait for review?

[estark, 2017-01-13 16:59:04]

On 2017/01/13 16:45:10, Mathieu Perreault wrote:
> On 2017/01/13 02:28:36, estark wrote:
> > PTAL?
> > mathp for components/autofill
> > miguelg for ui/android and chrome/browser/ui
> > 
> > I'm brand new to Android UI so apologies if I've done anything silly.
Thanks!
> > 
> >
>
https://codereview.chromium.org/2627153007/diff/20001/ui/android/java/src/org...
> > File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):
> > 
> >
>
https://codereview.chromium.org/2627153007/diff/20001/ui/android/java/src/org...
> > ui/android/java/src/org/chromium/ui/DropdownAdapter.java:188: int iconSize =
> > item.getIconSizeResId() == LayoutParams.WRAP_CONTENT
> > This seems a little clunky -- my goal is for items to be WRAP_CONTENT by
> > default, but in a way that AutofillSuggestion can override with a size for
> > specific suggestions. Please let me know if there's a better way.
> 
> Hi Emily! I see some comments on the bug by hwi@. Should I wait for review?

Yeah, probably best to hold off -- I'll ping in a little bit when I implement
those latest changes. I *think* it should just be changing the margin values and
it won't be substantive changes to this CL, but I'll let you know. Thanks!

[estark, 2017-01-13 18:33:17]

Patchset #3 (id:60001) has been deleted

[estark, 2017-01-13 18:33:53]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 18:34:42]

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

[estark, 2017-01-13 18:35:11]

Updated to match
https://bugs.chromium.org/p/chromium/issues/detail?id=678479#c16 (with
screenshots at
https://bugs.chromium.org/p/chromium/issues/detail?id=678479#c17). I changed
margin values to 10dp and removed a sublabel margin override that I no longer
needed.

[commit-bot: I haz the power, 2017-01-13 19:22:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 19:22:43]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-01-14 20:46:40]

lgtm from Autofill side

[estark, 2017-01-17 23:19:09]

Friendly ping to miguelg

[estark, 2017-01-18 23:31:14]

estark@chromium.org changed reviewers:
+ tedchoc@chromium.org
- miguelg@chromium.org

[estark, 2017-01-18 23:31:14]

tedchoc, can you please review chrome/browser/ui/android and ui/android? Thanks!

[Ted C, 2017-01-19 00:35:30]

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

https://codereview.chromium.org/2627153007/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:183: :
mContext.getResources().getDimensionPixelSize(item.getIconSizeResId());
This is a "bit" odd.  When specifying width/height in layout params, you specify
either WRAP_CONTENT, MATCH_PARENT or the size in pixels.  By returning
WRAP_CONTENT or a resource identifier, you are mixing up the two.  While I
"think" this is ok, I don't know if I can 100% guarantee that the namspaces
don't overlap.

I think it would be slightly cleaner to do one of the following:

1.) Instead of returning WRAP_CONTENT, have it return 0 in the default as 0 is
an invalid identifier.
2.) Change the method signature to be getLayoutParamDimension which can return
WRAP_CONTENT, MATCH_PARENT or the size already calculated in pixels (instead of
the resource identifier).

https://codereview.chromium.org/2627153007/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:185: new
LinearLayout.LayoutParams(iconSize, iconSize);
here instead of creating a new layout params, I would get the existing ones,
which ensures we don't clobber margin top and bottom if they are applied.

ViewGroup.MarginLayoutParams layoutParams = (ViewGroup.MarginLayoutParams)
iconView.getLayoutParams();

[estark, 2017-01-19 07:03:39]

Patchset #4 (id:100001) has been deleted

[estark, 2017-01-19 07:07:29]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 07:07:52]

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

[estark, 2017-01-19 07:11:15]

Description was changed from

==========
Fix styling of Form-Not-Secure warnings on Android

This CL updates the styling of the Android form-not-secure warning to
match the specs (which are at go/fns-ui-spec).

The changes are:
- Make the "Payment autofilling disabled" a single-line label so that it
  has the same padding as the other autofill suggestions.
- Change the color of "Payment autofilling disabled" to #646464.
- Make both the label and sublabel of the "Payment not secure" item font
  size 18sp, adjust the icon size to 24, and change horizontal spacings
  between elements to 16.

These changes are implemented by allowing AutofillSuggestion to override
the necessary layout values and font sizes for the "Payment autofilling
disabled" and "Payment/Login not secure" dropdown items.

Screenshots are in:
https://bugs.chromium.org/p/chromium/issues/detail?id=678479#c13

BUG=678479
==========

to

==========
Fix styling of Form-Not-Secure warnings on Android

This CL updates the styling of the Android form-not-secure warning to
match the specs (which are at go/fns-ui-spec).

The changes are:
- Make the "Payment autofilling disabled" a single-line label so that it
  has the same padding as the other autofill suggestions.
- Change the color of "Payment autofilling disabled" to #646464.
- Make both the label and sublabel of the "Payment not secure" item font
  size 18sp, adjust the icon size to 24, and change horizontal spacings
  between elements to 10.

These changes are implemented by allowing AutofillSuggestion to override
the necessary layout values and font sizes for the "Payment autofilling
disabled" and "Payment/Login not secure" dropdown items.

Screenshots are in:
https://bugs.chromium.org/p/chromium/issues/detail?id=678479#c13

BUG=678479
==========

[estark, 2017-01-19 07:16:48]

Thanks! Responded to your comments, Ted. Admittedly I haven't tested these
latest changes visually yet (no test device handy at the moment) so if you
prefer, feel free to hold off on reviewing the latest patch set until I've had a
chance to test in the morning. Will update here when I have.

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

https://codereview.chromium.org/2627153007/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:183: :
mContext.getResources().getDimensionPixelSize(item.getIconSizeResId());
On 2017/01/19 00:35:30, Ted C wrote:
> This is a "bit" odd.  When specifying width/height in layout params, you
specify
> either WRAP_CONTENT, MATCH_PARENT or the size in pixels.  By returning
> WRAP_CONTENT or a resource identifier, you are mixing up the two.  While I
> "think" this is ok, I don't know if I can 100% guarantee that the namspaces
> don't overlap.
> 
> I think it would be slightly cleaner to do one of the following:
> 
> 1.) Instead of returning WRAP_CONTENT, have it return 0 in the default as 0 is
> an invalid identifier.
> 2.) Change the method signature to be getLayoutParamDimension which can return
> WRAP_CONTENT, MATCH_PARENT or the size already calculated in pixels (instead
of
> the resource identifier).

Done (option #1)

https://codereview.chromium.org/2627153007/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:185: new
LinearLayout.LayoutParams(iconSize, iconSize);
On 2017/01/19 00:35:30, Ted C wrote:
> here instead of creating a new layout params, I would get the existing ones,
> which ensures we don't clobber margin top and bottom if they are applied.
> 
> ViewGroup.MarginLayoutParams layoutParams = (ViewGroup.MarginLayoutParams)
> iconView.getLayoutParams();

Done.

[commit-bot: I haz the power, 2017-01-19 08:13:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 08:13:36]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[estark, 2017-01-19 08:37:52]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 08:38:10]

Dry run: 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-19 09:15:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 09:15:25]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-01-19 16:55:42]

lgtm

[Ted C, 2017-01-19 16:56:52]

On 2017/01/19 16:55:42, Ted C wrote:
> lgtm

If you have to make any tweaks after testing on a device, I
"would" expect them to be trivial.  But yeah, the code looks
good to me now so go ahead and submit once you verify.

[estark, 2017-01-19 18:11:23]

On 2017/01/19 16:56:52, Ted C wrote:
> On 2017/01/19 16:55:42, Ted C wrote:
> > lgtm
> 
> If you have to make any tweaks after testing on a device, I
> "would" expect them to be trivial.  But yeah, the code looks
> good to me now so go ahead and submit once you verify.

Verified that everything looks good on a device. Thanks!

[estark, 2017-01-19 18:11:39]

The CQ bit was checked by estark@chromium.org

[estark, 2017-01-19 18:11:39]

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/2627153007/#ps140001
(title: "actually set the icon size")

[commit-bot: I haz the power, 2017-01-19 18:12:01]

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-19 18:16:50]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484849499282270,
"parent_rev": "ff0f8624d178d292103520afc0f1d8787f051c9f", "commit_rev":
"7723c111843d5e749daaf879c9df8205b17fe737"}

[commit-bot: I haz the power, 2017-01-19 18:17:23]

Description was changed from

==========
Fix styling of Form-Not-Secure warnings on Android

This CL updates the styling of the Android form-not-secure warning to
match the specs (which are at go/fns-ui-spec).

The changes are:
- Make the "Payment autofilling disabled" a single-line label so that it
  has the same padding as the other autofill suggestions.
- Change the color of "Payment autofilling disabled" to #646464.
- Make both the label and sublabel of the "Payment not secure" item font
  size 18sp, adjust the icon size to 24, and change horizontal spacings
  between elements to 10.

These changes are implemented by allowing AutofillSuggestion to override
the necessary layout values and font sizes for the "Payment autofilling
disabled" and "Payment/Login not secure" dropdown items.

Screenshots are in:
https://bugs.chromium.org/p/chromium/issues/detail?id=678479#c13

BUG=678479
==========

to

==========
Fix styling of Form-Not-Secure warnings on Android

This CL updates the styling of the Android form-not-secure warning to
match the specs (which are at go/fns-ui-spec).

The changes are:
- Make the "Payment autofilling disabled" a single-line label so that it
  has the same padding as the other autofill suggestions.
- Change the color of "Payment autofilling disabled" to #646464.
- Make both the label and sublabel of the "Payment not secure" item font
  size 18sp, adjust the icon size to 24, and change horizontal spacings
  between elements to 10.

These changes are implemented by allowing AutofillSuggestion to override
the necessary layout values and font sizes for the "Payment autofilling
disabled" and "Payment/Login not secure" dropdown items.

Screenshots are in:
https://bugs.chromium.org/p/chromium/issues/detail?id=678479#c13

BUG=678479

Review-Url: https://codereview.chromium.org/2627153007
Cr-Commit-Position: refs/heads/master@{#444790}
Committed:
https://chromium.googlesource.com/chromium/src/+/7723c111843d5e749daaf879c9df...
==========

[commit-bot: I haz the power, 2017-01-19 18:17:25]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/7723c111843d5e749daaf879c9df...