[Password Generation] Always save generated passwords

[Password Generation] Always save generated passwords

TBR=estade@chromium.org
BUG=506307
Committed: https://crrev.com/01403f89096e52d043a473417a291f838b5de704
Cr-Commit-Position: refs/heads/master@{#337648}

[Garrett Casto, 2015-07-01 19:06:08]

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

[vabr (Chromium), 2015-07-02 07:40:05]

Thanks for the prompt fix, Garrett!

LGTM + some comments.

Cheers,
Vaclav

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager.cc:206:
ProvisionallySavePassword(form);
Should this only happen when password_is_generated is true?

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_unittest.cc:1057: //
happening.
nit: Please mention that this passing the same |observed| as before would
normally not save the password, but generated passwords are an exception tested
in this test.

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_unittest.cc:1095:
observed.clear();
nit: We should clarify what would be the behaviour without clearing |observed|.
Ideally with a 3rd test.

[Garrett Casto, 2015-07-06 21:16:07]

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager.cc:206:
ProvisionallySavePassword(form);
On 2015/07/02 07:40:04, vabr (Chromium) wrote:
> Should this only happen when password_is_generated is true?

Yes, though when the password is no longer generated I guess we should delete
|privisional_save_managers_| so that we don't force save any longer. Updated.

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_unittest.cc:1057: //
happening.
On 2015/07/02 07:40:05, vabr (Chromium) wrote:
> nit: Please mention that this passing the same |observed| as before would
> normally not save the password, but generated passwords are an exception
tested
> in this test.

Comment was a bit stale. Updated.

[Garrett Casto, 2015-07-06 21:16:39]

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

[Garrett Casto, 2015-07-06 21:16:39]

+estade for tiny PasswordGenerationPopupControllerImpl change.

[vabr (Chromium), 2015-07-07 10:04:58]

Hi Garrett,
LGTM with a nit.

Thanks!
Vaclav

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1213803006/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_unittest.cc:1057: //
happening.
On 2015/07/06 21:16:07, Garrett Casto wrote:
> On 2015/07/02 07:40:05, vabr (Chromium) wrote:
> > nit: Please mention that this passing the same |observed| as before would
> > normally not save the password, but generated passwords are an exception
> tested
> > in this test.
> 
> Comment was a bit stale. Updated.

Acknowledged.

https://codereview.chromium.org/1213803006/diff/40001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1213803006/diff/40001/components/password_man...
components/password_manager/core/browser/password_manager.cc:212:
(*matched_manager_it)->set_has_generated_password(password_is_generated);
nit: Due to line 175, password_is_generated is false here, so we should replace
it with |true| on line 212 and 221. It took me some time to realise that nothing
here happens for non-generated forms.

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

gcasto@chromium.org changed reviewers:
- estade@chromium.org

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

The CQ bit was checked by gcasto@chromium.org

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

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/1213803006/#ps60001
(title: "Comments")

[commit-bot: I haz the power, 2015-07-07 17:21:01]

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

[commit-bot: I haz the power, 2015-07-07 18:23:32]

Committed patchset #4 (id:60001)

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

Patchset 4 (id:??) landed as
https://crrev.com/01403f89096e52d043a473417a291f838b5de704
Cr-Commit-Position: refs/heads/master@{#337648}

[Garrett Casto, 2015-07-07 22:14:40]

https://codereview.chromium.org/1213803006/diff/40001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1213803006/diff/40001/components/password_man...
components/password_manager/core/browser/password_manager.cc:212:
(*matched_manager_it)->set_has_generated_password(password_is_generated);
On 2015/07/07 10:04:57, vabr (Chromium) wrote:
> nit: Due to line 175, password_is_generated is false here, so we should
replace
> it with |true| on line 212 and 221. It took me some time to realise that
nothing
> here happens for non-generated forms.

Done.

[Garrett Casto, 2015-07-11 20:00:45]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1231733006/ by gcasto@chromium.org.

The reason for reverting is: Change causes generation to save passwords without
a username in some cases..