Adds animation as feature variation to keyboard accessory.

Adds animation as feature variation to keyboard accessory.

Moves keyboard accessory feature flag from switches to feature_list.

Desired layout and animation:

https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI_Keyacc_Onboarding#%2F14-icon-white%20bg-no%20overlay-video.mp4

Design doc:
https://docs.google.com/document/d/1LPO-unUyQ-YEGKpxB7tYA3j8yrwblyKf_U0eaBNBFx8/edit#

BUG=724624
Review-Url: https://codereview.chromium.org/2874933008
Cr-Commit-Position: refs/heads/master@{#473350}
Committed: https://chromium.googlesource.com/chromium/src/+/a447acf9e15594e71b329965d49e7334ff388563

[csashi, 2017-05-12 23:11:17]

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

[csashi, 2017-05-12 23:11:18]

Hi Rouslan,
Can you take a first look. I can add other reviewers after your pass. Thanks!
-sashi.

[csashi, 2017-05-12 23:11:58]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 23:12:51]

Dry run: CQ is trying da patch.

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

[csashi, 2017-05-13 00:36:32]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-13 00:37:19]

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-05-13 01:10:53]

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

[commit-bot: I haz the power, 2017-05-13 01:10:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[csashi, 2017-05-13 22:36:38]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-13 22:36:41]

Dry run: CQ is trying da patch.

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

[csashi, 2017-05-13 22:41:57]

Description was changed from

==========
Adds animation as feature variation to keyboard accessory.

Moves keyboard accessory feature flag from switches to feature_list.

BUG=
==========

to

==========
Adds animation as feature variation to keyboard accessory.

Moves keyboard accessory feature flag from switches to feature_list.

Desired layout and animation:

https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

BUG=
==========

[commit-bot: I haz the power, 2017-05-13 22:47:52]

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

[commit-bot: I haz the power, 2017-05-13 22:47:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[csashi, 2017-05-13 22:49:05]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-13 22:49:12]

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-05-13 23:59:04]

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

[commit-bot: I haz the power, 2017-05-13 23:59:05]

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

[csashi, 2017-05-14 02:48:55]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-14 02:49:02]

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-05-14 02:52:13]

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

[commit-bot: I haz the power, 2017-05-14 02:52:14]

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_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[csashi, 2017-05-14 03:34:57]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-14 03:35:04]

Dry run: CQ is trying da patch.

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

[csashi, 2017-05-14 04:02:12]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-14 04:02:19]

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-05-14 04:57:55]

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

