Pass all info to account chooser infobar.

Pass all info to  account chooser infobar.

Now account chooser infobar (which is one of the Smart Lock infobars) displaying only username. This patch adds ability to infobar to show full name and picture associated with account.

BUG=454815
Committed: https://crrev.com/71e08c4d22fb6035bf80b8a110ede3e3d840861b
Cr-Commit-Position: refs/heads/master@{#321099}

[melandory, 2015-02-24 17:17:39]

Patchset #1 (id:1) has been deleted

[melandory, 2015-02-24 20:09:08]

Patchset #3 (id:60001) has been deleted

[melandory, 2015-02-24 20:09:23]

Patchset #2 (id:40001) has been deleted

[melandory, 2015-02-24 20:17:19]

Patchset #2 (id:80001) has been deleted

[melandory, 2015-02-24 20:21:18]

Patchset #2 (id:100001) has been deleted

[melandory, 2015-02-24 20:28:45]

Patchset #2 (id:90009) has been deleted

[melandory, 2015-02-24 20:33:50]

Patchset #2 (id:130001) has been deleted

[melandory, 2015-02-24 20:38:51]

Patchset #2 (id:110010) has been deleted

[melandory, 2015-02-25 13:34:53]

Patchset #2 (id:160001) has been deleted

[melandory, 2015-02-25 13:35:04]

Patchset #2 (id:180001) has been deleted

[melandory, 2015-02-25 13:35:13]

Patchset #2 (id:200001) has been deleted

[melandory, 2015-02-25 13:35:25]

Patchset #2 (id:220001) has been deleted

[melandory, 2015-02-25 14:12:24]

melandory@chromium.org changed reviewers:
+ vabr@chromium.org

[melandory, 2015-02-25 14:12:25]

Hi Vaclav,

can you give me a pre-review. For now I'm interested in your opinion about
changes in:
components/autofill/core/common/password_form.cc
components/autofill/core/common/password_form.h

and in particular about is it a good idea to have code which is relevant only
for android there?

Thanks!

[vabr (Chromium), 2015-02-25 14:23:30]

On 2015/02/25 14:12:25, melandory wrote:
> Hi Vaclav,
> 
> can you give me a pre-review. For now I'm interested in your opinion about
> changes in:
> components/autofill/core/common/password_form.cc
> components/autofill/core/common/password_form.h
> 
> and in particular about is it a good idea to have code which is relevant only
> for android there?
> 
> Thanks!

Hi Tanja,

I'm just running off for some errands. I'll have a look ASAP, but possibly
tomorrow.

Sorry!
Vaclav

[melandory, 2015-02-25 14:33:02]

On 2015/02/25 14:23:30, vabr (Chromium) wrote:
> On 2015/02/25 14:12:25, melandory wrote:
> > Hi Vaclav,
> > 
> > can you give me a pre-review. For now I'm interested in your opinion about
> > changes in:
> > components/autofill/core/common/password_form.cc
> > components/autofill/core/common/password_form.h
> > 
> > and in particular about is it a good idea to have code which is relevant
only
> > for android there?
> > 
> > Thanks!
> 
> Hi Tanja,
> 
> I'm just running off for some errands. I'll have a look ASAP, but possibly
> tomorrow.
> 
> Sorry!
> Vaclav

No problem. Thanks for letting me know.

[vabr (Chromium), 2015-02-26 09:29:32]

Hi Tanja,

As requested, I did a pre-review, mostly looking only on PasswordForm, with one
other comment for the mocked password store.

Cheers,
Vaclav

https://codereview.chromium.org/925593006/diff/280001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/925593006/diff/280001/components/autofill/cor...
components/autofill/core/common/password_form.h:253:
base::android::ScopedJavaLocalRef<jobject> createNativeCredential(JNIEnv* env,
Note that PasswordForm is a struct, so all its data members are public. That
means it is easy to write non-member functions to use those data members,
without the need to add them as methods.

Because of the added functions being specific to Android, I suggest moving them
out of PasswordForm, and putting them somewhere near the code using these
functions. In any case, they should go to some android directory, into files
only compiled for Android. That would get rid of the #ifdefs, and keep
PasswordForm platform independent.

https://codereview.chromium.org/925593006/diff/280001/components/password_man...
File components/password_manager/core/browser/mock_password_store.h (left):

https://codereview.chromium.org/925593006/diff/280001/components/password_man...
components/password_manager/core/browser/mock_password_store.h:8: #include
"components/autofill/core/common/password_form.h"
Please do not remove the #include in favour of the forward declaration. A couple
of reasons not to:

(1) You don't need it for anything here, and it will decrease the size of your
CL by two files.
(2) In general, independent clean-ups should be in a separate CL to make the CLs
easier to review and revert if needed.
(3) The internal C++ style guide (can send you a link, or just search on the
intranet) says: "Avoid using forward declarations where possible. Just #include
the headers you need," while the external says: "You may forward declare
ordinary classes in order to avoid unnecessary #includes" (note "may", not
"must").

[melandory, 2015-02-26 12:05:54]

Patchset #5 (id:300001) has been deleted

[melandory, 2015-02-26 12:06:05]

Patchset #2 (id:240001) has been deleted

[melandory, 2015-02-26 12:06:25]

Patchset #1 (id:20001) has been deleted

[melandory, 2015-02-26 12:06:36]

Patchset #1 (id:260001) has been deleted

[melandory, 2015-02-26 12:29:26]

Patchset #2 (id:320001) has been deleted

[melandory, 2015-02-26 14:12:36]

Patchset #2 (id:340001) has been deleted

[melandory, 2015-02-26 14:12:46]

Patchset #3 (id:380001) has been deleted

[melandory, 2015-02-26 15:50:34]

melandory@chromium.org changed reviewers:
+ jochen@chromium.org, miguelg@chromium.org

[melandory, 2015-02-26 15:50:35]

Hi all,

please review this CL.

vabr@:
chrome/browser/password_manager/account_chooser_infobar_delegate_android.h
components/autofill/core/common/BUILD.gn
components/autofill/core/common/android/BUILD.gn
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
components/autofill/core/common/android/password_form_android_utils.h
components/autofill/core/common/android/password_form_android_utils.cc

miguelg@:
chrome/android/BUILD.gn
chrome/android/java/DEPS
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java
chrome/browser/ui/android/infobars/account_chooser_infobar.cc

jochen@:
chrome/chrome.gyp
components/autofill.gypi

[Miguel Garcia, 2015-02-26 15:57:54]

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

[Miguel Garcia, 2015-02-26 15:58:08]

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

[Miguel Garcia, 2015-02-26 15:58:09]

Adding Ted for the android pieces

[melandory, 2015-02-26 16:37:11]

On 2015/02/26 15:58:09, Miguel Garcia wrote:
> Adding Ted for the android pieces

If you and Ted don't mind I'll change Ted to newt@, who was reviewing everything
about AccountChooserInfoBar.java, so he is more familiar with this code.
My only motivation for not adding him on a first place was not to having US
reviewers due different time zone =)

