[android] Password generation UI for android.

[android] Password generation UI for android.

The feature is disabled through field trial infrastructure. Pass these
flags to try it out on a device:

--enable-password-generation
--local-heuristics-only-for-password-generation

BUG=412150
Committed: https://crrev.com/b2237a9c8e8c7c078157a02566bcd2f64f3eca94
Cr-Commit-Position: refs/heads/master@{#301526}

[please use gerrit instead, 2014-09-27 00:09:28]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2014-10-15 23:00:45]

Patchset #18 (id:410001) has been deleted

[please use gerrit instead, 2014-10-16 00:57:33]

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

[please use gerrit instead, 2014-10-16 00:57:35]

Aurimas: PTAL Patch Set 21.

I would like to check this in now to let people play with it and give me
feedback beyond what I already know.

The feature is disabled behind a field trial study that can be force-enabled
with --enable-password-generation flag. I also found
--local-heuristics-only-for-password-generation useful when playing with the UI.

I plan to ask aruslan@ for an owners review after you're satisfied.

[aurimas (slooooooooow), 2014-10-16 01:40:18]

comments from my 1 minute glance at this CL. Will take a proper look at this
tomorrow.

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:54: <color
name="password_generation_suggestion_text_color">#333333</color>
We should try to combine some of these colors. e.g. make #333 default_text
color.

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopup.java
(right):

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopup.java:92:
public enum PasswordGenerationView {
Using enums in Java is expensive. Change to using "final static int SUGGESTION =
0" and "final static int EXPLANATION = 1"

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java
(right):

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:93:
private void show(boolean isRtl, int minimumWidth, boolean passwordDisplayed,
String password,
Can you explain your reasoning why the bridge class in the one storing all the
information? Wouldn't be easier to pass all of it to the popup so we can
simplify the delegate interface?

[please use gerrit instead, 2014-10-16 03:31:34]

Aurimas: PTAL Patch Set 22.

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:54: <color
name="password_generation_suggestion_text_color">#333333</color>
On 2014/10/16 01:40:18, aurimas wrote:
> We should try to combine some of these colors. e.g. make #333 default_text
> color.

I combined infobar_text and password_generation_suggestion_text_color into
default_text_color with the value of #333. The rest of the colors are not
exactly the same. Let me know if the old colors need updating. For example
website_settings_popup_text_link does not match
password_generation_link_text_color. If a designer (rolfe@ for example) says
that we should use one or the other, than I will merge them.

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopup.java
(right):

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopup.java:92:
public enum PasswordGenerationView {
On 2014/10/16 01:40:18, aurimas wrote:
> Using enums in Java is expensive. Change to using "final static int SUGGESTION
=
> 0" and "final static int EXPLANATION = 1"

Done. I've made them public static final. I've seen some code that uses /*
package */ visibility qualifier instead. That could work here, because all the
objects are in the same package. What is the convention in Chrome?

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java
(right):

https://codereview.chromium.org/606153002/diff/490001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:93:
private void show(boolean isRtl, int minimumWidth, boolean passwordDisplayed,
String password,
On 2014/10/16 01:40:18, aurimas wrote:
> Can you explain your reasoning why the bridge class in the one storing all the
> information? Wouldn't be easier to pass all of it to the popup so we can
> simplify the delegate interface?

I agree that exposing the data through a delegate is an overkill. I've placed
the data into a POJO named PasswordGeneration that takes all this data in the
constructor. Would a Builder pattern be cleaner or an overkill for our purposes?

[aurimas (slooooooooow), 2014-10-16 23:30:01]

Another quick set of comments.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
File chrome/android/java/res/values/colors.xml (right):

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/res/values/colors.xml:40: <color
name="website_settings_popup_text">#333333</color>
Make this into default_text_color

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/res/values/colors.xml:55: <color
name="password_generation_link_text_color">#5595e3</color>
I'll start a new thread with UX team about unifying colors. Keep this one as it
is for now.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
File chrome/android/java/res/values/dimens.xml (right):

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/res/values/dimens.xml:85: <dimen
name="password_generation_icon_vertical_margin">13.5dp</dimen>
We shouldn't use half dps as it can look fuzzy on certain screen densities.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/res/values/dimens.xml:86: <dimen
name="password_generation_text_vertical_margin">12.5dp</dimen>
Same here.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGeneration.java
(right):

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGeneration.java:10:
public class PasswordGeneration {
I would just delete this class altogether and pass the values to
PasswordGenerationPopup, construct the adapter in the
PasswordGenerationPopupBridge and then only the required one to
PasswordGenerationAdapter.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java
(right):

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:100:
if (mPopup != null) {
Its pretty short so you can make it one line like:
if (mPopup != null) mPopup.dismiss();

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/browser/u...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/browser/u...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:217: #if
!defined(OS_ANDROID)
Are you planning a follow up CL for this?

[please use gerrit instead, 2014-10-17 05:19:00]

Aurimas: PTAL Patch Set 23.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
File chrome/android/java/res/values/colors.xml (right):

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/res/values/colors.xml:40: <color
name="website_settings_popup_text">#333333</color>
On 2014/10/16 23:30:01, aurimas wrote:
> Make this into default_text_color

Done.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/res/values/colors.xml:55: <color
name="password_generation_link_text_color">#5595e3</color>
On 2014/10/16 23:30:01, aurimas wrote:
> I'll start a new thread with UX team about unifying colors. Keep this one as
it
> is for now.

Thank you! Keep me posted, please.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
File chrome/android/java/res/values/dimens.xml (right):

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/res/values/dimens.xml:85: <dimen
name="password_generation_icon_vertical_margin">13.5dp</dimen>
On 2014/10/16 23:30:01, aurimas wrote:
> We shouldn't use half dps as it can look fuzzy on certain screen densities.

Changed 13.5dp -> 13dp.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/res/values/dimens.xml:86: <dimen
name="password_generation_text_vertical_margin">12.5dp</dimen>
On 2014/10/16 23:30:01, aurimas wrote:
> Same here.

Changed 12.5dp -> 12dp.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGeneration.java
(right):

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGeneration.java:10:
public class PasswordGeneration {
On 2014/10/16 23:30:01, aurimas wrote:
> I would just delete this class altogether and pass the values to
> PasswordGenerationPopup, construct the adapter in the
> PasswordGenerationPopupBridge and then only the required one to
> PasswordGenerationAdapter.

Great idea! Done.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java
(right):

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:100:
if (mPopup != null) {
On 2014/10/16 23:30:01, aurimas wrote:
> Its pretty short so you can make it one line like:
> if (mPopup != null) mPopup.dismiss();

Done.

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/browser/u...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://chromiumcodereview.appspot.com/606153002/diff/510001/chrome/browser/u...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:217: #if
!defined(OS_ANDROID)
On 2014/10/16 23:30:01, aurimas wrote:
> Are you planning a follow up CL for this?

Yes, I cannot remove the feature from behind the flag without this being
resolved. @mkwst might have some tips here, because he has a TODO for a similar
issue here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....

[aurimas (slooooooooow), 2014-10-17 05:42:13]

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java
(right):

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:121:
new LayoutParams(suggestion.getMeasuredWidth(), LayoutParams.WRAP_CONTENT));
Line breaks in Java are +8 spaces. Add 4 more spaces.

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:124:
suggestion.setVisibility(View.VISIBLE);
Instead of doing this hiding and showing you can have an adapter that has two
types of views: a suggestion view and an explanation view with different xml
layout files for each. Something similar to
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java
(right):

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:96:
mPopup.show(new PasswordGenerationAdapter(mContext, views, this, minimumWidth,
password,
Please pull out the construction of the adapter to a separate line to make it
easier to see what is going on.

Also you can call mPopup.setAdapter() here so you don't have to pass the adapter
to the popup.

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:119:
long nativePasswordGenerationPopupViewAndroid);
Java uses +8 spaces indentation for line breaks, so you need to add 4 more
spaces.

[please use gerrit instead, 2014-10-18 01:15:12]

Aurimas: PTAL Patch Set 25 & my question inline.

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java
(right):

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:121:
new LayoutParams(suggestion.getMeasuredWidth(), LayoutParams.WRAP_CONTENT));
On 2014/10/17 05:42:12, aurimas wrote:
> Line breaks in Java are +8 spaces. Add 4 more spaces.

Done.

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:124:
suggestion.setVisibility(View.VISIBLE);
On 2014/10/17 05:42:12, aurimas wrote:
> Instead of doing this hiding and showing you can have an adapter that has two
> types of views: a suggestion view and an explanation view with different xml
> layout files for each. Something similar to
>
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java

I've uploaded my approach to doing this, but it breaks somewhere deep inside of
Android. Can you help me understand what exactly does the system provide in
getView() method? In particular: what exactly is the "convertView" thing? It
seems that if I ignore "convertView" and return a cached UI element, then I get
a cast exception deep in teh bowels of the Android. I think that caching is the
best design for displaying static data, but i haven't seen anyone do that yet.

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java
(right):

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:96:
mPopup.show(new PasswordGenerationAdapter(mContext, views, this, minimumWidth,
password,
On 2014/10/17 05:42:13, aurimas wrote:
> Please pull out the construction of the adapter to a separate line to make it
> easier to see what is going on.
> 
> Also you can call mPopup.setAdapter() here so you don't have to pass the
adapter
> to the popup.

Done. The PasswordManagerPopup became so small there was no point in keeping it
anymore. The PasswordManagerPopupBridge now uses DropdownPopupWindow with
PasswordGenerationAdapter directly.

https://codereview.chromium.org/606153002/diff/530001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:119:
long nativePasswordGenerationPopupViewAndroid);
On 2014/10/17 05:42:13, aurimas wrote:
> Java uses +8 spaces indentation for line breaks, so you need to add 4 more
> spaces.

Done.

[please use gerrit instead, 2014-10-22 18:40:51]

Aurimas: PTAL Patch Set 27. I was able to get BaseAdapter to work, but I'm still
figuring out how to set the popup width to something sensible. (It's taking up
the whole width of the screen at the moment.)

https://codereview.chromium.org/606153002/diff/600001/chrome/android/java/res...
File chrome/android/java/res/layout/password_generation_popup_suggestion.xml
(right):

https://codereview.chromium.org/606153002/diff/600001/chrome/android/java/res...
chrome/android/java/res/layout/password_generation_popup_suggestion.xml:55:
android:background="@color/password_generation_divider_color"/>
I've originally placed the divider outside of the suggestion view to avoid it
influencing width calculations for the suggestion view. Now that I've separated
suggestion and explanation view, the popup is taking up the width of the whole
screen. I need to figure out how to set its width correctly. This might require
moving the divider view around.

https://codereview.chromium.org/606153002/diff/600001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java
(right):

https://codereview.chromium.org/606153002/diff/600001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:143:
.findViewById(R.id.password_generation_explanation);
Sometimes convertView for EXPLANATION has the layout for SUGGESTION instead. If
that happens, I throw away the old convertView and inflate the correct one
instead. It would be great to find out why that happens, so I don't have to
check that I have the correct convertView.

[please use gerrit instead, 2014-10-22 21:04:36]

https://codereview.chromium.org/606153002/diff/600001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java
(right):

https://codereview.chromium.org/606153002/diff/600001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:143:
.findViewById(R.id.password_generation_explanation);
On 2014/10/22 18:40:51, Rouslan Solomakhin wrote:
> Sometimes convertView for EXPLANATION has the layout for SUGGESTION instead.
If
> that happens, I throw away the old convertView and inflate the correct one
> instead. It would be great to find out why that happens, so I don't have to
> check that I have the correct convertView.

Figured it out: it's our own DropdownPopupWindow.measureContentWidth() that's
reusing the same contentView.

[please use gerrit instead, 2014-10-22 23:50:41]

Aurimas: PTAL Patch Set 30. I was able to get set the width of the popup
correctly after all.

[please use gerrit instead, 2014-10-23 00:54:09]

Aurimas: Patch Set 31 has the ability to open the "saved passwords" settings
page when you click on the corresponding link in the password generation popup.

[please use gerrit instead, 2014-10-23 00:55:00]

On 2014/10/23 00:54:09, Rouslan Solomakhin wrote:
> Aurimas: Patch Set 31 has the ability to open the "saved passwords" settings
> page when you click on the corresponding link in the password generation
popup.

This part of the patch can and probably should be separated into a separate
patch. I plan to do that tomorrow.

[aurimas (slooooooooow), 2014-10-23 00:57:01]

It is starting to look really good.

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
File chrome/android/java/res/layout/password_generation_popup_suggestion.xml
(right):

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
chrome/android/java/res/layout/password_generation_popup_suggestion.xml:19:
android:layout_marginTop="@dimen/password_generation_icon_vertical_margin"
This dimen only seems to be used in this file. It would be easier to read if you
just inline it.

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
chrome/android/java/res/layout/password_generation_popup_suggestion.xml:51:
<View android:id="@+id/password_generation_divider"
Could we actually use a real divider provided by ListPopupWindow? I think we
just need to call setDividerHeight and setDivider on the list view of the popup
window after we call show. Thoughts?

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java
(right):

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:27:
* The adapter that populates the list popup for password generation with data.
Can you update the comment to explain what the popup will show and how will the
things it display change based on the constructor arguments.

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:87:
mViews = passwordDisplayed ? Arrays.asList(SUGGESTION, EXPLANATION) : Arrays
Style nit: I would move ": Arrays" to the next line.

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:149:
textPaint.setUnderlineText(false);
Is it true that we really don't want the underline?

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java
(right):

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:55:
new Handler().post(new Runnable() {
Can you explain why we need this runnable? In what cases we we get mContext ==
null?

https://codereview.chromium.org/606153002/diff/660001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java (right):

https://codereview.chromium.org/606153002/diff/660001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:169: if (mAdapter
== null) return 0;
Why would mAdapter be null?

[please use gerrit instead, 2014-10-24 18:13:02]

Aurimas: PTAL Patch Set 36.

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
File chrome/android/java/res/layout/password_generation_popup_suggestion.xml
(right):

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
chrome/android/java/res/layout/password_generation_popup_suggestion.xml:19:
android:layout_marginTop="@dimen/password_generation_icon_vertical_margin"
On 2014/10/23 00:57:01, aurimas wrote:
> This dimen only seems to be used in this file. It would be easier to read if
you
> just inline it.

Done.

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
chrome/android/java/res/layout/password_generation_popup_suggestion.xml:51:
<View android:id="@+id/password_generation_divider"
On 2014/10/23 00:57:01, aurimas wrote:
> Could we actually use a real divider provided by ListPopupWindow? I think we
> just need to call setDividerHeight and setDivider on the list view of the
popup
> window after we call show. Thoughts?

setDivider() would require us to use DropdownDividerDrawable (or our own
implementation) and set its parameters like color and height. I prefer to keep
such styling information declared in this layout file instead. Would you be OK
with that?

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java
(right):

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:27:
* The adapter that populates the list popup for password generation with data.
On 2014/10/23 00:57:01, aurimas wrote:
> Can you update the comment to explain what the popup will show and how will
the
> things it display change based on the constructor arguments.

Done.

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:87:
mViews = passwordDisplayed ? Arrays.asList(SUGGESTION, EXPLANATION) : Arrays
On 2014/10/23 00:57:01, aurimas wrote:
> Style nit: I would move ": Arrays" to the next line.

Done. I use Android Java style in eclim to format my code and haven't seen the
option that would break the line after ":" instead of before ".". I hope I won't
forget to change this back the next time eclim reformats the code...

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:149:
textPaint.setUnderlineText(false);
On 2014/10/23 00:57:01, aurimas wrote:
> Is it true that we really don't want the underline?

Yes, UX explicitly requested no underline when I first showed them the
screenshots with underline. I think this is meant to match the desktop UI.

[aurimas (slooooooooow), 2014-10-24 20:35:52]

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
File chrome/android/java/res/layout/password_generation_popup_suggestion.xml
(right):

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
chrome/android/java/res/layout/password_generation_popup_suggestion.xml:51:
<View android:id="@+id/password_generation_divider"
On 2014/10/24 18:13:01, Rouslan Solomakhin wrote:
> On 2014/10/23 00:57:01, aurimas wrote:
> > Could we actually use a real divider provided by ListPopupWindow? I think we
> > just need to call setDividerHeight and setDivider on the list view of the
> popup
> > window after we call show. Thoughts?
> 
> setDivider() would require us to use DropdownDividerDrawable (or our own
> implementation) and set its parameters like color and height. I prefer to keep
> such styling information declared in this layout file instead. Would you be OK
> with that?

You could do something like this in java:
ColorDrawable sage = new
ColorDrawable(this.getResources().getColor(R.color.my_favorite_color));
listView.setDivider(sage);

[please use gerrit instead, 2014-10-24 22:10:15]

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
File chrome/android/java/res/layout/password_generation_popup_suggestion.xml
(right):

https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
chrome/android/java/res/layout/password_generation_popup_suggestion.xml:51:
<View android:id="@+id/password_generation_divider"
On 2014/10/24 20:35:52, aurimas wrote:
> On 2014/10/24 18:13:01, Rouslan Solomakhin wrote:
> > On 2014/10/23 00:57:01, aurimas wrote:
> > > Could we actually use a real divider provided by ListPopupWindow? I think
we
> > > just need to call setDividerHeight and setDivider on the list view of the
> > popup
> > > window after we call show. Thoughts?
> > 
> > setDivider() would require us to use DropdownDividerDrawable (or our own
> > implementation) and set its parameters like color and height. I prefer to
keep
> > such styling information declared in this layout file instead. Would you be
OK
> > with that?
> 
> You could do something like this in java:
> ColorDrawable sage = new
> ColorDrawable(this.getResources().getColor(R.color.my_favorite_color));
> listView.setDivider(sage);

I've tried both ColorDrawable and DropdownDividerDrawable. Neither show a
divider in the UI. (Of course, I removed setDividerHeight(0) in
DropdownPopupWindow first.) I'm not sure what sorcery is behind this behavior,
but it explains why DropdownPopupWindow resorts to drawing dividers by adding
DropdownDividerDrawable as elements in the popup instead of using setDivider()
directly.

[aurimas (slooooooooow), 2014-10-24 22:14:05]

On 2014/10/24 22:10:15, Rouslan Solomakhin wrote:
>
https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
> File chrome/android/java/res/layout/password_generation_popup_suggestion.xml
> (right):
> 
>
https://codereview.chromium.org/606153002/diff/660001/chrome/android/java/res...
> chrome/android/java/res/layout/password_generation_popup_suggestion.xml:51:
> <View android:id="@+id/password_generation_divider"
> On 2014/10/24 20:35:52, aurimas wrote:
> > On 2014/10/24 18:13:01, Rouslan Solomakhin wrote:
> > > On 2014/10/23 00:57:01, aurimas wrote:
> > > > Could we actually use a real divider provided by ListPopupWindow? I
think
> we
> > > > just need to call setDividerHeight and setDivider on the list view of
the
> > > popup
> > > > window after we call show. Thoughts?
> > > 
> > > setDivider() would require us to use DropdownDividerDrawable (or our own
> > > implementation) and set its parameters like color and height. I prefer to
> keep
> > > such styling information declared in this layout file instead. Would you
be
> OK
> > > with that?
> > 
> > You could do something like this in java:
> > ColorDrawable sage = new
> > ColorDrawable(this.getResources().getColor(R.color.my_favorite_color));
> > listView.setDivider(sage);
> 
> I've tried both ColorDrawable and DropdownDividerDrawable. Neither show a
> divider in the UI. (Of course, I removed setDividerHeight(0) in
> DropdownPopupWindow first.) I'm not sure what sorcery is behind this behavior,
> but it explains why DropdownPopupWindow resorts to drawing dividers by adding
> DropdownDividerDrawable as elements in the popup instead of using setDivider()
> directly.

Did you try calling setDividerHeight(10) or some other number to make sure it
shows even without your drawable setting?

[please use gerrit instead, 2014-10-24 22:32:27]

On 2014/10/24 22:14:05, aurimas wrote:
> Did you try calling setDividerHeight(10) or some other number to make sure it
> shows even without your drawable setting?

setDividerHeight(10) increases spacing between list items without drawing
anything in this space, regardless of whether setDraweable(...) is called.

[please use gerrit instead, 2014-10-24 22:33:48]

On 2014/10/24 22:32:27, Rouslan Solomakhin wrote:
> On 2014/10/24 22:14:05, aurimas wrote:
> > Did you try calling setDividerHeight(10) or some other number to make sure
it
> > shows even without your drawable setting?
> 
> setDividerHeight(10) increases spacing between list items without drawing
> anything in this space, regardless of whether setDraweable(...) is called.

s/setDraweable/setDivider/

[aurimas (slooooooooow), 2014-10-24 23:35:02]

Java + XML LGTM

https://codereview.chromium.org/606153002/diff/780001/chrome/android/java/res...
File chrome/android/java/res/layout/password_generation_popup_explanation.xml
(right):

https://codereview.chromium.org/606153002/diff/780001/chrome/android/java/res...
chrome/android/java/res/layout/password_generation_popup_explanation.xml:19:
android:includeFontPadding="false"
Why do we need to set this to false? I have seen very few places that use this
attribute.

https://codereview.chromium.org/606153002/diff/780001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java
(right):

https://codereview.chromium.org/606153002/diff/780001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:90:
mViewTypes = passwordDisplayed ? Arrays.asList(SUGGESTION, EXPLANATION) :
: on the next line

[please use gerrit instead, 2014-10-25 00:18:12]

https://codereview.chromium.org/606153002/diff/780001/chrome/android/java/res...
File chrome/android/java/res/layout/password_generation_popup_explanation.xml
(right):

https://codereview.chromium.org/606153002/diff/780001/chrome/android/java/res...
chrome/android/java/res/layout/password_generation_popup_explanation.xml:19:
android:includeFontPadding="false"
On 2014/10/24 23:35:01, aurimas wrote:
> Why do we need to set this to false? I have seen very few places that use this
> attribute.

Settings this to false removes the extra padding for accents that go beyond the
normal ascent and descent of the font. I've added this to make password
generation dropdown to look consistent with the rest of the dropdowns in Chrome.
See
https://code.google.com/p/chromium/codesearch#chromium/src/ui/android/java/re....

https://codereview.chromium.org/606153002/diff/780001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java
(right):

https://codereview.chromium.org/606153002/diff/780001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationAdapter.java:90:
mViewTypes = passwordDisplayed ? Arrays.asList(SUGGESTION, EXPLANATION) :
On 2014/10/24 23:35:02, aurimas wrote:
> : on the next line

Done.

[please use gerrit instead, 2014-10-25 00:20:45]

Aurimas, did you also mean to el-gee
chrome/browser/ui/android/autofill/password_generation_popup_view_android.{h,cc}?
You were the original author of autofill_popup_view_android.{h,cc}, which makes
you a good candidate to review the new JNI code. If not, I will ask aruslan@.

[please use gerrit instead, 2014-10-25 00:21:47]

rouslan@chromium.org changed reviewers:
+ #gcasto@chromium.or*

[please use gerrit instead, 2014-10-25 00:21:49]

Garrett: OWNERS PTAL
chrome/browser/password_manager/chrome_password_manager_client.cc.

[please use gerrit instead, 2014-10-25 00:22:21]

rouslan@chromium.org changed reviewers:
+ gcasto@chromium.or
- #gcasto@chromium.or*

[please use gerrit instead, 2014-10-25 00:22:21]

rouslan@chromium.org changed required reviewers:
+ gcasto@chromium.or

[please use gerrit instead, 2014-10-25 00:22:29]

rouslan@chromium.org changed reviewers:
+ gcasto@chromium.org
- gcasto@chromium.or

[please use gerrit instead, 2014-10-25 00:22:29]

rouslan@chromium.org changed required reviewers:
+ gcasto@chromium.org
- gcasto@chromium.or

[please use gerrit instead, 2014-10-25 00:22:55]

On 2014/10/25 00:21:49, Rouslan Solomakhin wrote:
> Garrett: OWNERS PTAL
> chrome/browser/password_manager/chrome_password_manager_client.cc.

... in Patch Set 37.

[please use gerrit instead, 2014-10-25 00:23:56]

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

[please use gerrit instead, 2014-10-25 00:23:56]

rouslan@chromium.org changed required reviewers:
+ jdduke@chromium.org

[please use gerrit instead, 2014-10-25 00:23:57]

Jared: OWNERS PTAL  ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java
in Patch Set 37.

[please use gerrit instead, 2014-10-25 00:25:27]

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

[please use gerrit instead, 2014-10-25 00:25:27]

rouslan@chromium.org changed required reviewers:
+ bauerb@chromium.org

[please use gerrit instead, 2014-10-25 00:25:28]

Bernhard, OWNERS PTAL chrome/browser/android/chrome_jni_registrar.cc in Patch
Set 37.

[aurimas (slooooooooow), 2014-10-25 00:28:21]

I'll take a look at the JNI code on Monday.

[jdduke (slow), 2014-10-25 00:29:19]

On 2014/10/25 00:23:57, Rouslan Solomakhin wrote:
> Jared: OWNERS PTAL 
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java
> in Patch Set 37.

ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java lgtm.

[Bernhard Bauer, 2014-10-27 10:13:02]

chrome/browser/android LGTM.

[Garrett Casto, 2014-10-27 22:48:59]

LGTM with nits

https://codereview.chromium.org/606153002/diff/800001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/606153002/diff/800001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:339: #if
defined(USE_AURA) || defined(OS_MACOSX) || defined(OS_ANDROID)
You should be able to remove these macros now. Linux no longer builds without
Aura (AFAIK) and iOS doesn't depend on this code path.

[please use gerrit instead, 2014-10-27 22:57:05]

https://codereview.chromium.org/606153002/diff/800001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/606153002/diff/800001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:339: #if
defined(USE_AURA) || defined(OS_MACOSX) || defined(OS_ANDROID)
On 2014/10/27 22:48:58, Garrett Casto wrote:
> You should be able to remove these macros now. Linux no longer builds without
> Aura (AFAIK) and iOS doesn't depend on this code path. 

Done.

[please use gerrit instead, 2014-10-27 23:46:12]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2014-10-27 23:49:40]

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

[commit-bot: I haz the power, 2014-10-28 00:59:09]

Committed patchset #38 (id:820001)

[commit-bot: I haz the power, 2014-10-28 00:59:38]

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