Password manager: improve the check for login failure

Password manager: improve the check for login failure

Before, we regarded any HTML form on the landing page after a login attempt as failure.

After this CL, it's only a failure, if the new form has the same action as the old one.

BUG=329860
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245105

[vabr (Chromium), 2014-01-14 17:17:31]

Hi Garrett,

May I interest you in yet another code review? :)

Thank you,
Vaclav

[Garrett Casto, 2014-01-15 02:28:27]

lgtm

https://codereview.chromium.org/137783003/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/137783003/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:357:
visible_forms[i].action) {
One thing to note is that it looks like at some point
PasswordFormManager::DoesManage() was used to check this (see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas...).
 This was before my time so I'm not sure why this was removed, but if it turns
out that your change makes matching too loose switching to DoesManage seems
reasonable.

[vabr (Chromium), 2014-01-15 08:37:53]

Thanks, Garrett.

Since the only change after your approval was adding a comment, I'm sending to
the CQ now.

Cheers,
Vaclav

https://codereview.chromium.org/137783003/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/137783003/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:357:
visible_forms[i].action) {
I actually started this CL with DoesManage + ACTION_MATCH_REQUIRED, but it
always returned false, because the names of the username or password elements
were not the same.

For now, I think just checking the actions is obvious enough to leave there
directly. If we end up with a more complex similarity check, it may make sense
to modify and use DoesManage for it. I'll put a comment saying this in the code.

On 2014/01/15 02:28:27, Garrett Casto wrote:
> One thing to note is that it looks like at some point
> PasswordFormManager::DoesManage() was used to check this (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas...).
>  This was before my time so I'm not sure why this was removed, but if it turns
> out that your change makes matching too loose switching to DoesManage seems
> reasonable.

[commit-bot: I haz the power, 2014-01-15 09:48:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/137783003/110001

[commit-bot: I haz the power, 2014-01-16 04:22:21]

Change committed as 245105