Introduce password_manager::FormSaver

Introduce password_manager::FormSaver

The goal: outsource modifying calls to PasswordStore made in PasswordFormManager to a separate class.

This CL achieves that by:
 * Creating a FormSaver interface to provide the store-modifications needed by PFM. PFM is changed to require a FormSaver object on creation.
 * Separating the store-modifying code from PFM into FormSaverImpl, implementing FormSaver.
 * Adding tests for FormSaverImpl, reducing PFM tests accordingly, providing test support for stubbing FormSaver and using it in existing tests.

TODO for a future CL:
 * Moving WipeStoreCopyIfOutdated to FormSaver.

For more details see the design doc at
https://docs.google.com/document/d/12CH_SV1gIJKaEIGTs7gWBiXrCMlBa3o32OyC2LRtPAo/edit?usp=sharing

BUG=621355
R=dvadym@chromium.org

Committed: https://crrev.com/0637ae71c31807c3882293be5f23453dd2e80342
Cr-Commit-Position: refs/heads/master@{#401647}

[vabr (Chromium), 2016-06-23 09:23:45]

Hi Vadym,

Could you please have a look? If you feel this is too big for you to find time
before holiday, that's OK, just please let me know.

Thank you!

Vaclav

[dvadym, 2016-06-23 15:28:22]

Hi Vaclav, thanks for refactoring this. In general it looks good, I have some
comments.

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
File components/password_manager/core/browser/form_saver.h (right):

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
components/password_manager/core/browser/form_saver.h:31: virtual void Save(
I'd prefer to split this function on 2, Save and Update. A signature of this
function is quite complex and now it's used for 2 goals and it's needed to use
DCHECK that both cases are correct. WDYT?

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

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
components/password_manager/core/browser/form_saver_impl.cc:45: best_matches_ =
&best_matches;
I'm worried a little bit about setting pointers to non-owning objects to member
variables, and that these member variables will keep pointers after finishing of
Save call. Now they are not used, but as usual with some changes it could
change. For example it's possible to set this variables back to nullptr in the
end of this function or use auto_reset. WDYT?

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
File components/password_manager/core/browser/form_saver_impl.h (right):

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
components/password_manager/core/browser/form_saver_impl.h:54: // Caches the
pending credential.
+ during a call to Save()

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

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
components/password_manager/core/browser/password_form_manager.cc:152: void
SanitizePossibleUsernames(PasswordForm* form) {
Thanks for refactoring this method, it becomes much clear now.

[vabr (Chromium), 2016-06-23 16:38:56]

Hi Vadym

Thanks for the helpful review!

Comments addressed, please take a look.

Vaclav

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
File components/password_manager/core/browser/form_saver.h (right):

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
components/password_manager/core/browser/form_saver.h:31: virtual void Save(
On 2016/06/23 15:28:22, dvadym wrote:
> I'd prefer to split this function on 2, Save and Update. A signature of this
> function is quite complex and now it's used for 2 goals and it's needed to use
> DCHECK that both cases are correct. WDYT?

Done.

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

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
components/password_manager/core/browser/form_saver_impl.cc:45: best_matches_ =
&best_matches;
On 2016/06/23 15:28:22, dvadym wrote:
> I'm worried a little bit about setting pointers to non-owning objects to
member
> variables, and that these member variables will keep pointers after finishing
of
> Save call. Now they are not used, but as usual with some changes it could
> change. For example it's possible to set this variables back to nullptr in the
> end of this function or use auto_reset. WDYT?

Done, I share the concern and AutoReset looks like a good idea.

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
File components/password_manager/core/browser/form_saver_impl.h (right):

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
components/password_manager/core/browser/form_saver_impl.h:54: // Caches the
pending credential.
On 2016/06/23 15:28:22, dvadym wrote:
> + during a call to Save()

Done.

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

https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
components/password_manager/core/browser/password_form_manager.cc:152: void
SanitizePossibleUsernames(PasswordForm* form) {
On 2016/06/23 15:28:22, dvadym wrote:
> Thanks for refactoring this method, it becomes much clear now.

Acknowledged. :)

[dvadym, 2016-06-23 16:46:18]

On 2016/06/23 16:38:56, vabr (Chromium) wrote:
> Hi Vadym
> 
> Thanks for the helpful review!
> 
> Comments addressed, please take a look.
> 
> Vaclav
> 
>
https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
> File components/password_manager/core/browser/form_saver.h (right):
> 
>
https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
> components/password_manager/core/browser/form_saver.h:31: virtual void Save(
> On 2016/06/23 15:28:22, dvadym wrote:
> > I'd prefer to split this function on 2, Save and Update. A signature of this
> > function is quite complex and now it's used for 2 goals and it's needed to
use
> > DCHECK that both cases are correct. WDYT?
> 
> Done.
> 
>
https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
> File components/password_manager/core/browser/form_saver_impl.cc (right):
> 
>
https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
> components/password_manager/core/browser/form_saver_impl.cc:45: best_matches_
=
> &best_matches;
> On 2016/06/23 15:28:22, dvadym wrote:
> > I'm worried a little bit about setting pointers to non-owning objects to
> member
> > variables, and that these member variables will keep pointers after
finishing
> of
> > Save call. Now they are not used, but as usual with some changes it could
> > change. For example it's possible to set this variables back to nullptr in
the
> > end of this function or use auto_reset. WDYT?
> 
> Done, I share the concern and AutoReset looks like a good idea.
> 
>
https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
> File components/password_manager/core/browser/form_saver_impl.h (right):
> 
>
https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
> components/password_manager/core/browser/form_saver_impl.h:54: // Caches the
> pending credential.
> On 2016/06/23 15:28:22, dvadym wrote:
> > + during a call to Save()
> 
> Done.
> 
>
https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
> File components/password_manager/core/browser/password_form_manager.cc
(right):
> 
>
https://codereview.chromium.org/2090583003/diff/1/components/password_manager...
> components/password_manager/core/browser/password_form_manager.cc:152: void
> SanitizePossibleUsernames(PasswordForm* form) {
> On 2016/06/23 15:28:22, dvadym wrote:
> > Thanks for refactoring this method, it becomes much clear now.
> 
> Acknowledged. :)

LGTM

[vabr (Chromium), 2016-06-23 16:51:31]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-06-23 16:51:49]

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

[commit-bot: I haz the power, 2016-06-23 17:44:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-23 17:47:20]

Description was changed from

==========
Introduce password_manager::FormSaver

The goal: outsource modifying calls to PasswordStore made in PasswordFormManager
to a separate class.

This CL achieves that by:
 * Creating a FormSaver interface to provide the store-modifications needed by
PFM. PFM is changed to require a FormSaver object on creation.
 * Separating the store-modifying code from PFM into FormSaverImpl, implementing
FormSaver.
 * Adding tests for FormSaverImpl, reducing PFM tests accordingly, providing
test support for stubbing FormSaver and using it in existing tests.

TODO for a future CL:
 * Moving WipeStoreCopyIfOutdated to FormSaver.

For more details see the design doc at
https://docs.google.com/document/d/12CH_SV1gIJKaEIGTs7gWBiXrCMlBa3o32OyC2LRtP...

BUG=621355
R=dvadym@chromium.org
==========

to

==========
Introduce password_manager::FormSaver

The goal: outsource modifying calls to PasswordStore made in PasswordFormManager
to a separate class.

This CL achieves that by:
 * Creating a FormSaver interface to provide the store-modifications needed by
PFM. PFM is changed to require a FormSaver object on creation.
 * Separating the store-modifying code from PFM into FormSaverImpl, implementing
FormSaver.
 * Adding tests for FormSaverImpl, reducing PFM tests accordingly, providing
test support for stubbing FormSaver and using it in existing tests.

TODO for a future CL:
 * Moving WipeStoreCopyIfOutdated to FormSaver.

For more details see the design doc at
https://docs.google.com/document/d/12CH_SV1gIJKaEIGTs7gWBiXrCMlBa3o32OyC2LRtP...

BUG=621355
R=dvadym@chromium.org

Committed: https://crrev.com/0637ae71c31807c3882293be5f23453dd2e80342
Cr-Commit-Position: refs/heads/master@{#401647}
==========

[commit-bot: I haz the power, 2016-06-23 17:47:21]

Patchset 2 (id:??) landed as
https://crrev.com/0637ae71c31807c3882293be5f23453dd2e80342
Cr-Commit-Position: refs/heads/master@{#401647}