Gray horizontal keyboard accessory

Gray horizontal keyboard accessory

- Horizontal layout.
- Light gray chips with rounded corners on white background.
- Dark gray text with fading edge.

TBR=aurimas@chromium.org
BUG=428087
Committed: https://crrev.com/d043ba5fa93ef75ff01c6a942c9741ab45a3b254
Cr-Commit-Position: refs/heads/master@{#341972}

[please use gerrit instead, 2015-07-25 00:08:43]

rouslan@chromium.org changed reviewers:
+ aurimas@chromium.org

[please use gerrit instead, 2015-07-25 00:08:44]

Aurimas, PTAL.

This is still missing lots of UI niceties like shadows and ripples. However,
it's good enough to land, IMHO.

[aurimas (slooooooooow), 2015-07-28 00:10:28]

Could you post a screenshot to the bug to see what the UI looks like with this
CL?

https://codereview.chromium.org/1260583004/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/main.xml (right):

https://codereview.chromium.org/1260583004/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/main.xml:23:
android:paddingEnd="@dimen/keyboard_accessory_half_padding"
Do we want to add the padding to the scroll view or the view that is going to be
inside the scroll view? The question is where we want to clip. I would suggest
to add padding to the content as that way it can scroll all the way to the edge
instead of getting clipped.

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
File ui/android/java/res/layout/autofill_keyboard_accessory_item.xml (right):

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:6: <FrameLayout
xmlns:android="http://schemas.android.com/apk/res/android"
Is this framelayout only needed for the margin?

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:13:
android:paddingStart="@dimen/keyboard_accessory_half_padding"
Why is half of the padding here and the other half in the scrollview?

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:17:
android:background="@drawable/autofill_chip_shape"
Can we rename the drawable to simply autofill_chip?

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:35:
android:fontFamily="sans-serif-medium"
This will not work for pre API v21. -medium was introduced with Android 5.0. I
would make sure that it looks ok with pre-L devices.

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:42:
android:textColor="#DE000000"
Em, why do we need alpha for the text color? The background never changes. We
should just pick the color that looks the when same with 100% opacity.

Same for text color of the label.

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:50:
android:fontFamily="sans-serif-regular"
No need to specify this, it is the default

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:79:
for (int i = 0; i < suggestions.length; ++i) {
How large do we anticipate suggestions array to be?

[please use gerrit instead, 2015-07-28 21:07:46]

Aurimas, PTAL Patch Set 2.

Screenshots @ http://crbug.com/428087.

https://codereview.chromium.org/1260583004/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/main.xml (right):

https://codereview.chromium.org/1260583004/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/main.xml:23:
android:paddingEnd="@dimen/keyboard_accessory_half_padding"
On 2015/07/28 00:10:28, aurimas wrote:
> Do we want to add the padding to the scroll view or the view that is going to
be
> inside the scroll view? The question is where we want to clip. I would suggest
> to add padding to the content as that way it can scroll all the way to the
edge
> instead of getting clipped.

Done.

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
File ui/android/java/res/layout/autofill_keyboard_accessory_item.xml (right):

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:6: <FrameLayout
xmlns:android="http://schemas.android.com/apk/res/android"
On 2015/07/28 00:10:28, aurimas wrote:
> Is this framelayout only needed for the margin?

Yes, this is the white 8dp margin all around the the gray chip. Is there a
better way to achieve this?

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:13:
android:paddingStart="@dimen/keyboard_accessory_half_padding"
On 2015/07/28 00:10:28, aurimas wrote:
> Why is half of the padding here and the other half in the scrollview?

This is 4dp (half padding), so that the distance between chips is 8dp (half
padding + half padding). There is also 4dp horizontal padding in ListView (after
your suggestions). That makes the distance to the left and right edges of the
screen also 8dp.

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:17:
android:background="@drawable/autofill_chip_shape"
On 2015/07/28 00:10:28, aurimas wrote:
> Can we rename the drawable to simply autofill_chip?

Done.

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:35:
android:fontFamily="sans-serif-medium"
On 2015/07/28 00:10:28, aurimas wrote:
> This will not work for pre API v21. -medium was introduced with Android 5.0. I
> would make sure that it looks ok with pre-L devices.

The font falls back to regular on pre-L devices. UX is OK with a screenshot of
what it looks like, but may require changes after they get their hands on actual
prototype.

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:42:
android:textColor="#DE000000"
On 2015/07/28 00:10:28, aurimas wrote:
> Em, why do we need alpha for the text color? The background never changes. We
> should just pick the color that looks the when same with 100% opacity.
> 
> Same for text color of the label.

Done.

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/res/layout/...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:50:
android:fontFamily="sans-serif-regular"
On 2015/07/28 00:10:28, aurimas wrote:
> No need to specify this, it is the default

Done.

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1260583004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:79:
for (int i = 0; i < suggestions.length; ++i) {
On 2015/07/28 00:10:28, aurimas wrote:
> How large do we anticipate suggestions array to be?

Autofill.AddressSuggestionsCount histogram shows at most 7 address suggestions
for the 90%th percentile. The mean is 3.

[aurimas (slooooooooow), 2015-07-29 00:16:08]

https://codereview.chromium.org/1260583004/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/main.xml (right):

https://codereview.chromium.org/1260583004/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/main.xml:21: android:layout_height="wrap_content"
Can this be fixed height instead of wrap_content? Layout calculations would be
faster then.

https://codereview.chromium.org/1260583004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1260583004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:101:
* @param name Suggested text. Usually the text that's going to be filled in the
focused field.
"Usually"? What else could it be?

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
File ui/android/java/res/layout/autofill_keyboard_accessory_item.xml (right):

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:16:
<LinearLayout
We can remove FrameLayout and keep just LinearLayout by using InsetDrawable [1]
to draw the background that is slightly smaller than the view.

[1]
http://developer.android.com/reference/android/graphics/drawable/InsetDrawabl...

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:34:
android:ellipsize="none"
I think this might be the default value. Can you try removing this and make sure
it still works?

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:36:
android:gravity="center_vertical|start"
Do we need the gravity to be start? We set width to wrap_content so the text
will already be the same width as the TextView. Same with the text below.

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:42:
android:textColor="#333"
you can use @color/default_text_color

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:43:
android:textSize="@dimen/keyboard_accessory_text_size"
What happens when you set your device font to extra large in Android settings?

[please use gerrit instead, 2015-07-30 21:04:23]

Aurimas, PTAL Patch Set 3.

https://codereview.chromium.org/1260583004/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/main.xml (right):

https://codereview.chromium.org/1260583004/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/main.xml:21: android:layout_height="wrap_content"
On 2015/07/29 00:16:08, aurimas wrote:
> Can this be fixed height instead of wrap_content? Layout calculations would be
> faster then.

Done.

https://codereview.chromium.org/1260583004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1260583004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:101:
* @param name Suggested text. Usually the text that's going to be filled in the
focused field.
On 2015/07/29 00:16:08, aurimas wrote:
> "Usually"? What else could it be?

It could be "Visa ****-3333", which indicates that the the text to be filled is
"0000-1111-2222-3333".

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
File ui/android/java/res/layout/autofill_keyboard_accessory_item.xml (right):

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:16:
<LinearLayout
On 2015/07/29 00:16:08, aurimas wrote:
> We can remove FrameLayout and keep just LinearLayout by using InsetDrawable
[1]
> to draw the background that is slightly smaller than the view.
> 
> [1]
>
http://developer.android.com/reference/android/graphics/drawable/InsetDrawabl...

Done.

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:34:
android:ellipsize="none"
On 2015/07/29 00:16:08, aurimas wrote:
> I think this might be the default value. Can you try removing this and make
sure
> it still works?

Removing this results in "..." at the end of the TextView. So, ellipsize="end"
is the default.

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:36:
android:gravity="center_vertical|start"
On 2015/07/29 00:16:08, aurimas (OOO until Aug 17) wrote:
> Do we need the gravity to be start? We set width to wrap_content so the text
> will already be the same width as the TextView. Same with the text below.

Removed "start" and everything still looks correct.

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:42:
android:textColor="#333"
On 2015/07/29 00:16:08, aurimas wrote:
> you can use @color/default_text_color

default_text_color is in chrome/, which is not available in chrome/. (Weird that
we have this separation. I thought "chrome" essentially means user interface
that is not the webpage.)

https://codereview.chromium.org/1260583004/diff/20001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:43:
android:textSize="@dimen/keyboard_accessory_text_size"
On 2015/07/29 00:16:08, aurimas wrote:
> What happens when you set your device font to extra large in Android settings?

This text size increases as well, because I am using "sp" units.

[aurimas (slooooooooow), 2015-07-30 22:26:56]

aurimas@chromium.org changed reviewers:
+ newt@chromium.org

[aurimas (slooooooooow), 2015-07-30 22:26:56]

I'll be OOO for the next two weeks (starting tomorrow) so adding newt@ to
continue this review.

[please use gerrit instead, 2015-07-30 23:29:51]

Newton, PTAL Patch Set 3.

[newt (away), 2015-08-03 21:31:25]

https://codereview.chromium.org/1260583004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1260583004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:101:
* @param name Suggested text. Usually the text that's going to be filled in the
focused field.
Can you be more precise than "usually"? E.g. "The text that will be filled into
the focused field, but with credit card numbers elided (e.g. Visa-xxxx)."

https://codereview.chromium.org/1260583004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:103:
* the form.
nit: indent wrapped line

https://codereview.chromium.org/1260583004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:109:
String name, String label, int iconId, int suggestionId) {
Could you clean up the naming inconsistency here? In native code, these are
called (value, label). Here they're called (name, label), and in
AutofillSuggestion.java, they're called (label, sublabel). Can we at least
decrease the number of names from 3 to 2? e.g. by changing these to (label,
sublabel) and updating the parameters in AutofillSuggestion's constructor.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/res/dra...
File ui/android/java/res/drawable/autofill_chip_inset.xml (right):

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/res/dra...
ui/android/java/res/drawable/autofill_chip_inset.xml:7:
android:drawable="@drawable/autofill_chip"
You can actually inline autofill_chip.xml into here, which will make this
simpler to read.

   <inset ...>
     <shape>...
     </shape>
   </inset>

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/res/lay...
File ui/android/java/res/layout/autofill_keyboard_accessory_item.xml (right):

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:6: <LinearLayout
xmlns:android="http://schemas.android.com/apk/res/android"
formatting nit: indent wrapped lines by 4 spaces. Also, put android:id on its
own line.

    <LineaLayout xmlns...
        android:background=... >

        <TextView
            android:id=...
            ... />

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:19:
android:layout_height="match_parent"
put layout_* attributes at the top, after android:id. layout_* attributes are
treated differently because they're interpreted by the parent layout rather than
by the view itself.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:61:
int deviceWidthPx = DeviceDisplayInfo.create(getContext()).getDisplayWidth();
This logic is likely to break in multiwindow mode, and probably won't produce
great results on tablets.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:79:
public void showWithSuggestions(AutofillSuggestion[] suggestions, boolean isRtl)
{
Time to delete SuggestionAdapter.java and autofill_suggestion_item.xml?

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:81:
for (int i = 0; i < suggestions.length; ++i) {
for (AutofillSuggestion suggestion : suggestions)

It produces equivalent code (or possibly slightly better) and is simpler.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:85:
TextView name = (TextView) touchTarget.findViewById(
"name" or "label" or "sublabel" or "value"? Let's pick something and be
consistent. The code down below reads:

    name.setText(...getLabel());

    label.setText(...getSublabel());

Talk about trying to confuse your readers ;)

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:94:
if (suggestions[i].getLabel() != null && !suggestions[i].getLabel().isEmpty()) {
if (!TextUtils.isEmpty(suggestions[i].getLabel()))

But... why would the name/label ever be empty? If so, why are we showing an
empty view to the user?

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:97:
if (suggestions[i].getSublabel() != null
TextUtils.isEmpty()

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:117:
container.setHorizontalScrollBarEnabled(false);
Since this doesn't change dynamically, I'd just set android:scrollbars="none" in
main.xml.

[newt (away), 2015-08-03 22:13:50]

Also, it'd be a good idea to check how this works in RTL mode (change your
device's language to Hebrew or Arabic), and in LTR mode with RTL text (e.g.
create an autofill profile with an Arabic name but keep your device's language
in English).

[please use gerrit instead, 2015-08-05 00:18:47]

Patchset #5 (id:80001) has been deleted

[please use gerrit instead, 2015-08-05 00:28:08]

Newton, PTAL Patch Set 5.

https://codereview.chromium.org/1260583004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1260583004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:101:
* @param name Suggested text. Usually the text that's going to be filled in the
focused field.
On 2015/08/03 21:31:24, newt wrote:
> Can you be more precise than "usually"? E.g. "The text that will be filled
into
> the focused field, but with credit card numbers elided (e.g. Visa-xxxx)."

Done.

https://codereview.chromium.org/1260583004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:103:
* the form.
On 2015/08/03 21:31:24, newt wrote:
> nit: indent wrapped line

Done.

https://codereview.chromium.org/1260583004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:109:
String name, String label, int iconId, int suggestionId) {
On 2015/08/03 21:31:24, newt wrote:
> Could you clean up the naming inconsistency here? In native code, these are
> called (value, label). Here they're called (name, label), and in
> AutofillSuggestion.java, they're called (label, sublabel). Can we at least
> decrease the number of names from 3 to 2? e.g. by changing these to (label,
> sublabel) and updating the parameters in AutofillSuggestion's constructor.

Done.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/res/dra...
File ui/android/java/res/drawable/autofill_chip_inset.xml (right):

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/res/dra...
ui/android/java/res/drawable/autofill_chip_inset.xml:7:
android:drawable="@drawable/autofill_chip"
On 2015/08/03 21:31:24, newt wrote:
> You can actually inline autofill_chip.xml into here, which will make this
> simpler to read.
> 
>    <inset ...>
>      <shape>...
>      </shape>
>    </inset>

Done.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/res/lay...
File ui/android/java/res/layout/autofill_keyboard_accessory_item.xml (right):

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:6: <LinearLayout
xmlns:android="http://schemas.android.com/apk/res/android"
On 2015/08/03 21:31:24, newt wrote:
> formatting nit: indent wrapped lines by 4 spaces. Also, put android:id on its
> own line.
> 
>     <LineaLayout xmlns...
>         android:background=... >
> 
>         <TextView
>             android:id=...
>             ... />

Done.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:19:
android:layout_height="match_parent"
On 2015/08/03 21:31:24, newt wrote:
> put layout_* attributes at the top, after android:id. layout_* attributes are
> treated differently because they're interpreted by the parent layout rather
than
> by the view itself.

Done.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:61:
int deviceWidthPx = DeviceDisplayInfo.create(getContext()).getDisplayWidth();
On 2015/08/03 21:31:24, newt wrote:
> This logic is likely to break in multiwindow mode, and probably won't produce
> great results on tablets.

What do you recommend? From what I understand, UX wants to play around with
implementation of their specs before making any more changes. This is behind a
flag, so it won't break for everyone.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:79:
public void showWithSuggestions(AutofillSuggestion[] suggestions, boolean isRtl)
{
On 2015/08/03 21:31:24, newt wrote:
> Time to delete SuggestionAdapter.java and autofill_suggestion_item.xml?

Good point. I missed that they were used only here. Done.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:81:
for (int i = 0; i < suggestions.length; ++i) {
On 2015/08/03 21:31:24, newt wrote:
> for (AutofillSuggestion suggestion : suggestions)
> 
> It produces equivalent code (or possibly slightly better) and is simpler.

Done.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:85:
TextView name = (TextView) touchTarget.findViewById(
On 2015/08/03 21:31:24, newt wrote:
> "name" or "label" or "sublabel" or "value"? Let's pick something and be
> consistent. The code down below reads:
> 
>     name.setText(...getLabel());
> 
>     label.setText(...getSublabel());
> 
> Talk about trying to confuse your readers ;)

Done.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:94:
if (suggestions[i].getLabel() != null && !suggestions[i].getLabel().isEmpty()) {
On 2015/08/03 21:31:24, newt wrote:
> if (!TextUtils.isEmpty(suggestions[i].getLabel()))
> 
> But... why would the name/label ever be empty? If so, why are we showing an
> empty view to the user?

An empty name can be used to show an icon-only item. Currently "autofill
settings" and "scan credit card" are planned to be icon-only. Until recently,
"CLEAR FORM" was going to be an icon-only item as well, but that has recently
changed.

An empty label means that other fields in the form will not be filled. This
happens when the user has saved information for only one field in a form.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:97:
if (suggestions[i].getSublabel() != null
On 2015/08/03 21:31:25, newt wrote:
> TextUtils.isEmpty()

Done.

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:117:
container.setHorizontalScrollBarEnabled(false);
On 2015/08/03 21:31:24, newt wrote:
> Since this doesn't change dynamically, I'd just set android:scrollbars="none"
in
> main.xml.

Done.

[newt (away), 2015-08-05 20:41:40]

lgtm

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1260583004/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:61:
int deviceWidthPx = DeviceDisplayInfo.create(getContext()).getDisplayWidth();
On 2015/08/05 00:28:08, Rouslan wrote:
> On 2015/08/03 21:31:24, newt wrote:
> > This logic is likely to break in multiwindow mode, and probably won't
produce
> > great results on tablets.
> 
> What do you recommend? From what I understand, UX wants to play around with
> implementation of their specs before making any more changes. This is behind a
> flag, so it won't break for everyone.

It's fine to land this as is, but I'd also suggest seeing how it looks with a
min and max width for these elements, or just a fixed width.

[please use gerrit instead, 2015-08-05 20:47:36]

TBRing Aurimas for 
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc.

[please use gerrit instead, 2015-08-05 20:47:52]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2015-08-05 20:49:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260583004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260583004/100001

[commit-bot: I haz the power, 2015-08-05 20:57:23]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-08-05 20:57:56]

Patchset 5 (id:??) landed as
https://crrev.com/d043ba5fa93ef75ff01c6a942c9741ab45a3b254
Cr-Commit-Position: refs/heads/master@{#341972}