Clean up password manager code.

Clean up password manager code.

BUG=none
TEST=password saving should continue to work


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

[Ilya Sherman, 2012-03-01 05:59:14]

Mike, please take a look at all of the changes except to profile code.
Rachel, please take a look at the (trivial) changes to profile code.
John, please take a look at the (trivial) changes to content code.

[jam, 2012-03-01 06:11:00]

lgtm with niy

https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile.h:192: virtual bool IsOffTheRecord() const = 0;
this should be taken out, no need to repeat the pure virtual methods of the
parent class here

[tim (not reviewing), 2012-03-01 06:25:21]

Thanks for doing this!  I had one question, sorry for being slow...

https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/password...
File chrome/browser/password_manager/password_manager.cc (right):

https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/password...
chrome/browser/password_manager/password_manager.cc:154:
pending_login_managers_.reset();
Could you help me understand how this is equivalent to the DidNavigate() code? 
Is it true that before we were cleaning up at a later navigation (i.e. away from
the landing page), versus now being a bit more aggressive?  I'm likely missing
something, but just checking.

[Ilya Sherman, 2012-03-01 06:35:54]

https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/password...
File chrome/browser/password_manager/password_manager.cc (right):

https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/password...
chrome/browser/password_manager/password_manager.cc:154:
pending_login_managers_.reset();
On 2012/03/01 06:25:21, timsteele wrote:
> Could you help me understand how this is equivalent to the DidNavigate() code?

> Is it true that before we were cleaning up at a later navigation (i.e. away
from
> the landing page), versus now being a bit more aggressive?  I'm likely missing
> something, but just checking.

Well, the DidNavigate() code wasn't actually wired up to anything, so I question
it's correctness to begin with. ;)  That said, I could see no reason for either
part of the if-stmt condition in the DidNavigate() code.  The first part,
"!provisional_save_manager_.get()", seems irrelevant since the
provisional_save_manager_ has already been yanked out of the
pending_login_managers_ list.  The second part,
"!pending_login_managers_.empty()", just says "if we're empty, don't clean
up"... but IMO that just makes things more confusing than always cleaning up.

https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile.h:192: virtual bool IsOffTheRecord() const = 0;
On 2012/03/01 06:11:00, John Abd-El-Malek wrote:
> this should be taken out, no need to repeat the pure virtual methods of the
> parent class here

Done.

[tim (not reviewing), 2012-03-01 07:01:28]

On 2012/03/01 06:35:54, Ilya Sherman wrote:
>
https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/password...
> File chrome/browser/password_manager/password_manager.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/password...
> chrome/browser/password_manager/password_manager.cc:154:
> pending_login_managers_.reset();
> On 2012/03/01 06:25:21, timsteele wrote:
> > Could you help me understand how this is equivalent to the DidNavigate()
code?
> 
> > Is it true that before we were cleaning up at a later navigation (i.e. away
> from
> > the landing page), versus now being a bit more aggressive?  I'm likely
missing
> > something, but just checking.
> 
> Well, the DidNavigate() code wasn't actually wired up to anything, so I
question
> it's correctness to begin with. ;)  That said, I could see no reason for
either
> part of the if-stmt condition in the DidNavigate() code.  The first part,
> "!provisional_save_manager_.get()", seems irrelevant since the
> provisional_save_manager_ has already been yanked out of the
> pending_login_managers_ list.  The second part,
> "!pending_login_managers_.empty()", just says "if we're empty, don't clean
> up"... but IMO that just makes things more confusing than always cleaning up.
> 

The original intent was to not clean up if this navigation was due to any of the
steps in the ascii-art diagram shown in password_manager.h.  It *definitely* was
correct and needed at one time (which was 3 years ago now), I can guarantee
that, because I have vivid flashbacks/night tremors of harping over exactly that
line of code.  But a lot has changed since then in terms of the navigation
semantics, detecting user gestures vs auto navs at higher layers, and code in
PasswordManager for that matter... so it's totally possible it became somehow
obsolete :) 

Have to head out, but I'll continue reviewing first thing tomorrow.
>
https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/profiles...
> File chrome/browser/profiles/profile.h (right):
> 
>
https://chromiumcodereview.appspot.com/9564001/diff/1/chrome/browser/profiles...
> chrome/browser/profiles/profile.h:192: virtual bool IsOffTheRecord() const =
0;
> On 2012/03/01 06:11:00, John Abd-El-Malek wrote:
> > this should be taken out, no need to repeat the pure virtual methods of the
> > parent class here
> 
> Done.

[Mike Mammarella, 2012-03-01 08:23:50]

LGTM. Shame on whoever wrote those iter++ lines.

https://chromiumcodereview.appspot.com/9564001/diff/7001/chrome/browser/passw...
File chrome/browser/password_manager/password_manager.cc (right):

https://chromiumcodereview.appspot.com/9564001/diff/7001/chrome/browser/passw...
chrome/browser/password_manager/password_manager.cc:118:
provisional_save_manager_.reset(manager);
Do we want to clear pending_login_managers_ here, or do we now defer that until
DidNavigateAnyFrame()?

[Ilya Sherman, 2012-03-01 08:39:18]

https://chromiumcodereview.appspot.com/9564001/diff/7001/chrome/browser/passw...
File chrome/browser/password_manager/password_manager.cc (right):

