Show icons in Android web autofill dropdown

Show icons in Android web autofill dropdown

BUG=402131
Committed: https://crrev.com/e37f822491958dc21dc4dbaf94432d28f759b51d
Cr-Commit-Position: refs/heads/master@{#303297}

[aruslan, 2014-08-08 22:06:39]

Adding the bitmaps is slightly complicated and look like I outlined in the
comment.
Feel free to chime in with questions.

https://codereview.chromium.org/443193006/diff/20001/chrome/browser/ui/androi...
File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right):

https://codereview.chromium.org/443193006/diff/20001/chrome/browser/ui/androi...
chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:75: icon =
gfx::ConvertToJavaBitmap(rb.GetImageNamed(icon_id).ToSkBitmap());
1. Get all necessary resolutions for every image you need (MDPI, HDPI, XHDPI and
XXHDPI) and place them to e.g. chrome/android/java/res

E.g. chrome/android/java/res/drawable-hdpi/abc_visa_card_image.png

2. Add all relevant mappings to chrome/browser/android/resource_id.h
E.g.
DEFINE_RESOURCE_ID(IDR_ABC_VISA_CARD_IMAGE, R.drawable.abc_visa_card_image)

3. In the autofill_popup_view_android.cc instead of rb.GetImageNamed(icon_id)
use

#include "chrome/browser/android/resource_mapper.h"
ResourceMapper::MapFromChromiumId(icon_id)

4. ...
5. PROFIT!!!

[Evan Stade, 2014-08-28 20:34:34]

estade@chromium.org changed reviewers:
+ aruslan@chromium.org

[Evan Stade, 2014-08-28 20:35:32]

Thanks for the pointers, Ruslan. The code is updated, PTAL. The images in this
CL are still placeholders while we await the final resources.

[aruslan, 2014-08-28 20:47:44]

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

[aruslan, 2014-08-28 20:47:44]

newt@ -- could you please review it, you've true grit :) Thanks a ton!

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

https://codereview.chromium.org/443193006/diff/60001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:111:
iconView.setVisibility(View.VISIBLE);
nit - the indent looks weird or this is a web Code Review glitch.

[Evan Stade, 2014-08-28 20:49:59]

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

[Evan Stade, 2014-08-28 20:49:59]

+aurimas for
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java

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

https://codereview.chromium.org/443193006/diff/60001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:111:
iconView.setVisibility(View.VISIBLE);
On 2014/08/28 20:47:44, aruslan wrote:
> nit - the indent looks weird or this is a web Code Review glitch.

no glitch, it is actually weird. fixed.

[aurimas (slooooooooow), 2014-08-28 21:06:04]

https://codereview.chromium.org/443193006/diff/80001/chrome/browser/android/r...
File chrome/browser/android/resource_mapper.cc (right):

https://codereview.chromium.org/443193006/diff/80001/chrome/browser/android/r...
chrome/browser/android/resource_mapper.cc:11: #include
"grit/component_scaled_resources.h"
Why do we need this import?

https://codereview.chromium.org/443193006/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java
(right):