[vabr (Chromium), 2015-02-26 16:44:24]

On 2015/02/26 16:37:11, melandory wrote:
> On 2015/02/26 15:58:09, Miguel Garcia wrote:
> > Adding Ted for the android pieces
> 
> If you and Ted don't mind I'll change Ted to newt@, who was reviewing
everything
> about AccountChooserInfoBar.java, so he is more familiar with this code.
> My only motivation for not adding him on a first place was not to having US
> reviewers due different time zone =)

Tanja,

I'm really sorry, but I'm OOO until Monday. I'm afraid I won't be able to review
until then.

Cheers,
Vaclav

[Ted C, 2015-02-26 17:27:45]

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

[Ted C, 2015-02-26 17:27:45]

More than happy to not sign up for more code reviews.  Dropping myself and
adding newt@ instead.

[jochen (gone - plz use gerrit), 2015-02-27 09:04:14]

lgtm

[vabr (Chromium), 2015-03-03 13:00:37]

Hi Tanja,

LGTM for
chrome/browser/password_manager/account_chooser_infobar_delegate_android.h.

LGTM with comments for 
components/autofill/core/common/android/password_form_android_utils.h
components/autofill/core/common/android/password_form_android_utils.cc

Please have newt@ review
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
for Java, and isherman@ or estade@ for the placement of that file in the
autofill component directory hierarchy. My ownership is mainly just for password
manager files, not for changing the directory structure.

While changes in
components/autofill/core/common/BUILD.gn
components/autofill/core/common/android/BUILD.gn
LGTM, you should also get a rubber-stamp from isherman@ or estade@ for those
changes, if one of them will already be looking at adding Credential.java.

Cheers,
Vaclav

https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
File components/autofill/core/common/BUILD.gn (right):

https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
components/autofill/core/common/BUILD.gn:33:
"android/password_form_android_utils.cc",
Reading https://code.google.com/p/chromium/wiki/GNStyleGuide, I believe the
suggested way would be to create a source_set target in the BUILD.gn in the
android subdir, containing the new two files, on which the target here would
depend.

