Focus should be moved to the next input field after we generate the password.

Focus should be moved to the next input field after we generate the password.


BUG=131390
TEST=PasswordGenerationManagerTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145007

[Ilya Sherman, 2012-06-28 00:08:52]

Can you add some tests?  Other than that, LGTM.

[Garrett Casto, 2012-06-28 21:27:13]

http://codereview.chromium.org/10705006/diff/2001/chrome/renderer/autofill/pa...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10705006/diff/2001/chrome/renderer/autofill/pa...
chrome/renderer/autofill/password_generation_manager.cc:194: // Advance focus to
the input field next to the two password fields.
This isn't guaranteed to be 2 password fields in the feature. Can you just put
this in the above loop so that this gets called the correct number of times when
we start changing the heuristics.

Also, we may want to note that this assumes that the passwords fields are
adjacent. I haven't seen a case where this isn't true yet, so it's probably a
reasonable assumption.

[zysxqn, 2012-06-28 22:57:02]

On 2012/06/28 00:08:52, Ilya Sherman wrote:
> Can you add some tests?  Other than that, LGTM.

Hmm, in the browser test the focus is still on the first password field. My
guess is that the advanceFocus() API is only checked in a few days ago and they
haven't made it work in the testing framework somehow. I couldn't think of any
other reason since this is a pretty straight forward change and the actual
chrome browser does advance the focus correctly. I currently add a TODO in the
test and plan to file a bug on this. Ilya, are you fine with this or you have
more thoughts on this?

[zysxqn, 2012-06-28 22:57:16]

http://codereview.chromium.org/10705006/diff/2001/chrome/renderer/autofill/pa...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10705006/diff/2001/chrome/renderer/autofill/pa...
chrome/renderer/autofill/password_generation_manager.cc:194: // Advance focus to
the input field next to the two password fields.
On 2012/06/28 21:27:13, Garrett Casto wrote:
> This isn't guaranteed to be 2 password fields in the feature. Can you just put
> this in the above loop so that this gets called the correct number of times
when
> we start changing the heuristics.
> 
> Also, we may want to note that this assumes that the passwords fields are
> adjacent. I haven't seen a case where this isn't true yet, so it's probably a
> reasonable assumption.

Done.

[Ilya Sherman, 2012-06-28 23:03:54]

On 2012/06/28 22:57:02, zysxqn wrote:
> On 2012/06/28 00:08:52, Ilya Sherman wrote:
> > Can you add some tests?  Other than that, LGTM.
> 
> Hmm, in the browser test the focus is still on the first password field. My
> guess is that the advanceFocus() API is only checked in a few days ago and
they
> haven't made it work in the testing framework somehow. I couldn't think of any
> other reason since this is a pretty straight forward change and the actual
> chrome browser does advance the focus correctly. I currently add a TODO in the
> test and plan to file a bug on this. Ilya, are you fine with this or you have
> more thoughts on this?

That's fine, but please file the bug prior to committing, and add a link to the
bug (http://crbug.com/####) in the TODO comment.

[zysxqn, 2012-06-28 23:34:38]

+fsamuel for the advanceFocus() problem in the browser test.

[zysxqn, 2012-06-28 23:35:06]

On 2012/06/28 23:03:54, Ilya Sherman wrote:
> On 2012/06/28 22:57:02, zysxqn wrote:
> > On 2012/06/28 00:08:52, Ilya Sherman wrote:
> > > Can you add some tests?  Other than that, LGTM.
> > 
> > Hmm, in the browser test the focus is still on the first password field. My
> > guess is that the advanceFocus() API is only checked in a few days ago and
> they
> > haven't made it work in the testing framework somehow. I couldn't think of
any
> > other reason since this is a pretty straight forward change and the actual
> > chrome browser does advance the focus correctly. I currently add a TODO in
the
> > test and plan to file a bug on this. Ilya, are you fine with this or you
have
> > more thoughts on this?
> 
> That's fine, but please file the bug prior to committing, and add a link to
the
> bug (http://crbug.com/####) in the TODO comment.

Done.

[zysxqn, 2012-06-29 18:32:39]

Submitting... And I think I can use a separate CL to fix the advanceFocus()
test once we figure out the issue.


On Thu, Jun 28, 2012 at 4:35 PM, <zysxqn@google.com> wrote:

> On 2012/06/28 23:03:54, Ilya Sherman wrote:
>
>> On 2012/06/28 22:57:02, zysxqn wrote:
>> > On 2012/06/28 00:08:52, Ilya Sherman wrote:
>> > > Can you add some tests?  Other than that, LGTM.
>> >
>> > Hmm, in the browser test the focus is still on the first password
>> field. My
>> > guess is that the advanceFocus() API is only checked in a few days ago
>> and
>> they
>> > haven't made it work in the testing framework somehow. I couldn't think
>> of
>>
> any
>
>> > other reason since this is a pretty straight forward change and the
>> actual
>> > chrome browser does advance the focus correctly. I currently add a TODO
>> in
>>
> the
>
>> > test and plan to file a bug on this. Ilya, are you fine with this or you
>>
> have
>
>> > more thoughts on this?
>>
>
>  That's fine, but please file the bug prior to committing, and add a link
>> to
>>
> the
>
>> bug (http://crbug.com/####) in the TODO comment.
>>
>
> Done.
>
>
http://codereview.chromium.**org/10705006/<http://codereview.chromium.org/107...
>

[commit-bot: I haz the power, 2012-06-29 18:40:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/10705006/12003

[commit-bot: I haz the power, 2012-06-29 19:27:08]

Try job failure for 10705006-12003 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-29 21:12:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/10705006/7006

[commit-bot: I haz the power, 2012-06-29 22:08:43]

Change committed as 145007