[Password Manager] Presave the form with generated password till successful login

[Password Manager] Presave the form with generated password till the user makes successful login or removes the generated password.

To be sure we save the generated password even if we missed form submission, presave the password right after the user accepted it. Also update the password after every user input and delete it, if the user clears the password field.

The form is presaved with empty username.

BUG=582434
Committed: https://crrev.com/578305d33ed2580610f3cdbb8ca529dfc3faf195
Cr-Commit-Position: refs/heads/master@{#386289}

[kolos1, 2016-04-05 15:09:25]

Description was changed from

==========
[Password Manager] Instant adding generated password into store
==========

to

==========
[Password Manager] Presave the form with generated password till the user makes
successful login or removes the generated password.
==========

[kolos1, 2016-04-05 15:18:19]

kolos@chromium.org changed reviewers:
+ dvadym@chromium.org, mkwst@chromium.org, vabr@chromium.org

[kolos1, 2016-04-05 15:18:20]

mkwst@: Please review autofill_messages.h

dvadym@: Please review this CL.

vabr@: Vadym will have a look at the most of files, please review
password_generation_agent_browsertest.cc.

[kolos1, 2016-04-05 15:22:51]

Description was changed from

==========
[Password Manager] Presave the form with generated password till the user makes
successful login or removes the generated password.
==========

to

==========
[Password Manager] Presave the form with generated password till the user makes
successful login or removes the generated password.

To be sure we save the generated password even if we missed form submission,
presave the password right after the user accepted it. Also update the password
after every user input and delete it, if the user clears the password field.

The form is presaved with empty username.
==========

[vabr (Chromium), 2016-04-06 01:19:07]

chrome/renderer/autofill/password_generation_agent_browsertest.cc LGTM.

Thanks!
Vaclav

[Mike West, 2016-04-06 07:24:09]

The IPC changes look reasonable. I'll take another look once Vadym has gone
through the CL, just to make sure any design shifts are covered.

[dvadym, 2016-04-06 12:44:23]

CL in general looks good. I have some comments (mostly nits) and 2 concerns:

 1.Does it work with a generation on a change password form and updating of
existing credentials after a successful submit? Could you please write a test
for this?
 2.Would it work correctly if the user decides to generate a password on
different forms on the same page? Or it's impossible?

https://codereview.chromium.org/1817483002/diff/320001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/1817483002/diff/320001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:345:
IPC_MESSAGE_ROUTED1(AutofillHostMsg_PresaveGeneratedPassword,
Why do we need 2 messages? It seems that 1 is enough, anyway in
PasswordFormManager::UpdatePresavedPassword we save if presaved_form is absent.

https://codereview.chromium.org/1817483002/diff/320001/components/autofill/co...
File components/autofill/content/renderer/password_generation_agent.h (right):

https://codereview.chromium.org/1817483002/diff/320001/components/autofill/co...
components/autofill/content/renderer/password_generation_agent.h:110: // Creates
a password form to presave generated password. It copies behavior
nit: generated->a generated

https://codereview.chromium.org/1817483002/diff/320001/components/autofill/co...
components/autofill/content/renderer/password_generation_agent.h:113: // If form
creating is failed, returns an empty unique_ptr.
nit: form -> a form

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:630: if
(!presaved_form_)
I'd prefer to reverse this condition, in order to avoid negation for code
simplification.

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:266: // Called
after successful login on the form with generated password.
nit: generated -> a generated

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_manager.h:87: // Presaves and
updates the form with generated password.
I'm not sure that it's correct to write presave together in comments, probably
it's more correct pre-save. Could you please ask native speaker how it's better?
And it's also related to all other comments.

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_manager.h:92: // If
|password_is_generated == false|, removes the presaved form.
nit: d == false| -> d| == false

[kolos1, 2016-04-07 13:05:18]

https://codereview.chromium.org/1817483002/diff/320001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/1817483002/diff/320001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:345:
IPC_MESSAGE_ROUTED1(AutofillHostMsg_PresaveGeneratedPassword,
On 2016/04/06 12:44:23, dvadym wrote:
> Why do we need 2 messages? It seems that 1 is enough, anyway in
> PasswordFormManager::UpdatePresavedPassword we save if presaved_form is
absent.

Done.

https://codereview.chromium.org/1817483002/diff/320001/components/autofill/co...
File components/autofill/content/renderer/password_generation_agent.h (right):

https://codereview.chromium.org/1817483002/diff/320001/components/autofill/co...
components/autofill/content/renderer/password_generation_agent.h:110: // Creates
a password form to presave generated password. It copies behavior
On 2016/04/06 12:44:23, dvadym wrote:
> nit: generated->a generated

Done.

https://codereview.chromium.org/1817483002/diff/320001/components/autofill/co...
components/autofill/content/renderer/password_generation_agent.h:113: // If form
creating is failed, returns an empty unique_ptr.
On 2016/04/06 12:44:23, dvadym wrote:
> nit: form -> a form

Done.

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:630: if
(!presaved_form_)
On 2016/04/06 12:44:23, dvadym wrote:
> I'd prefer to reverse this condition, in order to avoid negation for code
> simplification.

Done.

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:266: // Called
after successful login on the form with generated password.
On 2016/04/06 12:44:23, dvadym wrote:
> nit: generated -> a generated

Done.

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_manager.h:87: // Presaves and
updates the form with generated password.
On 2016/04/06 12:44:23, dvadym wrote:
> I'm not sure that it's correct to write presave together in comments, probably
> it's more correct pre-save. Could you please ask native speaker how it's
better?
> And it's also related to all other comments.

We should write 'presave'.

https://codereview.chromium.org/1817483002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_manager.h:92: // If
|password_is_generated == false|, removes the presaved form.
On 2016/04/06 12:44:23, dvadym wrote:
> nit: d == false| -> d| == false

Done.

[kolos1, 2016-04-07 13:29:44]

On 2016/04/06 12:44:23, dvadym wrote:
> CL in general looks good. I have some comments (mostly nits) and 2 concerns:
> 
>  1.Does it work with a generation on a change password form and updating of
> existing credentials after a successful submit? Could you please write a test
> for this?
Yes, it works for change password forms (for saving and for updating). Presaving
is independent of the configuration of passwords fields. It just works with the
password field where generated password was accepted. I believe there is no need
in a specific test for presaving on change password forms.

>  2.Would it work correctly if the user decides to generate a password on
> different forms on the same page? Or it's impossible?
It will work even if the user decide to generate password in another password
form. The presaved password will be associated to the corresponding form. So,
there might be several presaved passwords.

[kolos1, 2016-04-07 13:50:47]

Patchset #18 (id:340001) has been deleted

[dvadym, 2016-04-07 15:04:20]

On 2016/04/07 13:29:44, kolos1 wrote:
> On 2016/04/06 12:44:23, dvadym wrote:
> > CL in general looks good. I have some comments (mostly nits) and 2 concerns:
> > 
> >  1.Does it work with a generation on a change password form and updating of
> > existing credentials after a successful submit? Could you please write a
test
> > for this?
> Yes, it works for change password forms (for saving and for updating).
Presaving
> is independent of the configuration of passwords fields. It just works with
the
> password field where generated password was accepted. I believe there is no
need
> in a specific test for presaving on change password forms.

Great that it works. I'd prefer to have a test for change password forms. Test
is not only for testing current implementation (that could be similar of SignUp
and Change password forms) but also for protecting from future changes.

> >  2.Would it work correctly if the user decides to generate a password on
> > different forms on the same page? Or it's impossible?
> It will work even if the user decide to generate password in another password
> form. The presaved password will be associated to the corresponding form. So,
> there might be several presaved passwords.
Ok, great.

[dvadym, 2016-04-07 15:06:27]

Description was changed from

==========
[Password Manager] Presave the form with generated password till the user makes
successful login or removes the generated password.

To be sure we save the generated password even if we missed form submission,
presave the password right after the user accepted it. Also update the password
after every user input and delete it, if the user clears the password field.

The form is presaved with empty username.
==========

to

==========
[Password Manager] Presave the form with generated password till the user makes
successful login or removes the generated password.

To be sure we save the generated password even if we missed form submission,
presave the password right after the user accepted it. Also update the password
after every user input and delete it, if the user clears the password field.

The form is presaved with empty username.

BUG=582434
==========

[kolos1, 2016-04-07 15:09:44]

On 2016/04/07 15:04:20, dvadym wrote:
> On 2016/04/07 13:29:44, kolos1 wrote:
> > On 2016/04/06 12:44:23, dvadym wrote:
> > > CL in general looks good. I have some comments (mostly nits) and 2
concerns:
> > > 
> > >  1.Does it work with a generation on a change password form and updating
of
> > > existing credentials after a successful submit? Could you please write a
> test
> > > for this?
> > Yes, it works for change password forms (for saving and for updating).
> Presaving
> > is independent of the configuration of passwords fields. It just works with
> the
> > password field where generated password was accepted. I believe there is no
> need
> > in a specific test for presaving on change password forms.
> 
> Great that it works. I'd prefer to have a test for change password forms. Test
> is not only for testing current implementation (that could be similar of
SignUp
> and Change password forms) but also for protecting from future changes.
Sorry, forgot to mention that I added the test for change forms. See
password_generation_agent_browsertest.cc

[dvadym, 2016-04-07 19:20:42]

Thanks! LGTM % small nit

https://codereview.chromium.org/1817483002/diff/360001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1817483002/diff/360001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1328: 
It makes sense here to check that expectation on AddLogin on mock object store_
is fullfilled, by using Mock::VerifyAndClearExpectations
for example
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas...

[Mike West, 2016-04-08 08:11:03]

IPC LGTM.

[Mike West, 2016-04-08 08:11:03]

IPC LGTM.

[kolos1, 2016-04-08 08:21:51]

https://codereview.chromium.org/1817483002/diff/360001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1817483002/diff/360001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1328: 
On 2016/04/07 19:20:42, dvadym wrote:
> It makes sense here to check that expectation on AddLogin on mock object
store_
> is fullfilled, by using Mock::VerifyAndClearExpectations
> for example
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas...

Done.

[kolos1, 2016-04-08 08:52:14]

The CQ bit was checked by kolos@chromium.org

[kolos1, 2016-04-08 08:52:15]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
dvadym@chromium.org
Link to the patchset: https://codereview.chromium.org/1817483002/#ps380001
(title: "Added Mock::VerifyAndClearExpectations in a unittest")

[commit-bot: I haz the power, 2016-04-08 08:52:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1817483002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1817483002/380001

[commit-bot: I haz the power, 2016-04-08 09:01:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-08 09:01:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kolos1, 2016-04-08 09:25:18]

kolos@chromium.org changed reviewers:
+ isherman@chromium.org

[kolos1, 2016-04-08 09:25:19]

isherman@: Please review tools/metrics/histograms/histograms.xml. 

Regards,
Maxim

[kolos1, 2016-04-08 10:52:05]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-08 10:52:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1817483002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1817483002/400001

[commit-bot: I haz the power, 2016-04-08 11:59:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-08 11:59:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kolos1, 2016-04-08 20:53:22]

isherman@: friendly ping. Please review this tiny change in histograms.xml. 

Best regards,
Maxim

[Ilya Sherman, 2016-04-09 06:18:38]

histograms.xml lgtm

[Ilya Sherman, 2016-04-09 06:18:44]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-04-09 06:18:45]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
dvadym@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1817483002/#ps400001
(title: "Removed CPMD_BAD_ORIGIN_UPDATE_PRESAVED_PASSWORD in bad_message.h")

[commit-bot: I haz the power, 2016-04-09 06:18:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1817483002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1817483002/400001

[commit-bot: I haz the power, 2016-04-09 06:45:49]

Description was changed from

==========
[Password Manager] Presave the form with generated password till the user makes
successful login or removes the generated password.

To be sure we save the generated password even if we missed form submission,
presave the password right after the user accepted it. Also update the password
after every user input and delete it, if the user clears the password field.

The form is presaved with empty username.

BUG=582434
==========

to

==========
[Password Manager] Presave the form with generated password till the user makes
successful login or removes the generated password.

To be sure we save the generated password even if we missed form submission,
presave the password right after the user accepted it. Also update the password
after every user input and delete it, if the user clears the password field.

The form is presaved with empty username.

BUG=582434
==========

[commit-bot: I haz the power, 2016-04-09 06:45:50]

Committed patchset #20 (id:400001)

[commit-bot: I haz the power, 2016-04-09 06:47:01]

Description was changed from

==========
[Password Manager] Presave the form with generated password till the user makes
successful login or removes the generated password.

To be sure we save the generated password even if we missed form submission,
presave the password right after the user accepted it. Also update the password
after every user input and delete it, if the user clears the password field.

The form is presaved with empty username.

BUG=582434
==========

to

==========
[Password Manager] Presave the form with generated password till the user makes
successful login or removes the generated password.

To be sure we save the generated password even if we missed form submission,
presave the password right after the user accepted it. Also update the password
after every user input and delete it, if the user clears the password field.

The form is presaved with empty username.

BUG=582434

Committed: https://crrev.com/578305d33ed2580610f3cdbb8ca529dfc3faf195
Cr-Commit-Position: refs/heads/master@{#386289}
==========

[commit-bot: I haz the power, 2016-04-09 06:47:02]

Patchset 20 (id:??) landed as
https://crrev.com/578305d33ed2580610f3cdbb8ca529dfc3faf195
Cr-Commit-Position: refs/heads/master@{#386289}