https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
components/autofill/core/common/BUILD.gn:46: deps += [ ":autofill_jni_headers" ]
I don't understand what the removed TODO was about.
Can you tell me?

https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
File components/autofill/core/common/android/password_form_android_utils.h
(right):

https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
components/autofill/core/common/android/password_form_android_utils.h:19: //
Creates Java counterpart of PasswordForm, assigning it a |position| in case
nit: Please move this description up so that it applies to the above version of
CreateNativeCredential as well. Please mention how does the first version deal
with the lack of |position|.

https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
components/autofill/core/common/android/password_form_android_utils.h:28:
base::android::ScopedJavaLocalRef<jobjectArray> CreateNativeCredentialArray(
Please also add comments for this and the following function.

[melandory, 2015-03-05 10:39:42]

melandory@chromium.org changed reviewers:
+ isherman@chromium.org

[melandory, 2015-03-05 10:41:32]

On 2015/03/03 13:00:37, vabr (Chromium) wrote:
> Hi Tanja,
> 
> LGTM for
> chrome/browser/password_manager/account_chooser_infobar_delegate_android.h.
> 
> LGTM with comments for 
> components/autofill/core/common/android/password_form_android_utils.h
> components/autofill/core/common/android/password_form_android_utils.cc
> 
> Please have newt@ review
>
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
> for Java, and isherman@ or estade@ for the placement of that file in the
> autofill component directory hierarchy. My ownership is mainly just for
password
> manager files, not for changing the directory structure.
> 
> While changes in
> components/autofill/core/common/BUILD.gn
> components/autofill/core/common/android/BUILD.gn
> LGTM, you should also get a rubber-stamp from isherman@ or estade@ for those
> changes, if one of them will already be looking at adding Credential.java.
> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
> File components/autofill/core/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
> components/autofill/core/common/BUILD.gn:33:
> "android/password_form_android_utils.cc",
> Reading https://code.google.com/p/chromium/wiki/GNStyleGuide, I believe the
> suggested way would be to create a source_set target in the BUILD.gn in the
> android subdir, containing the new two files, on which the target here would
> depend.
> 
>
https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
> components/autofill/core/common/BUILD.gn:46: deps += [ ":autofill_jni_headers"
]
> I don't understand what the removed TODO was about.
> Can you tell me?
> 
>
https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
> File components/autofill/core/common/android/password_form_android_utils.h
> (right):
> 
>
https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
> components/autofill/core/common/android/password_form_android_utils.h:19: //
> Creates Java counterpart of PasswordForm, assigning it a |position| in case
> nit: Please move this description up so that it applies to the above version
of
> CreateNativeCredential as well. Please mention how does the first version deal
> with the lack of |position|.
> 
>
https://codereview.chromium.org/925593006/diff/400001/components/autofill/cor...
> components/autofill/core/common/android/password_form_android_utils.h:28:
> base::android::ScopedJavaLocalRef<jobjectArray> CreateNativeCredentialArray(
> Please also add comments for this and the following function.
Hey,

isherman@ can you please review

components/autofill/core/common/BUILD.gn
components/autofill/core/common/android/BUILD.gn

and also comment on placing
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
in the autofill component directory hierarchy.

[melandory, 2015-03-05 12:26:08]

Patchset #4 (id:420001) has been deleted

[melandory, 2015-03-05 12:33:17]

Patchset #4 (id:440001) has been deleted

[melandory, 2015-03-05 14:47:44]

Patchset #4 (id:460001) has been deleted

[Ilya Sherman, 2015-03-05 22:54:29]

On 2015/03/05 10:41:32, melandory wrote:
> Hey,
> 
> isherman@ can you please review
> 
> components/autofill/core/common/BUILD.gn
> components/autofill/core/common/android/BUILD.gn
> 
> and also comment on placing
>
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
> in the autofill component directory hierarchy.

A few high-level questions and comments:
(1) Please update the CL description to be more detailed.  Ideally, someone
should be able to glance at the description and quickly tell roughly what the CL
is doing.  Right now, I need to click through to the bug for additional context.
(2) Why are you adding this code to the autofill component, rather than to one
of the passwords-related components?  It seems like this is more oriented toward
passwords than autofill.
(3) Why are you adding it to the /core/common directory?  The common directory
is for code that's shared between the browser and the renderer processes.  I
assume that's not the case here... right?  My gut instinct is to put this code
under //components/foo/android/bar/baz/qux... but:
(4) Is there a common precedent for having Java files live within the
//components directory?  What style tends to be used there?

[Ilya Sherman, 2015-03-05 23:53:05]

Sorry, forgot to mail an inline comment:

https://codereview.chromium.org/925593006/diff/480001/components/autofill/cor...
File components/autofill/core/common/BUILD.gn (right):

https://codereview.chromium.org/925593006/diff/480001/components/autofill/cor...
components/autofill/core/common/BUILD.gn:75:
"../common/android/java/src/org/chromium/components/autofill/core/common/Credential.java",
nit: Why "../common"?   You're already in the "common" directory...

[melandory, 2015-03-09 22:08:07]

On 2015/03/05 22:54:29, Ilya Sherman wrote:
> On 2015/03/05 10:41:32, melandory wrote:
> > Hey,
> > 
> > isherman@ can you please review
> > 
> > components/autofill/core/common/BUILD.gn
> > components/autofill/core/common/android/BUILD.gn
> > 
> > and also comment on placing
> >
>
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
> > in the autofill component directory hierarchy.
> 
> A few high-level questions and comments:
> (1) Please update the CL description to be more detailed.  Ideally, someone
> should be able to glance at the description and quickly tell roughly what the
CL
> is doing.  Right now, I need to click through to the bug for additional
context.
updated
> (2) Why are you adding this code to the autofill component, rather than to one
> of the passwords-related components?  It seems like this is more oriented
toward
> passwords than autofill.
My motivation was following: since Credential is java counter part of password
form, they
should be in same dir structure.
> (3) Why are you adding it to the /core/common directory?  The common directory
> is for code that's shared between the browser and the renderer processes.  I
> assume that's not the case here... right?  My gut instinct is to put this code
> under //components/foo/android/bar/baz/qux... but:
> (4) Is there a common precedent for having Java files live within the
> //components directory?  What style tends to be used there?
Yes, for example:
src/components/variations/android/java/src/org/chromium/components/variations/
src/components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/

[Ilya Sherman, 2015-03-09 22:53:01]

On 2015/03/09 22:08:07, melandory wrote:
> On 2015/03/05 22:54:29, Ilya Sherman wrote:
> > On 2015/03/05 10:41:32, melandory wrote:
> > > Hey,
> > > 
> > > isherman@ can you please review
> > > 
> > > components/autofill/core/common/BUILD.gn
> > > components/autofill/core/common/android/BUILD.gn
> > > 
> > > and also comment on placing
> > >
> >
>
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
> > > in the autofill component directory hierarchy.
> > 
> > A few high-level questions and comments:
> > (1) Please update the CL description to be more detailed.  Ideally, someone
> > should be able to glance at the description and quickly tell roughly what
the
> CL
> > is doing.  Right now, I need to click through to the bug for additional
> context.
> updated
> > (2) Why are you adding this code to the autofill component, rather than to
one
> > of the passwords-related components?  It seems like this is more oriented
> toward
> > passwords than autofill.
> My motivation was following: since Credential is java counter part of password
> form, they
> should be in same dir structure.

That's understandable.  However, the password form files are in the autofill
component for legacy reasons.  It's best to add new files in the most closely
related component, if possible... and it does seem to be possible in this case.

> > (3) Why are you adding it to the /core/common directory?  The common
directory
> > is for code that's shared between the browser and the renderer processes.  I
> > assume that's not the case here... right?  My gut instinct is to put this
code
> > under //components/foo/android/bar/baz/qux... but:
> > (4) Is there a common precedent for having Java files live within the
> > //components directory?  What style tends to be used there?
> Yes, for example:
> src/components/variations/android/java/src/org/chromium/components/variations/
>
src/components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/

Okay, so it looks like that matches my suggestion #3.

[melandory, 2015-03-09 22:54:06]

On 2015/03/09 22:53:01, Ilya Sherman wrote:
> On 2015/03/09 22:08:07, melandory wrote:
> > On 2015/03/05 22:54:29, Ilya Sherman wrote:
> > > On 2015/03/05 10:41:32, melandory wrote:
> > > > Hey,
> > > > 
> > > > isherman@ can you please review
> > > > 
> > > > components/autofill/core/common/BUILD.gn
> > > > components/autofill/core/common/android/BUILD.gn
> > > > 
> > > > and also comment on placing
> > > >
> > >
> >
>
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
> > > > in the autofill component directory hierarchy.
> > > 
> > > A few high-level questions and comments:
> > > (1) Please update the CL description to be more detailed.  Ideally,
someone
> > > should be able to glance at the description and quickly tell roughly what
> the
> > CL
> > > is doing.  Right now, I need to click through to the bug for additional
> > context.
> > updated
> > > (2) Why are you adding this code to the autofill component, rather than to
> one
> > > of the passwords-related components?  It seems like this is more oriented
> > toward
> > > passwords than autofill.
> > My motivation was following: since Credential is java counter part of
password
> > form, they
> > should be in same dir structure.
> 
> That's understandable.  However, the password form files are in the autofill
> component for legacy reasons.  It's best to add new files in the most closely
> related component, if possible... and it does seem to be possible in this
case.
> 
> > > (3) Why are you adding it to the /core/common directory?  The common
> directory
> > > is for code that's shared between the browser and the renderer processes. 
I
> > > assume that's not the case here... right?  My gut instinct is to put this
> code
> > > under //components/foo/android/bar/baz/qux... but:
> > > (4) Is there a common precedent for having Java files live within the
> > > //components directory?  What style tends to be used there?
> > Yes, for example:
> >
src/components/variations/android/java/src/org/chromium/components/variations/
> >
>
src/components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/
> 
> Okay, so it looks like that matches my suggestion #3.

Yes. Will fix it tomorrow. Thanks for looking into it.

[melandory, 2015-03-10 17:04:06]

On 2015/03/09 22:54:06, melandory wrote:
> On 2015/03/09 22:53:01, Ilya Sherman wrote:
> > On 2015/03/09 22:08:07, melandory wrote:
> > > On 2015/03/05 22:54:29, Ilya Sherman wrote:
> > > > On 2015/03/05 10:41:32, melandory wrote:
> > > > > Hey,
> > > > > 
> > > > > isherman@ can you please review
> > > > > 
> > > > > components/autofill/core/common/BUILD.gn
> > > > > components/autofill/core/common/android/BUILD.gn
> > > > > 
> > > > > and also comment on placing
> > > > >
> > > >
> > >
> >
>
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
> > > > > in the autofill component directory hierarchy.
> > > > 
> > > > A few high-level questions and comments:
> > > > (1) Please update the CL description to be more detailed.  Ideally,
> someone
> > > > should be able to glance at the description and quickly tell roughly
what
> > the
> > > CL
> > > > is doing.  Right now, I need to click through to the bug for additional
> > > context.
> > > updated
> > > > (2) Why are you adding this code to the autofill component, rather than
to
> > one
> > > > of the passwords-related components?  It seems like this is more
oriented
> > > toward
> > > > passwords than autofill.
> > > My motivation was following: since Credential is java counter part of
> password
> > > form, they
> > > should be in same dir structure.
> > 
> > That's understandable.  However, the password form files are in the autofill
> > component for legacy reasons.  It's best to add new files in the most
closely
> > related component, if possible... and it does seem to be possible in this
> case.
> > 
> > > > (3) Why are you adding it to the /core/common directory?  The common
> > directory
> > > > is for code that's shared between the browser and the renderer
processes. 
> I
> > > > assume that's not the case here... right?  My gut instinct is to put
this
> > code
> > > > under //components/foo/android/bar/baz/qux... but:
> > > > (4) Is there a common precedent for having Java files live within the
> > > > //components directory?  What style tends to be used there?
> > > Yes, for example:
> > >
> src/components/variations/android/java/src/org/chromium/components/variations/
> > >
> >
>
src/components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/
> > 
> > Okay, so it looks like that matches my suggestion #3.
What about placing android code under chrome/browser/password_manager instead of
components? Any objection?

[vabr (Chromium), 2015-03-10 18:39:14]

On 2015/03/10 17:04:06, melandory wrote:
> On 2015/03/09 22:54:06, melandory wrote:
> > On 2015/03/09 22:53:01, Ilya Sherman wrote:
> > > On 2015/03/09 22:08:07, melandory wrote:
> > > > On 2015/03/05 22:54:29, Ilya Sherman wrote:
> > > > > On 2015/03/05 10:41:32, melandory wrote:
> > > > > > Hey,
> > > > > > 
> > > > > > isherman@ can you please review
> > > > > > 
> > > > > > components/autofill/core/common/BUILD.gn
> > > > > > components/autofill/core/common/android/BUILD.gn
> > > > > > 
> > > > > > and also comment on placing
> > > > > >
> > > > >
> > > >
> > >
> >
>
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
> > > > > > in the autofill component directory hierarchy.
> > > > > 
> > > > > A few high-level questions and comments:
> > > > > (1) Please update the CL description to be more detailed.  Ideally,
> > someone
> > > > > should be able to glance at the description and quickly tell roughly
> what
> > > the
> > > > CL
> > > > > is doing.  Right now, I need to click through to the bug for
additional
> > > > context.
> > > > updated
> > > > > (2) Why are you adding this code to the autofill component, rather
than
> to
> > > one
> > > > > of the passwords-related components?  It seems like this is more
> oriented
> > > > toward
> > > > > passwords than autofill.
> > > > My motivation was following: since Credential is java counter part of
> > password
> > > > form, they
> > > > should be in same dir structure.
> > > 
> > > That's understandable.  However, the password form files are in the
autofill
> > > component for legacy reasons.  It's best to add new files in the most
> closely
> > > related component, if possible... and it does seem to be possible in this
> > case.
> > > 
> > > > > (3) Why are you adding it to the /core/common directory?  The common
> > > directory
> > > > > is for code that's shared between the browser and the renderer
> processes. 
> > I
> > > > > assume that's not the case here... right?  My gut instinct is to put
> this
> > > code
> > > > > under //components/foo/android/bar/baz/qux... but:
> > > > > (4) Is there a common precedent for having Java files live within the
> > > > > //components directory?  What style tends to be used there?
> > > > Yes, for example:
> > > >
> >
src/components/variations/android/java/src/org/chromium/components/variations/
> > > >
> > >
> >
>
src/components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/
> > > 
> > > Okay, so it looks like that matches my suggestion #3.
> What about placing android code under chrome/browser/password_manager instead
of
> components? Any objection?

No objections.

Cheers,
Vaclav

[melandory, 2015-03-11 08:39:03]

Patchset #5 (id:500001) has been deleted

[melandory, 2015-03-11 15:29:46]

Patchset #5 (id:520001) has been deleted

[melandory, 2015-03-11 15:32:39]

Patchset #2 (id:360001) has been deleted

[melandory, 2015-03-11 23:58:50]

Patchset #6 (id:580001) has been deleted

[melandory, 2015-03-11 23:59:05]

Patchset #5 (id:560001) has been deleted

[melandory, 2015-03-11 23:59:16]

Patchset #4 (id:540001) has been deleted

[melandory, 2015-03-12 20:30:06]

On 2015/03/10 18:39:14, vabr (Chromium) wrote:
> On 2015/03/10 17:04:06, melandory wrote:
> > On 2015/03/09 22:54:06, melandory wrote:
> > > On 2015/03/09 22:53:01, Ilya Sherman wrote:
> > > > On 2015/03/09 22:08:07, melandory wrote:
> > > > > On 2015/03/05 22:54:29, Ilya Sherman wrote:
> > > > > > On 2015/03/05 10:41:32, melandory wrote:
> > > > > > > Hey,
> > > > > > > 
> > > > > > > isherman@ can you please review
> > > > > > > 
> > > > > > > components/autofill/core/common/BUILD.gn
> > > > > > > components/autofill/core/common/android/BUILD.gn
> > > > > > > 
> > > > > > > and also comment on placing
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
components/autofill/core/common/android/java/src/org/chromium/components/autofill/core/common/Credential.java
> > > > > > > in the autofill component directory hierarchy.
> > > > > > 
> > > > > > A few high-level questions and comments:
> > > > > > (1) Please update the CL description to be more detailed.  Ideally,
> > > someone
> > > > > > should be able to glance at the description and quickly tell roughly
> > what
> > > > the
> > > > > CL
> > > > > > is doing.  Right now, I need to click through to the bug for
> additional
> > > > > context.
> > > > > updated
> > > > > > (2) Why are you adding this code to the autofill component, rather
> than
> > to
> > > > one
> > > > > > of the passwords-related components?  It seems like this is more
> > oriented
> > > > > toward
> > > > > > passwords than autofill.
> > > > > My motivation was following: since Credential is java counter part of
> > > password
> > > > > form, they
> > > > > should be in same dir structure.
> > > > 
> > > > That's understandable.  However, the password form files are in the
> autofill
> > > > component for legacy reasons.  It's best to add new files in the most
> > closely
> > > > related component, if possible... and it does seem to be possible in
this
> > > case.
> > > > 
> > > > > > (3) Why are you adding it to the /core/common directory?  The common
> > > > directory
> > > > > > is for code that's shared between the browser and the renderer
> > processes. 
> > > I
> > > > > > assume that's not the case here... right?  My gut instinct is to put
> > this
> > > > code
> > > > > > under //components/foo/android/bar/baz/qux... but:
> > > > > > (4) Is there a common precedent for having Java files live within
the
> > > > > > //components directory?  What style tends to be used there?
> > > > > Yes, for example:
> > > > >
> > >
> src/components/variations/android/java/src/org/chromium/components/variations/
> > > > >
> > > >
> > >
> >
>
src/components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/
> > > > 
> > > > Okay, so it looks like that matches my suggestion #3.
> > What about placing android code under chrome/browser/password_manager
instead
> of
> > components? Any objection?
> 
> No objections.
> 
> Cheers,
> Vaclav

vabr@ and isherman@ can you look at new location of file?

newt@ can you look at:
chrome/android/BUILD.gn
chrome/android/java/DEPS
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java
chrome/browser/ui/android/infobars/account_chooser_infobar.cc
chrome/android/java/src/org/chromium/chrome/browser/password_manager/Credential.java

[Ilya Sherman, 2015-03-13 01:06:10]

> vabr@ and isherman@ can you look at new location of file?

The new location seems fine to me, but I am not especially familiar with how
Android source code is typically organized.  I defer to OWNERS of the relevant
directory.

Looks like you don't need my review anymore, so I'll bow out.  Feel free to
re-add me if need be.

[Ilya Sherman, 2015-03-13 01:06:26]

isherman@chromium.org changed reviewers:
- isherman@chromium.org

[vabr (Chromium), 2015-03-13 08:46:06]

Moving account_chooser_infobar_delegate_android.h and
password_form_android_utils.h into //chrome/browser/password_manager LGTM,
because since https://codereview.chromium.org/861103002 we already have such
code in there. I'm not sure if that's good or bad, but if we need to fix this,
we'll need to move files beyond the scope of this CL. I'll follow up on that
outside of this CL.

Cheers!
Vaclav

[newt (away), 2015-03-17 06:04:18]

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java
(right):

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:108:
final int currentCredentialIndex = credential.getIndex();
You can use the local variable "position" here, instead of
credential.getIndex(). Then you can also remove the "mIndex" field from
Credential altogether.

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:132:
nativeOnCredentialClicked(mNativeInfoBarPtr, credentialIndex, credentialType);
I'd inline this method. No need for the extra indirection here.

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/password_manager/Credential.java
(right):

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/password_manager/Credential.java:13:
public class Credential {
The convention is to name the C++ counterpart with the same name as the Java
class (or as similar as possible). Otherwise, it becomes difficult to find the
counterpart. In this case, the C++ file could be named credential.cc or
credential_android.cc, rather than password_form_android_utils.cc.

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/password_manager/Credential.java:20:
* @param username username which corresponds to the credential.
These parameters should be defined more precisely. Reading this I'm not sure if
username is an email address, or an opaque GUID, or what. Worse, I don't know
where to look for that information.

A brief explanation of what form the username takes and/or where it's defined
(e.g. in password_manager.h) would be helpful.

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/password_manager/Credential.java:22:
* @param type type which should be either local or federated.
Since "type" is an int, you need to explain what its values correspond to (is it
a C++ enum or list of constants in Java?). Otherwise, the reader won't know how
to interpret "type".

https://codereview.chromium.org/925593006/diff/620001/chrome/browser/password...
File chrome/browser/password_manager/password_form_android_utils.h (right):

https://codereview.chromium.org/925593006/diff/620001/chrome/browser/password...
chrome/browser/password_manager/password_form_android_utils.h:15:
base::android::ScopedJavaLocalRef<jobject> CreateNativeCredential(
I'd avoid overloading this method unless it's really useful to have both
versions (especially when the parameter types make it hard to infer which
argument is which). In this case, this method doesn't seem to be used, so I'd
just remove it.

https://codereview.chromium.org/925593006/diff/620001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/account_chooser_infobar.cc (right):

https://codereview.chromium.org/925593006/diff/620001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/account_chooser_infobar.cc:21: for (auto
password_form : credentials_forms) {
nit: be consistent with your naming. Either would be OK:

    for (auto password_form : password_forms) {

or

    for (auto credentials_form : credentials_forms) {

[melandory, 2015-03-17 13:05:33]

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java
(right):

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:108:
final int currentCredentialIndex = credential.getIndex();
On 2015/03/17 06:04:17, newt wrote:
> You can use the local variable "position" here, instead of
> credential.getIndex(). Then you can also remove the "mIndex" field from
> Credential altogether.

Nope, |position| here is an index in array array of both local + federated
credentials.
credential.getIndex() returns position in array of federated credentials or
position in array of local credentials.

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:132:
nativeOnCredentialClicked(mNativeInfoBarPtr, credentialIndex, credentialType);
On 2015/03/17 06:04:17, newt wrote:
> I'd inline this method. No need for the extra indirection here. 

Done.

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/password_manager/Credential.java
(right):

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/password_manager/Credential.java:13:
public class Credential {
On 2015/03/17 06:04:17, newt wrote:
> The convention is to name the C++ counterpart with the same name as the Java
> class (or as similar as possible). Otherwise, it becomes difficult to find the
> counterpart. In this case, the C++ file could be named credential.cc or
> credential_android.cc, rather than password_form_android_utils.cc.

Done: prefer changing c++ file name.

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/password_manager/Credential.java:20:
* @param username username which corresponds to the credential.
On 2015/03/17 06:04:18, newt wrote:
> These parameters should be defined more precisely. Reading this I'm not sure
if
> username is an email address, or an opaque GUID, or what. Worse, I don't know
> where to look for that information.
> 
> A brief explanation of what form the username takes and/or where it's defined
> (e.g. in password_manager.h) would be helpful.

Done.

https://codereview.chromium.org/925593006/diff/620001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/password_manager/Credential.java:22:
* @param type type which should be either local or federated.
On 2015/03/17 06:04:17, newt wrote:
> Since "type" is an int, you need to explain what its values correspond to (is
it
> a C++ enum or list of constants in Java?). Otherwise, the reader won't know
how
> to interpret "type".

Done.

https://codereview.chromium.org/925593006/diff/620001/chrome/browser/password...
File chrome/browser/password_manager/password_form_android_utils.h (right):

https://codereview.chromium.org/925593006/diff/620001/chrome/browser/password...
chrome/browser/password_manager/password_form_android_utils.h:15:
base::android::ScopedJavaLocalRef<jobject> CreateNativeCredential(
On 2015/03/17 06:04:18, newt wrote:
> I'd avoid overloading this method unless it's really useful to have both
> versions (especially when the parameter types make it hard to infer which
> argument is which). In this case, this method doesn't seem to be used, so I'd
> just remove it.

I'm planing to use this method in a future when I'll implement auto sign in
toast. But I remove it for now.

https://codereview.chromium.org/925593006/diff/620001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/account_chooser_infobar.cc (right):

https://codereview.chromium.org/925593006/diff/620001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/account_chooser_infobar.cc:21: for (auto
password_form : credentials_forms) {
On 2015/03/17 06:04:18, newt wrote:
> nit: be consistent with your naming. Either would be OK:
> 
>     for (auto password_form : password_forms) {
> 
> or
> 
>     for (auto credentials_form : credentials_forms) {

Done.

[melandory, 2015-03-17 14:58:12]

Patchset #7 (id:660001) has been deleted

[newt (away), 2015-03-17 17:22:06]

https://codereview.chromium.org/925593006/diff/700001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java
(right):

https://codereview.chromium.org/925593006/diff/700001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:132:
nativeOnCredentialClicked(mNativeInfoBarPtr, credentialIndex, credentialType);
inline this

https://codereview.chromium.org/925593006/diff/700001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/account_chooser_infobar.cc (right):

https://codereview.chromium.org/925593006/diff/700001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/account_chooser_infobar.cc:21: int index =
indexStart;
Do you still need the index parameter, now that index and position are the same
value?

[melandory, 2015-03-17 20:46:22]

Patchset #6 (id:640001) has been deleted

[melandory, 2015-03-17 20:46:37]

Patchset #6 (id:680001) has been deleted

[melandory, 2015-03-17 20:47:46]

https://codereview.chromium.org/925593006/diff/700001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java
(right):

https://codereview.chromium.org/925593006/diff/700001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:132:
nativeOnCredentialClicked(mNativeInfoBarPtr, credentialIndex, credentialType);
On 2015/03/17 17:22:06, newt wrote:
> inline this

Done.

https://codereview.chromium.org/925593006/diff/700001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/account_chooser_infobar.cc (right):

https://codereview.chromium.org/925593006/diff/700001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/account_chooser_infobar.cc:21: int index =
indexStart;
On 2015/03/17 17:22:06, newt wrote:
> Do you still need the index parameter, now that index and position are the
same
> value?

Oh, my bad. They shouldn't be the same. Credential should be created with
index-indexStart

[newt (away), 2015-03-17 21:05:40]

lgtm

[melandory, 2015-03-18 09:19:29]

The CQ bit was checked by melandory@chromium.org

[melandory, 2015-03-18 09:19:30]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/925593006/#ps720001
(title: " ")

[commit-bot: I haz the power, 2015-03-18 09:19:47]

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

[commit-bot: I haz the power, 2015-03-18 10:15:50]

Committed patchset #7 (id:720001)

[commit-bot: I haz the power, 2015-03-18 10:16:26]

Patchset 7 (id:??) landed as
https://crrev.com/71e08c4d22fb6035bf80b8a110ede3e3d840861b
Cr-Commit-Position: refs/heads/master@{#321099}