https://codereview.chromium.org/443193006/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java:34:
return 0;
Extract this 0 to be a constant in DropdownItem, something line NO_ICON

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/res/layo...
File ui/android/java/res/layout/dropdown_item.xml (right):

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/res/layo...
ui/android/java/res/layout/dropdown_item.xml:13:
android:id="@+id/dropdown_menu_text"
I don't think you need this id.

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/res/layo...
ui/android/java/res/layout/dropdown_item.xml:17:
android:gravity="center_vertical">
Do you need gravity center_vertical? You are setting your height to be
wrap_content, therefore centering within the parent will do nothing.

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/res/layo...
ui/android/java/res/layout/dropdown_item.xml:50:
android:layout_height="wrap_content"
I think you can set layout_gravity="end" instead of adding the spacer view.

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

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:87: TextView labelView
= (TextView) labelLayout.findViewById(R.id.dropdown_label);
Do you need labelLayout here? Can't you just call
layout.findViewById(R.id.dropdown_label)?

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:107: if
(item.getIconId() == 0) {
Use a constant you will add to DropdownItem instead of 0.

[Evan Stade, 2014-08-29 17:45:35]

https://codereview.chromium.org/443193006/diff/80001/chrome/browser/android/r...
File chrome/browser/android/resource_mapper.cc (right):

https://codereview.chromium.org/443193006/diff/80001/chrome/browser/android/r...
chrome/browser/android/resource_mapper.cc:11: #include
"grit/component_scaled_resources.h"
On 2014/08/28 21:06:04, aurimas wrote:
> Why do we need this import?

it's where the autofill resource ids live

https://codereview.chromium.org/443193006/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java
(right):

https://codereview.chromium.org/443193006/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java:34:
return 0;
On 2014/08/28 21:06:04, aurimas wrote:
> Extract this 0 to be a constant in DropdownItem, something line NO_ICON

Done.

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/res/layo...
File ui/android/java/res/layout/dropdown_item.xml (right):

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/res/layo...
ui/android/java/res/layout/dropdown_item.xml:13:
android:id="@+id/dropdown_menu_text"
On 2014/08/28 21:06:04, aurimas wrote:
> I don't think you need this id.

Done.

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/res/layo...
ui/android/java/res/layout/dropdown_item.xml:17:
android:gravity="center_vertical">
On 2014/08/28 21:06:04, aurimas wrote:
> Do you need gravity center_vertical? You are setting your height to be
> wrap_content, therefore centering within the parent will do nothing.

I guess not. I copied this from the LinearLayout that existed before, but it
doesn't appear to do anything.

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/res/layo...
ui/android/java/res/layout/dropdown_item.xml:50:
android:layout_height="wrap_content"
On 2014/08/28 21:06:04, aurimas wrote:
> I think you can set layout_gravity="end" instead of adding the spacer view.

That doesn't appear to work, and the internet tells me LinearLayout ignores
gravity in the axis of its layout (i.e. in the horizontal axis in this case).

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

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:87: TextView labelView
= (TextView) labelLayout.findViewById(R.id.dropdown_label);
On 2014/08/28 21:06:04, aurimas wrote:
> Do you need labelLayout here? Can't you just call
> layout.findViewById(R.id.dropdown_label)?

Done.

https://codereview.chromium.org/443193006/diff/80001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:107: if
(item.getIconId() == 0) {
On 2014/08/28 21:06:04, aurimas wrote:
> Use a constant you will add to DropdownItem instead of 0.

Done.

[aurimas (slooooooooow), 2014-08-29 18:51:53]

lgtm

[aurimas (slooooooooow), 2014-09-05 20:59:42]

On 2014/08/29 18:51:53, aurimas wrote:
> lgtm

Ping?

[Evan Stade, 2014-09-05 21:06:21]

On 2014/09/05 20:59:42, aurimas wrote:
> On 2014/08/29 18:51:53, aurimas wrote:
> > lgtm
> 
> Ping?

https://chromiumcodereview.appspot.com/443193006/#msg3

We don't have the image resources from Jeff yet.

[Evan Stade, 2014-09-05 21:09:11]

On 2014/09/05 21:06:21, Evan Stade wrote:
> On 2014/09/05 20:59:42, aurimas wrote:
> > On 2014/08/29 18:51:53, aurimas wrote:
> > > lgtm
> > 
> > Ping?
> 
> https://chromiumcodereview.appspot.com/443193006/#msg3
> 
> We don't have the image resources from Jeff yet.

(If you are blocked on this or there is some other incentive to land it now, we
could land this with the placeholder images and a one-line change to disable
showing the images, but I'd just as soon not add dead code).

[aurimas (slooooooooow), 2014-09-05 21:24:12]

On 2014/09/05 21:09:11, Evan Stade wrote:
> On 2014/09/05 21:06:21, Evan Stade wrote:
> > On 2014/09/05 20:59:42, aurimas wrote:
> > > On 2014/08/29 18:51:53, aurimas wrote:
> > > > lgtm
> > > 
> > > Ping?
> > 
> > https://chromiumcodereview.appspot.com/443193006/#msg3
> > 
> > We don't have the image resources from Jeff yet.
> 
> (If you are blocked on this or there is some other incentive to land it now,
we
> could land this with the placeholder images and a one-line change to disable
> showing the images, but I'd just as soon not add dead code).

I'm not blocked, just wanted to make sure that this CL is still moving. Thanks
for the update.

[Evan Stade, 2014-11-04 21:13:15]

ready to commit. PTAL (No major changes since last review.)

[aurimas (slooooooooow), 2014-11-04 21:17:39]

https://codereview.chromium.org/443193006/diff/160001/build/android/developer...
File build/android/developer_recommended_flags.gypi (right):

https://codereview.chromium.org/443193006/diff/160001/build/android/developer...
build/android/developer_recommended_flags.gypi:42: #'gyp_managed_install%': 1,
Why are you changing this flag? It is not related to this CL at all.

It is working fine for me.

https://codereview.chromium.org/443193006/diff/160001/ui/android/java/res/lay...
File ui/android/java/res/layout/dropdown_item.xml (right):

https://codereview.chromium.org/443193006/diff/160001/ui/android/java/res/lay...
ui/android/java/res/layout/dropdown_item.xml:42: <!-- Spacer to move the icon to
the right. -->
Why don't we simply make the linear layout that contains the text views expand?
Then we dont have this spacer view.

[Evan Stade, 2014-11-04 22:09:41]

https://codereview.chromium.org/443193006/diff/160001/build/android/developer...
File build/android/developer_recommended_flags.gypi (right):

https://codereview.chromium.org/443193006/diff/160001/build/android/developer...
build/android/developer_recommended_flags.gypi:42: #'gyp_managed_install%': 1,
On 2014/11/04 21:17:39, aurimas wrote:
> Why are you changing this flag? It is not related to this CL at all.
> 
> It is working fine for me. 

sorry, I forgot to re-upload after reverting this.

https://codereview.chromium.org/443193006/diff/160001/ui/android/java/res/lay...
File ui/android/java/res/layout/dropdown_item.xml (right):

https://codereview.chromium.org/443193006/diff/160001/ui/android/java/res/lay...
ui/android/java/res/layout/dropdown_item.xml:42: <!-- Spacer to move the icon to
the right. -->
On 2014/11/04 21:17:39, aurimas wrote:
> Why don't we simply make the linear layout that contains the text views
expand?
> Then we dont have this spacer view.

good idea, done.

[aurimas (slooooooooow), 2014-11-04 23:03:25]

lgtm!

[Evan Stade, 2014-11-04 23:03:52]

The CQ bit was checked by estade@chromium.org

[aurimas (slooooooooow), 2014-11-04 23:04:22]

https://codereview.chromium.org/443193006/diff/180001/ui/android/java/res/lay...
File ui/android/java/res/layout/dropdown_item.xml (right):

https://codereview.chromium.org/443193006/diff/180001/ui/android/java/res/lay...
ui/android/java/res/layout/dropdown_item.xml:16:
android:layout_width="wrap_content"
You want to change the width to 0dp since you have weight for it.

[commit-bot: I haz the power, 2014-11-04 23:05:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/443193006/180001

[aurimas (slooooooooow), 2014-11-04 23:08:13]

On 2014/11/04 23:04:22, aurimas wrote:
>
https://codereview.chromium.org/443193006/diff/180001/ui/android/java/res/lay...
> File ui/android/java/res/layout/dropdown_item.xml (right):
> 
>
https://codereview.chromium.org/443193006/diff/180001/ui/android/java/res/lay...
> ui/android/java/res/layout/dropdown_item.xml:16:
> android:layout_width="wrap_content"
> You want to change the width to 0dp since you have weight for it.

http://stackoverflow.com/questions/12016781/why-is-0dp-considered-a-performan...

[Evan Stade, 2014-11-04 23:09:52]

https://codereview.chromium.org/443193006/diff/180001/ui/android/java/res/lay...
File ui/android/java/res/layout/dropdown_item.xml (right):

https://codereview.chromium.org/443193006/diff/180001/ui/android/java/res/lay...
ui/android/java/res/layout/dropdown_item.xml:16:
android:layout_width="wrap_content"
On 2014/11/04 23:04:22, aurimas wrote:
> You want to change the width to 0dp since you have weight for it.

Wouldn't leaving layout_width at wrap_content effectively create a minimum width
(which in this case would be good)? I'm having trouble finding detailed docs but
I thought layout_weight only applied to how to divide leftover space.

[Evan Stade, 2014-11-04 23:10:34]

ping newt@ for:
    chrome/browser/android/resource_id.h
    chrome/browser/android/resource_mapper.cc
    ui/android/java/res/layout/dropdown_item.xml
    ui/android/java/src/org/chromium/ui/DropdownAdapter.java
    ui/android/java/src/org/chromium/ui/DropdownItem.java

[commit-bot: I haz the power, 2014-11-04 23:12:50]

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

[commit-bot: I haz the power, 2014-11-04 23:12:52]

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

[newt (away), 2014-11-05 19:12:49]

lgtm

[Evan Stade, 2014-11-05 20:51:13]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-11-05 20:52:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/443193006/220001

[commit-bot: I haz the power, 2014-11-05 21:11:21]

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

[commit-bot: I haz the power, 2014-11-05 21:11:22]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Evan Stade, 2014-11-05 22:51:28]

estade@chromium.org changed reviewers:
+ sgurun@chromium.org

[Evan Stade, 2014-11-05 22:51:29]

+sgurun@chromium.org for android_webview

[sgurun-gerrit only, 2014-11-06 19:56:10]

On 2014/11/05 22:51:29, Evan Stade wrote:
> mailto:+sgurun@chromium.org for android_webview

I think we can pass no_icon as you did since webview support is limited to
autocomplete, and my understanding is this is important for autofill of credit
card data. Note that the error I see right now is:
external/chromium_org/android_webview/java/src/org/chromium/android_webview/AwAutofillClient.java:88:
error: ')' expected
        array[index] = new AutofillSuggestion(name, label,
DropdownItem::NO_ICON, uniqueId);
                                                                       ^
which is C++ way of scoping.

[Evan Stade, 2014-11-07 17:12:01]

estade@chromium.org changed reviewers:
+ thestig@chromium.org

[Evan Stade, 2014-11-07 17:12:02]

Lei, can you take a look at the build files? (gn and gypi)

[sgurun-gerrit only, 2014-11-07 19:25:21]

On 2014/11/07 17:12:02, Evan Stade wrote:
> Lei, can you take a look at the build files? (gn and gypi)

aw lgtm

[Lei Zhang, 2014-11-07 19:32:50]

build files lgtm

[Evan Stade, 2014-11-07 20:40:15]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2014-11-07 20:40:30]

thanks all.

[commit-bot: I haz the power, 2014-11-07 20:41:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/443193006/340001

[commit-bot: I haz the power, 2014-11-07 21:35:35]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2014-11-07 21:36:18]

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