Fixed handling of preferred login info in password manager.

Fixed handling of preferred login info in password manager.

BUG=23850
TEST=Go to a login page (i.e. gmail), log in with several different user accounts. Observe that the last used account is now the one that is used for the initial page autofill.


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

[zel, 2009-11-24 02:50:13]

This CL fixes handling of preferred login info in password manager. Previously,
we ended up with more than one preferred login info that resulted in the very
first one always being selected during initial page autofill.

[Ben Goodger (Google), 2009-11-24 23:49:08]

Ping tim.

-Ben

On Mon, Nov 23, 2009 at 6:50 PM,  <zelidrag@chromium.org> wrote:
> Reviewers: timsteele, Ben Goodger,
>
> Message:
> This CL fixes handling of preferred login info in password manager.
> Previously,
> we ended up with more than one preferred login info that resulted in the
> very
> first one always being selected during initial page autofill.
>
> Description:
> Fixed handling of preferred login info in password manager.
>
> BUG=23850
> TEST=Go to a login page (i.e. gmail), log in with several different user
> accounts. Observe that the last used account is now the one that is used for
> the
> initial page autofill.
>
> Please review this at http://codereview.chromium.org/437032
>
> Affected files:
>  M chrome/browser/password_manager/password_form_manager.h
>  M chrome/browser/password_manager/password_form_manager.cc
>
>
>

[tim (not reviewing), 2009-11-25 01:16:55]

LGTM

http://codereview.chromium.org/437032/diff/1/2
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/437032/diff/1/2#newcode323
chrome/browser/password_manager/password_form_manager.cc:323: if
(!password_store) {
nit - this check is redundant given the callsites, you could just swap for a
DCHECK... but not a big deal.

http://codereview.chromium.org/437032/diff/1/2#newcode330
chrome/browser/password_manager/password_form_manager.cc:330:
pending_credentials_.username_value) && iter->second->preferred) {
this block needs less indentation.

[zel, 2009-11-25 02:16:12]

http://codereview.chromium.org/437032/diff/1/2
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/437032/diff/1/2#newcode323
chrome/browser/password_manager/password_form_manager.cc:323: if
(!password_store) {
On 2009/11/25 01:16:55, timsteele wrote:
> nit - this check is redundant given the callsites, you could just swap for a
> DCHECK... but not a big deal.

Done.

http://codereview.chromium.org/437032/diff/1/2#newcode330
chrome/browser/password_manager/password_form_manager.cc:330:
pending_credentials_.username_value) && iter->second->preferred) {
On 2009/11/25 01:16:55, timsteele wrote:
> this block needs less indentation.

Done.

[Ben Goodger (Google), 2009-11-25 02:25:49]

I can land this for you tomorrow morning.

-Ben

On Tue, Nov 24, 2009 at 6:16 PM,  <zelidrag@chromium.org> wrote:
>
> http://codereview.chromium.org/437032/diff/1/2
> File chrome/browser/password_manager/password_form_manager.cc (right):
>
> http://codereview.chromium.org/437032/diff/1/2#newcode323
> chrome/browser/password_manager/password_form_manager.cc:323: if
> (!password_store) {
> On 2009/11/25 01:16:55, timsteele wrote:
>>
>> nit - this check is redundant given the callsites, you could just swap
>
> for a
>>
>> DCHECK... but not a big deal.
>
> Done.
>
> http://codereview.chromium.org/437032/diff/1/2#newcode330
> chrome/browser/password_manager/password_form_manager.cc:330:
> pending_credentials_.username_value) && iter->second->preferred) {
> On 2009/11/25 01:16:55, timsteele wrote:
>>
>> this block needs less indentation.
>
> Done.
>
> http://codereview.chromium.org/437032
>

[tim (not reviewing), 2009-11-25 02:32:22]

Oh, I thought you had commit access from the chromium.org account.

I can land it, no worries! (might be tomorrow morning or later tonight)

On Tue, Nov 24, 2009 at 6:25 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> I can land this for you tomorrow morning.
>
> -Ben
>
> On Tue, Nov 24, 2009 at 6:16 PM,  <zelidrag@chromium.org> wrote:
> >
> > http://codereview.chromium.org/437032/diff/1/2
> > File chrome/browser/password_manager/password_form_manager.cc (right):
> >
> > http://codereview.chromium.org/437032/diff/1/2#newcode323
> > chrome/browser/password_manager/password_form_manager.cc:323: if
> > (!password_store) {
> > On 2009/11/25 01:16:55, timsteele wrote:
> >>
> >> nit - this check is redundant given the callsites, you could just swap
> >
> > for a
> >>
> >> DCHECK... but not a big deal.
> >
> > Done.
> >
> > http://codereview.chromium.org/437032/diff/1/2#newcode330
> > chrome/browser/password_manager/password_form_manager.cc:330:
> > pending_credentials_.username_value) && iter->second->preferred) {
> > On 2009/11/25 01:16:55, timsteele wrote:
> >>
> >> this block needs less indentation.
> >
> > Done.
> >
> > http://codereview.chromium.org/437032
> >
>

[Ben Goodger (Google), 2009-11-25 02:38:08]

OK, let me know when you commit it.

-Ben

On Tue, Nov 24, 2009 at 6:32 PM, Tim Steele <tim@chromium.org> wrote:
> Oh, I thought you had commit access from the chromium.org account.
> I can land it, no worries! (might be tomorrow morning or later tonight)
>
> On Tue, Nov 24, 2009 at 6:25 PM, Ben Goodger (Google) <ben@chromium.org>
> wrote:
>>
>> I can land this for you tomorrow morning.
>>
>> -Ben
>>
>> On Tue, Nov 24, 2009 at 6:16 PM,  <zelidrag@chromium.org> wrote:
>> >
>> > http://codereview.chromium.org/437032/diff/1/2
>> > File chrome/browser/password_manager/password_form_manager.cc (right):
>> >
>> > http://codereview.chromium.org/437032/diff/1/2#newcode323
>> > chrome/browser/password_manager/password_form_manager.cc:323: if
>> > (!password_store) {
>> > On 2009/11/25 01:16:55, timsteele wrote:
>> >>
>> >> nit - this check is redundant given the callsites, you could just swap
>> >
>> > for a
>> >>
>> >> DCHECK... but not a big deal.
>> >
>> > Done.
>> >
>> > http://codereview.chromium.org/437032/diff/1/2#newcode330
>> > chrome/browser/password_manager/password_form_manager.cc:330:
>> > pending_credentials_.username_value) && iter->second->preferred) {
>> > On 2009/11/25 01:16:55, timsteele wrote:
>> >>
>> >> this block needs less indentation.
>> >
>> > Done.
>> >
>> > http://codereview.chromium.org/437032
>> >
>
>

[tim (not reviewing), 2009-11-25 06:06:22]

Committed.

[Ben Goodger (Google), 2009-11-25 06:11:27]

Thanks!

On Tue, Nov 24, 2009 at 10:06 PM,  <tim@chromium.org> wrote:
> Committed.
>
> http://codereview.chromium.org/437032
>