https://chromiumcodereview.appspot.com/9564001/diff/7001/chrome/browser/passw...
chrome/browser/password_manager/password_manager.cc:118:
provisional_save_manager_.reset(manager);
On 2012/03/01 08:23:51, Mike Mammarella wrote:
> Do we want to clear pending_login_managers_ here, or do we now defer that
until
> DidNavigateAnyFrame()?

AFAICT, this is only ever called from DidNavigateAnyFrame().  Since we want to
clear the vector there anyway, might as well just do it in the one place.

[tim (not reviewing), 2012-03-01 17:57:52]

http://codereview.chromium.org/9564001/diff/7001/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager.cc (right):

http://codereview.chromium.org/9564001/diff/7001/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:154:
pending_login_managers_.reset();
So, two things.  One, yeah, DidNavigate (then in WebContents) used to be much
less sophisticated...  It did not do the filtering / splitting navigation types
up into certain functions, like this one, which is only called post-commit of
the load (versus possibly during provisional load).

A second thing I notice, however - because we're using 'AnyFrame', and we're not
checking matching location urls, if a subframe navigates (e.g. automatically, or
by user clicking) on the page that is *not* the one containing the password form
(that's what the origin.is_valid() check is for, it's basically "is there a form
here"), then we'll clear pending_login_managers, meaning if the user tries to
submit the password form we won't be able to save it.

This would have been OK before, as the clear was protected by the above if
clause.

Does that seem plausible?

[Mike Mammarella, 2012-03-01 19:02:04]

http://codereview.chromium.org/9564001/diff/7001/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager.cc (right):

http://codereview.chromium.org/9564001/diff/7001/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:154:
pending_login_managers_.reset();
On 2012/03/01 17:57:52, timsteele wrote:
> So, two things.  One, yeah, DidNavigate (then in WebContents) used to be much
> less sophisticated...  It did not do the filtering / splitting navigation
types
> up into certain functions, like this one, which is only called post-commit of
> the load (versus possibly during provisional load).
> 
> A second thing I notice, however - because we're using 'AnyFrame', and we're
not
> checking matching location urls, if a subframe navigates (e.g. automatically,
or
> by user clicking) on the page that is *not* the one containing the password
form
> (that's what the origin.is_valid() check is for, it's basically "is there a
form
> here"), then we'll clear pending_login_managers, meaning if the user tries to
> submit the password form we won't be able to save it.
> 
> This would have been OK before, as the clear was protected by the above if
> clause.
> 
> Does that seem plausible?

That sounds both plausible and eminently testable. :) I've put together a little
test for this: http://www/~mdm/subnav.html

Does the new code behave the same as the old code with this?

[rpetterson, 2012-03-01 19:54:27]

Profiles LGTM.

[Ilya Sherman, 2012-03-02 01:07:15]

http://codereview.chromium.org/9564001/diff/7001/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager.cc (right):

http://codereview.chromium.org/9564001/diff/7001/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:154:
pending_login_managers_.reset();
On 2012/03/01 19:02:04, Mike Mammarella wrote:
> On 2012/03/01 17:57:52, timsteele wrote:
> > So, two things.  One, yeah, DidNavigate (then in WebContents) used to be
much
> > less sophisticated...  It did not do the filtering / splitting navigation
> types
> > up into certain functions, like this one, which is only called post-commit
of
> > the load (versus possibly during provisional load).
> > 
> > A second thing I notice, however - because we're using 'AnyFrame', and we're
> not
> > checking matching location urls, if a subframe navigates (e.g.
automatically,
> or
> > by user clicking) on the page that is *not* the one containing the password
> form
> > (that's what the origin.is_valid() check is for, it's basically "is there a
> form
> > here"), then we'll clear pending_login_managers, meaning if the user tries
to
> > submit the password form we won't be able to save it.
> > 
> > This would have been OK before, as the clear was protected by the above if
> > clause.
> > 
> > Does that seem plausible?
> 
> That sounds both plausible and eminently testable. :) I've put together a
little
> test for this: http://www/%7Emdm/subnav.html
> 
> Does the new code behave the same as the old code with this?

Ah, that's a really good catch -- I hadn't really understood what the if-stmt
was all about.  Fixed.  (Thanks for putting together the test site, Mike!)

[Mike Mammarella, 2012-03-02 01:46:09]

http://codereview.chromium.org/9564001/diff/1033/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager.cc (right):

http://codereview.chromium.org/9564001/diff/1033/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:153: return;
How feasible would it be to write a unit test for this?

[Ilya Sherman, 2012-03-02 02:50:06]

http://codereview.chromium.org/9564001/diff/1033/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager.cc (right):

http://codereview.chromium.org/9564001/diff/1033/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:153: return;
On 2012/03/02 01:46:09, Mike Mammarella wrote:
> How feasible would it be to write a unit test for this?

Done.

[Ilya Sherman, 2012-03-02 02:51:50]

http://codereview.chromium.org/9564001/diff/3007/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager.h (right):

http://codereview.chromium.org/9564001/diff/3007/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.h:51: // the LoginPrompt code.
Incidentally, I'm rather confused as to how the LoginPrompt ever goes from
provisionally saving a password to actually saving the password...

[Mike Mammarella, 2012-03-02 03:36:26]

LGTM! Thanks for the test :)

[tim (not reviewing), 2012-03-02 18:21:01]

LGTM++ and thanks guys!

re: LoginPrompt... I'll need to refresh my memory on that one, too...

[commit-bot: I haz the power, 2012-03-02 18:24:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/9564001/3007

[commit-bot: I haz the power, 2012-03-02 21:31:34]

Change committed as 124735