[commit-bot: I haz the power, 2017-05-14 04:57:56]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-05-15 14:37:29]

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/re...
File chrome/android/java/res/drawable/ic_create_googblue.xml (right):

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/re...
chrome/android/java/res/drawable/ic_create_googblue.xml:2: <!-- Copyright 2016
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/re...
chrome/android/java/res/drawable/ic_create_googblue.xml:16: android:pathData="M3
17.25V21h3.75L17.81 9.94l-3.75-3.75L3 17.25zM20.71 7.04c.39-.39 .39 -1.02
100 character line limit similar to
https://cs.chromium.org/chromium/src/chrome/android/java/res/drawable/ic_sett...
please.

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:76:
* pixel width.
No need for the word "pixel" in this comment.

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:77:
* @param isFirstSuggestionAHint If true, the first suggestion is a nin-fillable
hint.
non-fillable

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryTest.java
(right):

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryTest.java:46:
@CommandLineFlags.Add({AutofillKeyboardAccessoryTest.ENABLE_AUTOFILL_KEYBOARD_ACCESSORY,
This constant is not used anywhere else, so it's OK to use a literal string
instead of a constant.

https://codereview.chromium.org/2874933008/diff/160001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2874933008/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1988: {"enable-autofill-keyboard-accessory-view",
Remove "enable-" part of the string?

https://codereview.chromium.org/2874933008/diff/160001/chrome/browser/ui/andr...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h
(right):

https://codereview.chromium.org/2874933008/diff/160001/chrome/browser/ui/andr...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:27:
unsigned int animation_duration_millis,
Let's keep signedness consistent throughout. This param is unsigned, but it's
stored into a signed member variable.

https://codereview.chromium.org/2874933008/diff/160001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/2874933008/diff/160001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:57:
* pixel width.
Remove the word "pixel" from this comment, please.

https://codereview.chromium.org/2874933008/diff/160001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:192:
int hint_width = getChildAt(0).getWidth();
hintWidth

https://codereview.chromium.org/2874933008/diff/160001/components/autofill/co...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/2874933008/diff/160001/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.cc:88: if
(IsHintBeforeKeyboardAccessorySuggestions()) {
#if defined(OS_ANDROID)

[csashi, 2017-05-15 20:07:26]

Hi Rouslan,
Please take a look. Thanks!
-sashi.

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/re...
File chrome/android/java/res/drawable/ic_create_googblue.xml (right):

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/re...
chrome/android/java/res/drawable/ic_create_googblue.xml:2: <!-- Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/05/15 14:37:29, ಠ_ಠ wrote:
> 2017

Done.

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/re...
chrome/android/java/res/drawable/ic_create_googblue.xml:16: android:pathData="M3
17.25V21h3.75L17.81 9.94l-3.75-3.75L3 17.25zM20.71 7.04c.39-.39 .39 -1.02
On 2017/05/15 14:37:29, ಠ_ಠ wrote:
> 100 character line limit similar to
>
https://cs.chromium.org/chromium/src/chrome/android/java/res/drawable/ic_sett...
> please.

Done.

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:76:
* pixel width.
On 2017/05/15 14:37:29, ಠ_ಠ wrote:
> No need for the word "pixel" in this comment.

Done.

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:77:
* @param isFirstSuggestionAHint If true, the first suggestion is a nin-fillable
hint.
On 2017/05/15 14:37:29, ಠ_ಠ wrote:
> non-fillable

I removed this as a constructor parameter because we show "CLEAR FORM" when form
is filled.

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryTest.java
(right):

https://codereview.chromium.org/2874933008/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryTest.java:46:
@CommandLineFlags.Add({AutofillKeyboardAccessoryTest.ENABLE_AUTOFILL_KEYBOARD_ACCESSORY,
On 2017/05/15 14:37:29, ಠ_ಠ wrote:
> This constant is not used anywhere else, so it's OK to use a literal string
> instead of a constant.

Done.

https://codereview.chromium.org/2874933008/diff/160001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2874933008/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1988: {"enable-autofill-keyboard-accessory-view",
On 2017/05/15 14:37:29, ಠ_ಠ wrote:
> Remove "enable-" part of the string?

Done.

https://codereview.chromium.org/2874933008/diff/160001/chrome/browser/ui/andr...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h
(right):

https://codereview.chromium.org/2874933008/diff/160001/chrome/browser/ui/andr...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:27:
unsigned int animation_duration_millis,
On 2017/05/15 14:37:29, ಠ_ಠ wrote:
> Let's keep signedness consistent throughout. This param is unsigned, but it's
> stored into a signed member variable.

Done.

https://codereview.chromium.org/2874933008/diff/160001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/2874933008/diff/160001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:57:
* pixel width.
On 2017/05/15 14:37:29, ಠ_ಠ wrote:
> Remove the word "pixel" from this comment, please.

Done.

https://codereview.chromium.org/2874933008/diff/160001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:192:
int hint_width = getChildAt(0).getWidth();
On 2017/05/15 14:37:29, ಠ_ಠ wrote:
> hintWidth

Done.

https://codereview.chromium.org/2874933008/diff/160001/components/autofill/co...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/2874933008/diff/160001/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.cc:88: if
(IsHintBeforeKeyboardAccessorySuggestions()) {
On 2017/05/15 14:37:29, ಠ_ಠ wrote:
> #if defined(OS_ANDROID)

Done.

[csashi, 2017-05-15 20:07:36]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 20:08:09]

Dry run: CQ is trying da patch.

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

[please use gerrit instead, 2017-05-15 20:12:09]

lgtm

[csashi, 2017-05-15 20:44:55]

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

[csashi, 2017-05-15 20:44:56]

Hi Ted,
Please review changes in chrome/android and chrome/browser

Hi Mathieu,
Please review changes in components/autofill.

Thanks!
-sashi.

[commit-bot: I haz the power, 2017-05-15 21:28:00]

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

[commit-bot: I haz the power, 2017-05-15 21:28:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[csashi, 2017-05-16 00:18:27]

Description was changed from

==========
Adds animation as feature variation to keyboard accessory.

Moves keyboard accessory feature flag from switches to feature_list.

Desired layout and animation:

https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

BUG=
==========

to

==========
Adds animation as feature variation to keyboard accessory.

Moves keyboard accessory feature flag from switches to feature_list.

Desired layout and animation:

https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Design doc:
https://docs.google.com/document/d/1LPO-unUyQ-YEGKpxB7tYA3j8yrwblyKf_U0eaBNBF...

BUG=
==========

[Mathieu, 2017-05-16 00:27:38]

components/autofill lgtm

[csashi, 2017-05-16 17:36:04]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 17:37:36]

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-05-16 18:24:27]

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

[commit-bot: I haz the power, 2017-05-16 18:24:29]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ted C, 2017-05-17 00:04:30]

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/re...
File chrome/android/java/res/drawable/ic_create_googblue.xml (right):

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/re...
chrome/android/java/res/drawable/ic_create_googblue.xml:6: <vector
xmlns:android="http://schemas.android.com/apk/res/android"
Hmm..we already have two pencil icons in Chrome

bookmark_edit_normal and bookmark_edit_active

We should really just consolidate all of these into one.  The gotcha is that it
doesn't have the line underneath it, but I don't think we should introduce a new
asset just for the line when we could reduce 3 into one and tint them.

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:76:
* If {@param animationDurationMillis} &gt; 0, we reverse animation to scroll the
first
style nit

If this is a continuation of shouldLimitLabelWidth, it should be aligned with
"If true, ..." above to make it clearer.  Seems like this would actually be
better to just be continuations on the animationDurationMillis comment though

If you want this to be separate, I believe it should go above the params and I
would put a blank line to split them up.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:36:
private static final int NUM_DIP_TO_ANIMATE_SCROLL_FROM = 100;
I would call this START_ANIMATION_TRANSLATION_X_DP = 100;

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:84:
public void showWithSuggestions(AutofillSuggestion[] suggestions, final boolean
isRtl) {
Is this called only a single time for each showing of keyboard suggestions? 
Just wondering if we need to worry about canceling animations or anything

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:167:
if (isFirstSuggestionAHint) {
these asserts seem duplicated with what is in the loop above.

I wonder if we should just have

isFirstSuggestionAHint up top, then in the loop just do:

boolean isHint = i == 0;

To try and consolidate/de-duplicate the checks.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:181:
// TODO(crbug/722897): Fix animation for RTL.
Won't the suggestions just flow from the right to left in this case?

Just wondering why this isn't

int translateX = isRtl ? NUM_DIP_TO_ANIMATE_FROM : -NUM_DIP_TO_ANIMATE_FROM;

The hint will be on the right so you want it to peak out and hide.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:183:
this, View.TRANSLATION_X, -NUM_DIP_TO_ANIMATE_SCROLL_FROM, 0);
translateX takes pixels, so despite your DIP in the name, you are always
animating 100 px regardless of density.

The easiest thing would be to put it in dimens.xml and read it from there. 
Otherwise, multiply it by context.getResources().getDisplayMetrics().density

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:193:
if (isFirstSuggestionAHint) {
if would do:

if (!isFirstSuggestionAHint) return;

Then the rest can be indented less.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:200:
mAnimator.setFloatValues(0, -hintWidth);
I would just create a new animation.  The only thing we are getting is the
duration to be shared right?

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:204:
mAnimator.start();
You'll need to make sure you haven't been dismissed by the time this gets
called.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:215:
mAnimator.start();
general question about this animation...translation doesn't normally affect
layout bounds.  scroll and translate do different things ("normally").

With you translating from off the screen onto screen.  I would assume the
suggestion on the far right (assuming LTR) would be clipped.  Then when you
translate the hint off the screen at the end of the process, I would think the
total drawable area for the autofill suggestions would be desired width -
hintWidth.

In the past when I've done these types of animations, I've need to "lie" to
android and tell them my total layout width was desired width + translation
delta.  Then once everything settles, you need to hide views/trigger
relayouts/do cleanups to get this all to fill the viewport nicely.  Maybe this
is all done for you by this view, which would be awesome.

[Ted C, 2017-05-17 00:04:55]

tedchoc@chromium.org changed reviewers:
+ twellington@chromium.org

[Ted C, 2017-05-17 00:04:56]

+twellington for the icon de-duplicate comment

[csashi, 2017-05-17 01:14:16]

Hi Ted and Theresa,
Please take a look. Thanks!
-sashi.

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/re...
File chrome/android/java/res/drawable/ic_create_googblue.xml (right):

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/re...
chrome/android/java/res/drawable/ic_create_googblue.xml:6: <vector
xmlns:android="http://schemas.android.com/apk/res/android"
On 2017/05/17 00:04:29, Ted C wrote:
> Hmm..we already have two pencil icons in Chrome
> 
> bookmark_edit_normal and bookmark_edit_active
> 
> We should really just consolidate all of these into one.  The gotcha is that
it
> doesn't have the line underneath it, but I don't think we should introduce a
new
> asset just for the line when we could reduce 3 into one and tint them.

How about I use bookmark_edit_normal? FWIW, bookmark_edit_normal is being used
in chrome/android/java/res/layout/payment_option_edit_icon.xml. I don't know if
the blue or the line underneath it is critical for the experiment. If you agree,
I can remove this asset.

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:76:
* If {@param animationDurationMillis} &gt; 0, we reverse animation to scroll the
first
On 2017/05/17 00:04:29, Ted C wrote:
> style nit
> 
> If this is a continuation of shouldLimitLabelWidth, it should be aligned with
> "If true, ..." above to make it clearer.  Seems like this would actually be
> better to just be continuations on the animationDurationMillis comment though
> 
> If you want this to be separate, I believe it should go above the params and I
> would put a blank line to split them up.

Done.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:36:
private static final int NUM_DIP_TO_ANIMATE_SCROLL_FROM = 100;
On 2017/05/17 00:04:30, Ted C wrote:
> I would call this START_ANIMATION_TRANSLATION_X_DP = 100;

Done.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:84:
public void showWithSuggestions(AutofillSuggestion[] suggestions, final boolean
isRtl) {
On 2017/05/17 00:04:30, Ted C wrote:
> Is this called only a single time for each showing of keyboard suggestions? 
> Just wondering if we need to worry about canceling animations or anything

It may be called again while previous animation in progress if I click on the
form element while animation is in progress - but that seems to work OK. It just
restarts the animation.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:167:
if (isFirstSuggestionAHint) {
On 2017/05/17 00:04:30, Ted C wrote:
> these asserts seem duplicated with what is in the loop above.
> 
> I wonder if we should just have
> 
> isFirstSuggestionAHint up top, then in the loop just do:
> 
> boolean isHint = i == 0;
> 
> To try and consolidate/de-duplicate the checks.

Done.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:181:
// TODO(crbug/722897): Fix animation for RTL.
On 2017/05/17 00:04:29, Ted C wrote:
> Won't the suggestions just flow from the right to left in this case?
> 
> Just wondering why this isn't
> 
> int translateX = isRtl ? NUM_DIP_TO_ANIMATE_FROM : -NUM_DIP_TO_ANIMATE_FROM;
> 
> The hint will be on the right so you want it to peak out and hide.

I tried setting developer options to force right-to-left-layout direction but
the suggestion did not flow from right to left. I was not sure why the
setLayoutDirection View.LAYOUT_DIRECTION_RTL did not change this.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:183:
this, View.TRANSLATION_X, -NUM_DIP_TO_ANIMATE_SCROLL_FROM, 0);
On 2017/05/17 00:04:29, Ted C wrote:
> translateX takes pixels, so despite your DIP in the name, you are always
> animating 100 px regardless of density.
> 
> The easiest thing would be to put it in dimens.xml and read it from there. 
> Otherwise, multiply it by context.getResources().getDisplayMetrics().density

Done.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:200:
mAnimator.setFloatValues(0, -hintWidth);
On 2017/05/17 00:04:29, Ted C wrote:
> I would just create a new animation.  The only thing we are getting is the
> duration to be shared right?

Done.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:215:
mAnimator.start();
On 2017/05/17 00:04:29, Ted C wrote:
> general question about this animation...translation doesn't normally affect
> layout bounds.  scroll and translate do different things ("normally").
> 
> With you translating from off the screen onto screen.  I would assume the
> suggestion on the far right (assuming LTR) would be clipped.  Then when you
> translate the hint off the screen at the end of the process, I would think the
> total drawable area for the autofill suggestions would be desired width -
> hintWidth.
> 
> In the past when I've done these types of animations, I've need to "lie" to
> android and tell them my total layout width was desired width + translation
> delta.  Then once everything settles, you need to hide views/trigger
> relayouts/do cleanups to get this all to fill the viewport nicely.  Maybe this
> is all done for you by this view, which would be awesome.

This is an issue and the view does not take care of this. I am still trying to
tweak this. From the design doc:
"""
When we have a single card animating into and out of the view port, the settings
icon is no longer laid out at the right edge of the screen.
See https://drive.google.com/file/d/0B-jTnU8uuAL9NFN1cXU0Y3pmbXc/view.
"""

The solution I am working on is tweaking the width and layout weight for the
separator view. Please see createSeparatorView. It is not yet working correctly
and am wondering if for the purpose of the experiment we can simply not show the
last settings icon (I suspect it has very low CTR).

[Theresa, 2017-05-17 15:13:46]

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/re...
File chrome/android/java/res/drawable/ic_create_googblue.xml (right):

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/re...
chrome/android/java/res/drawable/ic_create_googblue.xml:6: <vector
xmlns:android="http://schemas.android.com/apk/res/android"
On 2017/05/17 01:14:15, csashi wrote:
> On 2017/05/17 00:04:29, Ted C wrote:
> > Hmm..we already have two pencil icons in Chrome
> > 
> > bookmark_edit_normal and bookmark_edit_active
> > 
> > We should really just consolidate all of these into one.  The gotcha is that
> it
> > doesn't have the line underneath it, but I don't think we should introduce a
> new
> > asset just for the line when we could reduce 3 into one and tint them.
> 
> How about I use bookmark_edit_normal? FWIW, bookmark_edit_normal is being used
> in chrome/android/java/res/layout/payment_option_edit_icon.xml. I don't know
if
> the blue or the line underneath it is critical for the experiment. If you
agree,
> I can remove this asset.

Keeping bookmark_edit_normal sounds good to me. bookmark_edit_active (only used
in bookmarks) can be removed and tinted programatically (either as part of this
change or I can handle it separately).

Since bookmark_edit_normal hasn't been a bookmark-only asset for some time now,
let's rename it to ic_edit_24dp so that it's easier for others to find.

[csashi, 2017-05-17 18:38:53]

Hi Theresa and Ted,
Please take a look. Thanks!

I can remove the bookmark_edit_active in a followup change.
-sashi.

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/re...
File chrome/android/java/res/drawable/ic_create_googblue.xml (right):

https://codereview.chromium.org/2874933008/diff/200001/chrome/android/java/re...
chrome/android/java/res/drawable/ic_create_googblue.xml:6: <vector
xmlns:android="http://schemas.android.com/apk/res/android"
On 2017/05/17 15:13:46, Theresa wrote:
> On 2017/05/17 01:14:15, csashi wrote:
> > On 2017/05/17 00:04:29, Ted C wrote:
> > > Hmm..we already have two pencil icons in Chrome
> > > 
> > > bookmark_edit_normal and bookmark_edit_active
> > > 
> > > We should really just consolidate all of these into one.  The gotcha is
that
> > it
> > > doesn't have the line underneath it, but I don't think we should introduce
a
> > new
> > > asset just for the line when we could reduce 3 into one and tint them.
> > 
> > How about I use bookmark_edit_normal? FWIW, bookmark_edit_normal is being
used
> > in chrome/android/java/res/layout/payment_option_edit_icon.xml. I don't know
> if
> > the blue or the line underneath it is critical for the experiment. If you
> agree,
> > I can remove this asset.
> 
> Keeping bookmark_edit_normal sounds good to me. bookmark_edit_active (only
used
> in bookmarks) can be removed and tinted programatically (either as part of
this
> change or I can handle it separately).
> 
> Since bookmark_edit_normal hasn't been a bookmark-only asset for some time
now,
> let's rename it to ic_edit_24dp so that it's easier for others to find.

Done.

[csashi, 2017-05-17 18:47:02]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 18:48:18]

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-05-17 23:36:18]

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

[commit-bot: I haz the power, 2017-05-17 23:36:19]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[csashi, 2017-05-18 00:04:06]

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:84:
public void showWithSuggestions(AutofillSuggestion[] suggestions, final boolean
isRtl) {
On 2017/05/17 01:14:15, csashi wrote:
> On 2017/05/17 00:04:30, Ted C wrote:
> > Is this called only a single time for each showing of keyboard suggestions? 
> > Just wondering if we need to worry about canceling animations or anything
> 
> It may be called again while previous animation in progress if I click on the
> form element while animation is in progress - but that seems to work OK. It
just
> restarts the animation.

I am now trying a different variation to re-layout the views after animation is
over by removing the hint view. In that process, I added an animation cancel
also.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:215:
mAnimator.start();
On 2017/05/17 01:14:15, csashi wrote:
> On 2017/05/17 00:04:29, Ted C wrote:
> > general question about this animation...translation doesn't normally affect
> > layout bounds.  scroll and translate do different things ("normally").
> > 
> > With you translating from off the screen onto screen.  I would assume the
> > suggestion on the far right (assuming LTR) would be clipped.  Then when you
> > translate the hint off the screen at the end of the process, I would think
the
> > total drawable area for the autofill suggestions would be desired width -
> > hintWidth.
> > 
> > In the past when I've done these types of animations, I've need to "lie" to
> > android and tell them my total layout width was desired width + translation
> > delta.  Then once everything settles, you need to hide views/trigger
> > relayouts/do cleanups to get this all to fill the viewport nicely.  Maybe
this
> > is all done for you by this view, which would be awesome.
> 
> This is an issue and the view does not take care of this. I am still trying to
> tweak this. From the design doc:
> """
> When we have a single card animating into and out of the view port, the
settings
> icon is no longer laid out at the right edge of the screen.
> See https://drive.google.com/file/d/0B-jTnU8uuAL9NFN1cXU0Y3pmbXc/view.
> """
> 
> The solution I am working on is tweaking the width and layout weight for the
> separator view. Please see createSeparatorView. It is not yet working
correctly
> and am wondering if for the purpose of the experiment we can simply not show
the
> last settings icon (I suspect it has very low CTR).
> 

I did not have much success with tweaking the width and layout weight. Instead I
now remove the hint view after animation is over. It is still not working quite
right - I suspect there is one more padding I need to keep.

See https://drive.google.com/open?id=0B-jTnU8uuAL9SXI0NDc5bk9tNDg

[csashi, 2017-05-18 00:53:04]

Hi Ted and Theresa,
I resolved the post animation alignment issue. Please take a look. Thanks!
-sashi.

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/2874933008/diff/200001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:215:
mAnimator.start();
On 2017/05/18 00:04:05, csashi wrote:
> On 2017/05/17 01:14:15, csashi wrote:
> > On 2017/05/17 00:04:29, Ted C wrote:
> > > general question about this animation...translation doesn't normally
affect
> > > layout bounds.  scroll and translate do different things ("normally").
> > > 
> > > With you translating from off the screen onto screen.  I would assume the
> > > suggestion on the far right (assuming LTR) would be clipped.  Then when
you
> > > translate the hint off the screen at the end of the process, I would think
> the
> > > total drawable area for the autofill suggestions would be desired width -
> > > hintWidth.
> > > 
> > > In the past when I've done these types of animations, I've need to "lie"
to
> > > android and tell them my total layout width was desired width +
translation
> > > delta.  Then once everything settles, you need to hide views/trigger
> > > relayouts/do cleanups to get this all to fill the viewport nicely.  Maybe
> this
> > > is all done for you by this view, which would be awesome.
> > 
> > This is an issue and the view does not take care of this. I am still trying
to
> > tweak this. From the design doc:
> > """
> > When we have a single card animating into and out of the view port, the
> settings
> > icon is no longer laid out at the right edge of the screen.
> > See https://drive.google.com/file/d/0B-jTnU8uuAL9NFN1cXU0Y3pmbXc/view.
> > """
> > 
> > The solution I am working on is tweaking the width and layout weight for the
> > separator view. Please see createSeparatorView. It is not yet working
> correctly
> > and am wondering if for the purpose of the experiment we can simply not show
> the
> > last settings icon (I suspect it has very low CTR).
> > 
> 
> I did not have much success with tweaking the width and layout weight. Instead
I
> now remove the hint view after animation is over. It is still not working
quite
> right - I suspect there is one more padding I need to keep.
> 
> See https://drive.google.com/open?id=0B-jTnU8uuAL9SXI0NDc5bk9tNDg

I resolved this issue by resetting the 'X'. See
https://drive.google.com/open?id=0B-jTnU8uuAL9MjluS1oxbzlkM2s

[csashi, 2017-05-18 17:04:15]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 17:05:05]

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-05-18 18:56:58]

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

[commit-bot: I haz the power, 2017-05-18 18:56:59]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ted C, 2017-05-19 16:49:46]

https://codereview.chromium.org/2874933008/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/2874933008/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:75:
* direction. We reverse animation to scroll the first suggestion (which is a
hint to call
nit

Should align with "If 0, ..." for these following lines as it breaks up the
comments visually and makes them easier to grok in code

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:193:
this, View.TRANSLATION_X, -mStartAnimationTranslationPx, 0);
I "think" we can make the animation look better by doing "something" like the
following.

mForwardAnimator = new AnimatorSet();
mForwardAnimator.setDuration(...)

List<Animator> animations = new ArrayList<>();
animations.add(ObjectAnimator.ofFloat(...));
animations.add(ObjectAnimator.ofFloat(getChildAt(getChildCount() - 1),
       View.TRANSLATION_X, mStartAnimationTranslationPx, 0);

mForwardAnimator.playTogether(animations);

The goal of this would be to keep the last view (i.e. the settings icon) right
aligned during the animation to prevent the jumping at the end when you set the
translation back and set the hint view to gone.  You would end up needing to do
something in the reverse direction to keep it right aligned there as well.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:254:
private View createSeparatorView(boolean isFirstSuggestionAHint) {
the param isn't used

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:263:
private void cancelAnimations(final HorizontalScrollView container) {
Do you need the param to be a HorizontalScrollView or should we just pass in
View here?

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:264:
if (mForwardAnimator != null && mForwardAnimator.isStarted())
mForwardAnimator.cancel();
any reason to keep the animators separately?  You only want one to be running at
a time right?  If so, I would just have a single value.

And to be extra cautious, I would set them to null here too.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:271:
private void scheduleReverseAnimation(final HorizontalScrollView container) {
same thing, I think you should be able to pass a generic View or ViewGroup here
instead

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:286:
public void onAnimationEnd(Animator animator) {
Since you are canceling these animations above, you need to be sure it is OK to
call onAnimationEnd when you cancel them (it gets called when cancelled or ended
naturally).

We have a CancelAwareAnimatorListener in the chrome/ package to help with that. 
If we needed something similar here, we could move it to ui/ I think.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:291:
setX(0);
I think you want setTranslationX(0) here

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:292:
postInvalidate();
removing the view, setting the position will trigger both a layout and
invalidate, so you shouldn't need this

[csashi, 2017-05-19 17:25:44]

https://codereview.chromium.org/2874933008/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/2874933008/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:75:
* direction. We reverse animation to scroll the first suggestion (which is a
hint to call
On 2017/05/19 16:49:45, Ted C wrote:
> nit
> 
> Should align with "If 0, ..." for these following lines as it breaks up the
> comments visually and makes them easier to grok in code

Done.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:193:
this, View.TRANSLATION_X, -mStartAnimationTranslationPx, 0);
On 2017/05/19 16:49:45, Ted C wrote:
> I "think" we can make the animation look better by doing "something" like the
> following.
> 
> mForwardAnimator = new AnimatorSet();
> mForwardAnimator.setDuration(...)
> 
> List<Animator> animations = new ArrayList<>();
> animations.add(ObjectAnimator.ofFloat(...));
> animations.add(ObjectAnimator.ofFloat(getChildAt(getChildCount() - 1),
>        View.TRANSLATION_X, mStartAnimationTranslationPx, 0);
> 
> mForwardAnimator.playTogether(animations);
> 
> The goal of this would be to keep the last view (i.e. the settings icon) right
> aligned during the animation to prevent the jumping at the end when you set
the
> translation back and set the hint view to gone.  You would end up needing to
do
> something in the reverse direction to keep it right aligned there as well.

Acknowledged. Another polish I would like to try is the jumping of the keyboard
accessory from the bottom of the screen to the top of the keyboard. If it is
fine with you, I would like to resolve other issues and work on removing the
jitters in the animation.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:254:
private View createSeparatorView(boolean isFirstSuggestionAHint) {
On 2017/05/19 16:49:46, Ted C wrote:
> the param isn't used

Done.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:263:
private void cancelAnimations(final HorizontalScrollView container) {
On 2017/05/19 16:49:45, Ted C wrote:
> Do you need the param to be a HorizontalScrollView or should we just pass in
> View here?

Done.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:264:
if (mForwardAnimator != null && mForwardAnimator.isStarted())
mForwardAnimator.cancel();
On 2017/05/19 16:49:46, Ted C wrote:
> any reason to keep the animators separately?  You only want one to be running
at
> a time right?  If so, I would just have a single value.
> 
> And to be extra cautious, I would set them to null here too.

Done.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:271:
private void scheduleReverseAnimation(final HorizontalScrollView container) {
On 2017/05/19 16:49:45, Ted C wrote:
> same thing, I think you should be able to pass a generic View or ViewGroup
here
> instead

Done.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:286:
public void onAnimationEnd(Animator animator) {
On 2017/05/19 16:49:46, Ted C wrote:
> Since you are canceling these animations above, you need to be sure it is OK
to
> call onAnimationEnd when you cancel them (it gets called when cancelled or
ended
> naturally).
> 
> We have a CancelAwareAnimatorListener in the chrome/ package to help with
that. 
> If we needed something similar here, we could move it to ui/ I think.

Acknowledged. I manually tested this path. It is OK to call onAnimationEnd when
we cancel because we simply restart the animation with a fresh View Group.

Having said that, CancelAwareAnimatorListener is clearly better interface than
the AnimatorListenerAdapter and I can move it if you wish.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:291:
setX(0);
On 2017/05/19 16:49:45, Ted C wrote:
> I think you want setTranslationX(0) here

Done.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:292:
postInvalidate();
On 2017/05/19 16:49:45, Ted C wrote:
> removing the view, setting the position will trigger both a layout and
> invalidate, so you shouldn't need this

Done.

[csashi, 2017-05-19 17:26:07]

On 2017/05/19 17:25:44, csashi wrote:
>
https://codereview.chromium.org/2874933008/diff/240001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
> (right):
> 
>
https://codereview.chromium.org/2874933008/diff/240001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:75:
> * direction. We reverse animation to scroll the first suggestion (which is a
> hint to call
> On 2017/05/19 16:49:45, Ted C wrote:
> > nit
> > 
> > Should align with "If 0, ..." for these following lines as it breaks up the
> > comments visually and makes them easier to grok in code
> 
> Done.
> 
>
https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
> File
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
> (right):
> 
>
https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:193:
> this, View.TRANSLATION_X, -mStartAnimationTranslationPx, 0);
> On 2017/05/19 16:49:45, Ted C wrote:
> > I "think" we can make the animation look better by doing "something" like
the
> > following.
> > 
> > mForwardAnimator = new AnimatorSet();
> > mForwardAnimator.setDuration(...)
> > 
> > List<Animator> animations = new ArrayList<>();
> > animations.add(ObjectAnimator.ofFloat(...));
> > animations.add(ObjectAnimator.ofFloat(getChildAt(getChildCount() - 1),
> >        View.TRANSLATION_X, mStartAnimationTranslationPx, 0);
> > 
> > mForwardAnimator.playTogether(animations);
> > 
> > The goal of this would be to keep the last view (i.e. the settings icon)
right
> > aligned during the animation to prevent the jumping at the end when you set
> the
> > translation back and set the hint view to gone.  You would end up needing to
> do
> > something in the reverse direction to keep it right aligned there as well.
> 
> Acknowledged. Another polish I would like to try is the jumping of the
keyboard
> accessory from the bottom of the screen to the top of the keyboard. If it is
> fine with you, I would like to resolve other issues and work on removing the
> jitters in the animation.
> 
>
https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:254:
> private View createSeparatorView(boolean isFirstSuggestionAHint) {
> On 2017/05/19 16:49:46, Ted C wrote:
> > the param isn't used
> 
> Done.
> 
>
https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:263:
> private void cancelAnimations(final HorizontalScrollView container) {
> On 2017/05/19 16:49:45, Ted C wrote:
> > Do you need the param to be a HorizontalScrollView or should we just pass in
> > View here?
> 
> Done.
> 
>
https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:264:
> if (mForwardAnimator != null && mForwardAnimator.isStarted())
> mForwardAnimator.cancel();
> On 2017/05/19 16:49:46, Ted C wrote:
> > any reason to keep the animators separately?  You only want one to be
running
> at
> > a time right?  If so, I would just have a single value.
> > 
> > And to be extra cautious, I would set them to null here too.
> 
> Done.
> 
>
https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:271:
> private void scheduleReverseAnimation(final HorizontalScrollView container) {
> On 2017/05/19 16:49:45, Ted C wrote:
> > same thing, I think you should be able to pass a generic View or ViewGroup
> here
> > instead
> 
> Done.
> 
>
https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:286:
> public void onAnimationEnd(Animator animator) {
> On 2017/05/19 16:49:46, Ted C wrote:
> > Since you are canceling these animations above, you need to be sure it is OK
> to
> > call onAnimationEnd when you cancel them (it gets called when cancelled or
> ended
> > naturally).
> > 
> > We have a CancelAwareAnimatorListener in the chrome/ package to help with
> that. 
> > If we needed something similar here, we could move it to ui/ I think.
> 
> Acknowledged. I manually tested this path. It is OK to call onAnimationEnd
when
> we cancel because we simply restart the animation with a fresh View Group.
> 
> Having said that, CancelAwareAnimatorListener is clearly better interface than
> the AnimatorListenerAdapter and I can move it if you wish.
> 
>
https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:291:
> setX(0);
> On 2017/05/19 16:49:45, Ted C wrote:
> > I think you want setTranslationX(0) here
> 
> Done.
> 
>
https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:292:
> postInvalidate();
> On 2017/05/19 16:49:45, Ted C wrote:
> > removing the view, setting the position will trigger both a layout and
> > invalidate, so you shouldn't need this
> 
> Done.

Hi Ted,
Please take a look. Thanks!
-sashi.

[Ted C, 2017-05-19 17:36:14]

lgtm

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:193:
this, View.TRANSLATION_X, -mStartAnimationTranslationPx, 0);
On 2017/05/19 17:25:43, csashi wrote:
> On 2017/05/19 16:49:45, Ted C wrote:
> > I "think" we can make the animation look better by doing "something" like
the
> > following.
> > 
> > mForwardAnimator = new AnimatorSet();
> > mForwardAnimator.setDuration(...)
> > 
> > List<Animator> animations = new ArrayList<>();
> > animations.add(ObjectAnimator.ofFloat(...));
> > animations.add(ObjectAnimator.ofFloat(getChildAt(getChildCount() - 1),
> >        View.TRANSLATION_X, mStartAnimationTranslationPx, 0);
> > 
> > mForwardAnimator.playTogether(animations);
> > 
> > The goal of this would be to keep the last view (i.e. the settings icon)
right
> > aligned during the animation to prevent the jumping at the end when you set
> the
> > translation back and set the hint view to gone.  You would end up needing to
> do
> > something in the reverse direction to keep it right aligned there as well.
> 
> Acknowledged. Another polish I would like to try is the jumping of the
keyboard
> accessory from the bottom of the screen to the top of the keyboard. If it is
> fine with you, I would like to resolve other issues and work on removing the
> jitters in the animation.

I guess I'm hesitant to add a noticeably janky animation.  Is the keyboard
accessory still an experiment with no immediate launch plans to large stable
percentages?

If so, I think it is ok to follow up, but I don't think my proposed change is
terribly difficult and should make this feel much nicer, but I'll defer to you.

FWIW, I don't think you'll be able to solve the keyboard jumping issue  in the
way you want (i.e. where it would nicely track the keyboard flowing up).  If
anything, you'll have to just defer showing the accessory, wait for the layout
of the screen to shrink then do a y translation and maybe a subtle fade or
something.  Doing things that match keyboard appearing is nearly impossible, so
I want to preemptively set you up to aim lower :-/

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:286:
public void onAnimationEnd(Animator animator) {
On 2017/05/19 17:25:43, csashi wrote:
> On 2017/05/19 16:49:46, Ted C wrote:
> > Since you are canceling these animations above, you need to be sure it is OK
> to
> > call onAnimationEnd when you cancel them (it gets called when cancelled or
> ended
> > naturally).
> > 
> > We have a CancelAwareAnimatorListener in the chrome/ package to help with
> that. 
> > If we needed something similar here, we could move it to ui/ I think.
> 
> Acknowledged. I manually tested this path. It is OK to call onAnimationEnd
when
> we cancel because we simply restart the animation with a fresh View Group.
> 
> Having said that, CancelAwareAnimatorListener is clearly better interface than
> the AnimatorListenerAdapter and I can move it if you wish.

Unless you need it, I don't think there is a point to move it yet.

[csashi, 2017-05-19 17:42:13]

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:193:
this, View.TRANSLATION_X, -mStartAnimationTranslationPx, 0);
On 2017/05/19 17:36:14, Ted C wrote:
> On 2017/05/19 17:25:43, csashi wrote:
> > On 2017/05/19 16:49:45, Ted C wrote:
> > > I "think" we can make the animation look better by doing "something" like
> the
> > > following.
> > > 
> > > mForwardAnimator = new AnimatorSet();
> > > mForwardAnimator.setDuration(...)
> > > 
> > > List<Animator> animations = new ArrayList<>();
> > > animations.add(ObjectAnimator.ofFloat(...));
> > > animations.add(ObjectAnimator.ofFloat(getChildAt(getChildCount() - 1),
> > >        View.TRANSLATION_X, mStartAnimationTranslationPx, 0);
> > > 
> > > mForwardAnimator.playTogether(animations);
> > > 
> > > The goal of this would be to keep the last view (i.e. the settings icon)
> right
> > > aligned during the animation to prevent the jumping at the end when you
set
> > the
> > > translation back and set the hint view to gone.  You would end up needing
to
> > do
> > > something in the reverse direction to keep it right aligned there as well.
> > 
> > Acknowledged. Another polish I would like to try is the jumping of the
> keyboard
> > accessory from the bottom of the screen to the top of the keyboard. If it is
> > fine with you, I would like to resolve other issues and work on removing the
> > jitters in the animation.
> 
> I guess I'm hesitant to add a noticeably janky animation.  Is the keyboard
> accessory still an experiment with no immediate launch plans to large stable
> percentages?
> 
> If so, I think it is ok to follow up, but I don't think my proposed change is
> terribly difficult and should make this feel much nicer, but I'll defer to
you.
> 
> FWIW, I don't think you'll be able to solve the keyboard jumping issue  in the
> way you want (i.e. where it would nicely track the keyboard flowing up).  If
> anything, you'll have to just defer showing the accessory, wait for the layout
> of the screen to shrink then do a y translation and maybe a subtle fade or
> something.  Doing things that match keyboard appearing is nearly impossible,
so
> I want to preemptively set you up to aim lower :-/

The reason I am deferring perfecting the animations are:

1. Yes, we first need to verify whether the animation improves keyboard
accessory usage in a 1% stable experiment (approval not yet granted).

2. The disappearing icon hint is only 1 variation which is likely more
controversial - we may not get approval to test this variation on 1% stable even
if I perfected the animation.

https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:286:
public void onAnimationEnd(Animator animator) {
On 2017/05/19 17:36:14, Ted C wrote:
> On 2017/05/19 17:25:43, csashi wrote:
> > On 2017/05/19 16:49:46, Ted C wrote:
> > > Since you are canceling these animations above, you need to be sure it is
OK
> > to
> > > call onAnimationEnd when you cancel them (it gets called when cancelled or
> > ended
> > > naturally).
> > > 
> > > We have a CancelAwareAnimatorListener in the chrome/ package to help with
> > that. 
> > > If we needed something similar here, we could move it to ui/ I think.
> > 
> > Acknowledged. I manually tested this path. It is OK to call onAnimationEnd
> when
> > we cancel because we simply restart the animation with a fresh View Group.
> > 
> > Having said that, CancelAwareAnimatorListener is clearly better interface
than
> > the AnimatorListenerAdapter and I can move it if you wish.
> 
> Unless you need it, I don't think there is a point to move it yet.

Acknowledged.

[csashi, 2017-05-19 17:42:50]

The CQ bit was checked by csashi@google.com

[csashi, 2017-05-19 17:42:50]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2874933008/#ps300001
(title: "Uses only 1 Animator object. Removes redundant call to invalidate.")

[commit-bot: I haz the power, 2017-05-19 17:43:43]

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-05-19 17:53:59]

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

[commit-bot: I haz the power, 2017-05-19 17:54:01]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[csashi, 2017-05-19 18:10:43]

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

[csashi, 2017-05-19 18:11:35]

On 2017/05/19 17:54:01, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Hi Robert,
Can you please approve for the DEPS change:

"""
You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
    '+components/variations',
"""
Thanks,
-sashi.

[csashi, 2017-05-19 18:28:38]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 18:29:41]

Dry run: CQ is trying da patch.

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

[rkaplow, 2017-05-19 19:48:28]

lgtm

[Alexei Svitkine (slow), 2017-05-19 19:57:21]

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

[Alexei Svitkine (slow), 2017-05-19 19:57:22]

This seems like a sizeable CL. Can you associate it with a relevant crbug? File
one if there isn't one yet.

https://codereview.chromium.org/2874933008/diff/300001/components/autofill/co...
File components/autofill/core/common/autofill_util.cc (right):

https://codereview.chromium.org/2874933008/diff/300001/components/autofill/co...
components/autofill/core/common/autofill_util.cc:73: const std::string
param_value = variations::GetVariationParamValueByFeature(
Please use base/metrics/field_trial_params.h APIs instead.

Then, you don't need the variations DEPS change.

[commit-bot: I haz the power, 2017-05-19 20:24:31]

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

[commit-bot: I haz the power, 2017-05-19 20:24:33]

Dry run: This issue passed the CQ dry run.

[csashi, 2017-05-19 20:25:19]

Description was changed from

==========
Adds animation as feature variation to keyboard accessory.

Moves keyboard accessory feature flag from switches to feature_list.

Desired layout and animation:

https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Design doc:
https://docs.google.com/document/d/1LPO-unUyQ-YEGKpxB7tYA3j8yrwblyKf_U0eaBNBF...

BUG=
==========

to

==========
Adds animation as feature variation to keyboard accessory.

Moves keyboard accessory feature flag from switches to feature_list.

Desired layout and animation:

https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Design doc:
https://docs.google.com/document/d/1LPO-unUyQ-YEGKpxB7tYA3j8yrwblyKf_U0eaBNBF...

BUG=724624
==========

[csashi, 2017-05-19 20:41:22]

https://codereview.chromium.org/2874933008/diff/300001/components/autofill/co...
File components/autofill/core/common/autofill_util.cc (right):

https://codereview.chromium.org/2874933008/diff/300001/components/autofill/co...
components/autofill/core/common/autofill_util.cc:73: const std::string
param_value = variations::GetVariationParamValueByFeature(
On 2017/05/19 19:57:22, Alexei Svitkine (slow) wrote:
> Please use base/metrics/field_trial_params.h APIs instead.
> 
> Then, you don't need the variations DEPS change.

Done.

[csashi, 2017-05-19 20:41:44]

The CQ bit was checked by csashi@google.com

[csashi, 2017-05-19 20:41:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, mathp@chromium.org, rouslan@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2874933008/#ps320001
(title: "Switch to base/metrics/field_trial_params.h API from variations::")

[commit-bot: I haz the power, 2017-05-19 20:42:40]

CQ is trying da patch.

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

[Alexei Svitkine (slow), 2017-05-19 20:44:09]

lgtm

[commit-bot: I haz the power, 2017-05-19 22:05:34]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1495226504444890,
"parent_rev": "fd04fe52772ad91203606d3797d5e7a8b6226c04", "commit_rev":
"a447acf9e15594e71b329965d49e7334ff388563"}

[commit-bot: I haz the power, 2017-05-19 22:05:46]

Description was changed from

==========
Adds animation as feature variation to keyboard accessory.

Moves keyboard accessory feature flag from switches to feature_list.

Desired layout and animation:

https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Design doc:
https://docs.google.com/document/d/1LPO-unUyQ-YEGKpxB7tYA3j8yrwblyKf_U0eaBNBF...

BUG=724624
==========

to

==========
Adds animation as feature variation to keyboard accessory.

Moves keyboard accessory feature flag from switches to feature_list.

Desired layout and animation:

https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Design doc:
https://docs.google.com/document/d/1LPO-unUyQ-YEGKpxB7tYA3j8yrwblyKf_U0eaBNBF...

BUG=724624

Review-Url: https://codereview.chromium.org/2874933008
Cr-Commit-Position: refs/heads/master@{#473350}
Committed:
https://chromium.googlesource.com/chromium/src/+/a447acf9e15594e71b329965d49e...
==========

[commit-bot: I haz the power, 2017-05-19 22:05:51]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/a447acf9e15594e71b329965d49e...