[Password Generation] Save passwords even if heuristics determine submit failed

[Password Generation] Save passwords even if heuristics determine submit failed

The previous attempt at this (https://codereview.chromium.org/1213803006)
would save even if the user didn't finish submitting the page. This will not
save passwords if the user generated the password, and then navigates somewhere
without submitting.

BUG=506307
Committed: https://crrev.com/6824eb61d14a8306d93498897dec46deaf7e390b
Cr-Commit-Position: refs/heads/master@{#338536}

[Garrett Casto, 2015-07-11 23:17:41]

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

[Garrett Casto, 2015-07-11 23:17:42]

Hey Valcav,

This is the second try at automatically saving generated passwords. I reverted
the first because it would drop the username from automatically saved passwords
and the fix for that was non-trivial enough that I was worried about merging it
this late. This change is a subset of the previous one that only tries to force
save if the submission fails. I also fixed the tests (I overlooked the fact that
FormMatches didn't verify the content of the form) and added a new interactive
UI test. The later is disabled for now because of crbug.com/407988, which I am
currently working on fixing.

[vabr (Chromium), 2015-07-13 14:57:00]

Hi Garrett,

LGTM, thanks for being careful with the approaching branch point.

Cheers,
Vaclav

https://codereview.chromium.org/1237543002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_generation_interactive_uitest.cc
(right):

https://codereview.chromium.org/1237543002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:193:
DISABLED_AutoSavingGeneratedPassword) {
nit: Could you please add a bugtracker URL associated with enabling this test?

https://codereview.chromium.org/1237543002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:204:
// Change the action so that it looks like the submission failed.
I'm confused -- where is the script below changing the action? And if you meant
"change the username", how does that make the submission look failed?

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1037: //
The initial load.
optional nit: I'm not convinced that the comment here and on line 1039 is
actually useful, the method names are self-explanatory. In fact, I'm just
putting together a clean-up CL to remove these comments, so I suggest dropping
them here as well.

Similarly on lines 1062 and 1064 and in the rest of the tests.

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1044: //
The user should not be presented with an infobar as they have already given
nit: Alternative phrasing:
"be presented with an infobar" -> "need to confirm saving"
as not everywhere the UI is an infobar.

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1086: //
occured.
typo: occurred

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1106:
PasswordGenerationNoLongerGeneratedPasswordNotForceSaved_FailedSubmit) {
In this test and the next one, it is not clear, what a "failed submit" means.
Looking just at the comments, these tests are the same, but only the second one
provisionally saves the form. Could you please clarify which types of submit
failures this deals with?

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1131:
observed);  // The post-navigation load.
Here we give a second reason (after not provisionally saving the form) for the
saving to fail. That sounds fishy, the tests should check one thing every time.

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1183: //
Simulate user changing the password and username, without every completely
typo: every -> ever

[Garrett Casto, 2015-07-13 17:20:13]

https://codereview.chromium.org/1237543002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_generation_interactive_uitest.cc
(right):

https://codereview.chromium.org/1237543002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:193:
DISABLED_AutoSavingGeneratedPassword) {
On 2015/07/13 14:57:00, vabr (Chromium) wrote:
> nit: Could you please add a bugtracker URL associated with enabling this test?

Done.

https://codereview.chromium.org/1237543002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:204:
// Change the action so that it looks like the submission failed.
On 2015/07/13 14:57:00, vabr (Chromium) wrote:
> I'm confused -- where is the script below changing the action? And if you
meant
> "change the username", how does that make the submission look failed?

I have no idea what I was thinking when I wrote this. Changed to match the test.

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1037: //
The initial load.
On 2015/07/13 14:57:00, vabr (Chromium) wrote:
> optional nit: I'm not convinced that the comment here and on line 1039 is
> actually useful, the method names are self-explanatory. In fact, I'm just
> putting together a clean-up CL to remove these comments, so I suggest dropping
> them here as well.
> 
> Similarly on lines 1062 and 1064 and in the rest of the tests.

Done.

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1044: //
The user should not be presented with an infobar as they have already given
On 2015/07/13 14:57:00, vabr (Chromium) wrote:
> nit: Alternative phrasing:
> "be presented with an infobar" -> "need to confirm saving"
> as not everywhere the UI is an infobar.

True, done.

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1086: //
occured.
On 2015/07/13 14:57:00, vabr (Chromium) wrote:
> typo: occurred

Done.

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1106:
PasswordGenerationNoLongerGeneratedPasswordNotForceSaved_FailedSubmit) {
On 2015/07/13 14:57:00, vabr (Chromium) wrote:
> In this test and the next one, it is not clear, what a "failed submit" means.
> Looking just at the comments, these tests are the same, but only the second
one
> provisionally saves the form. Could you please clarify which types of submit
> failures this deals with?

I only mention failed submit here. I can expound upon it in a comment.

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1131:
observed);  // The post-navigation load.
On 2015/07/13 14:57:00, vabr (Chromium) wrote:
> Here we give a second reason (after not provisionally saving the form) for the
> saving to fail. That sounds fishy, the tests should check one thing every
time.

They are testing two different things. The two tests verify that if you generate
a password and then make it no longer generated 1) you are still prompted
normally if a submit occurs and 2) if submit fails we don't automatically save
(unlike with a generated password). I'll try to clear this up with comments.

https://codereview.chromium.org/1237543002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1183: //
Simulate user changing the password and username, without every completely
On 2015/07/13 14:57:00, vabr (Chromium) wrote:
> typo: every -> ever

Done.

[Garrett Casto, 2015-07-13 17:20:30]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2015-07-13 17:20:30]

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

[commit-bot: I haz the power, 2015-07-13 17:33:10]

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

[commit-bot: I haz the power, 2015-07-13 18:37:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-13 18:38:47]

Patchset 3 (id:??) landed as
https://crrev.com/6824eb61d14a8306d93498897dec46deaf7e390b
Cr-Commit-Position: refs/heads/master@{#338536}