Creating a preference for storing a sync password hash.

Creating a preference for storing a sync password hash.

This CL contains:
1.Adding constants for a new preference name (password_manager_pref_names.*)
2.Registring a new preference in a stotic method of password_manager.cc (it's a place where all password related preferences are registered)
3.Adding |prefs| argument to PasswordStore->Init and passing prefs to PasswordReusedDetector in PasswordStore->Init
4.Saving |prefs| to a member variable in PasswordReusedDetector
5.Saving an empty string to the new preference in PasswordReusedDetector->SaveSyncPasswordHash: an empty string is saving because: we can test that saving to a preference works in PasswordReusedDetector and to avoid unused error.
6.Test in password_reuse_detector_unittest.cc that saving to the preference is working. 
7.Changes in all others files are just adding prefs argument to all overrides of Init method and passing nullptr in tests to this parameter.

BUG=657041
Review-Url: https://codereview.chromium.org/2846023003
Cr-Commit-Position: refs/heads/master@{#468628}
Committed: https://chromium.googlesource.com/chromium/src/+/3e1ee15ee6d944585a2e616e3f5ea286f84ab51d

[dvadym, 2017-04-28 14:08:31]

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

[commit-bot: I haz the power, 2017-04-28 14:09:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 14:13:32]

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

[commit-bot: I haz the power, 2017-04-28 14:13:33]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[dvadym, 2017-04-28 15:41:48]

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

[commit-bot: I haz the power, 2017-04-28 15:42:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 15:50:27]

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

[commit-bot: I haz the power, 2017-04-28 15:50:28]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dvadym, 2017-04-28 15:56:35]

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

[commit-bot: I haz the power, 2017-04-28 15:57:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 16:06:16]

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

[commit-bot: I haz the power, 2017-04-28 16:06:18]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dvadym, 2017-05-02 12:38:25]

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

[commit-bot: I haz the power, 2017-05-02 12:38:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dvadym, 2017-05-02 12:47:06]

Description was changed from

==========
Creating a preference for sync password hash

BUG=657041
==========

to

==========
Creating a preference for storing a sync password hash.

This CL contains:
1.Adding constants for a new preference name (password_manager_pref_names.*)
2.Registring a new preference in a stotic method of password_manager.cc (it's a
place where all password related preferences are registered)
3.Adding |prefs| argument to PasswordStore->Init and passing prefs to
PasswordReusedDetector in PasswordStore->Init
4.Saving |prefs| to a member variable in PasswordReusedDetector
5.Saving an empty string to the new preference in
PasswordReusedDetector->SaveSyncPasswordHash: an empty string is saving because:
we can test that saving to a preference works in PasswordReusedDetector and to
avoid unused error.
6.Test in password_reuse_detector_unittest.cc that saving to the preference is
working. 
7.Changes in all others files are just adding prefs argument to all overrides of
Init method and passing nullptr in tests to this parameter.

BUG=657041
==========

[dvadym, 2017-05-02 12:47:06]

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

[dvadym, 2017-05-02 12:49:36]

Hi Vaclav,

could you please review this CL? While the number of changed files looked scary,
changes in almost all files are trivial, not trivial parts are in CL
description.

Regards,
Vadym

[vabr (Chromium), 2017-05-02 13:26:15]

Hi Vadym,

LGTM with comments.

Cheers,
Vaclav

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:158:
registry->RegisterStringPref(prefs::kSyncPasswordHash, "",
nit: Please use std::string() instead of "", that's more efficient, harder to
overlook, and makes the type clear.

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_reuse_detector.h (right):

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detector.h:39:
PasswordReuseDetector(PrefService* prefs);
explicit

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detector.h:97:
PrefService* prefs_;
Could this be "PrefService* const" to make clear that this pointer stays the
same for the object's lifetime?

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
File
components/password_manager/core/browser/password_reuse_detector_unittest.cc
(right):

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detector_unittest.cc:235:
prefs.registry()->RegisterStringPref(prefs::kSyncPasswordHash, "",
nit: "" -> std::string()

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_store.cc:731:
reuse_detector_.reset(new PasswordReuseDetector(prefs));
optional: When you are already touching this, would you mind rewriting to the
following?

reuse_detector_ = base::MakeUnique<PasswordReuseDetector>(prefs);

There is little difference, but I'd like to have a bare "new" usage as rare as
possible in the code, to encourage zero exposure of raw owning pointers.

[dvadym, 2017-05-02 13:37:19]

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

[commit-bot: I haz the power, 2017-05-02 13:37:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dvadym, 2017-05-02 13:37:33]

Thanks for review Vaclav! I've addressed all your comments

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:158:
registry->RegisterStringPref(prefs::kSyncPasswordHash, "",
On 2017/05/02 13:26:14, vabr (Chromium) wrote:
> nit: Please use std::string() instead of "", that's more efficient, harder to
> overlook, and makes the type clear.

Done.

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_reuse_detector.h (right):

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detector.h:39:
PasswordReuseDetector(PrefService* prefs);
On 2017/05/02 13:26:14, vabr (Chromium) wrote:
> explicit

Done.

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detector.h:97:
PrefService* prefs_;
On 2017/05/02 13:26:14, vabr (Chromium) wrote:
> Could this be "PrefService* const" to make clear that this pointer stays the
> same for the object's lifetime?

Thanks, it makes sense. Done.

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
File
components/password_manager/core/browser/password_reuse_detector_unittest.cc
(right):

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detector_unittest.cc:235:
prefs.registry()->RegisterStringPref(prefs::kSyncPasswordHash, "",
On 2017/05/02 13:26:14, vabr (Chromium) wrote:
> nit: "" -> std::string()

Done.

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/2846023003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_store.cc:731:
reuse_detector_.reset(new PasswordReuseDetector(prefs));
On 2017/05/02 13:26:14, vabr (Chromium) wrote:
> optional: When you are already touching this, would you mind rewriting to the
> following?
> 
> reuse_detector_ = base::MakeUnique<PasswordReuseDetector>(prefs);
> 
> There is little difference, but I'd like to have a bare "new" usage as rare as
> possible in the code, to encourage zero exposure of raw owning pointers.

Done.

[vabr (Chromium), 2017-05-02 13:39:59]

LGTM!

[dvadym, 2017-05-02 14:03:45]

The CQ bit was unchecked by dvadym@chromium.org

[dvadym, 2017-05-02 14:03:45]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2017-05-02 14:04:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 14:26:17]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493733825966930,
"parent_rev": "68557831bd0eefa24608e16f7018dc89059a9a09", "commit_rev":
"3e1ee15ee6d944585a2e616e3f5ea286f84ab51d"}

[commit-bot: I haz the power, 2017-05-02 14:26:37]

Description was changed from

==========
Creating a preference for storing a sync password hash.

This CL contains:
1.Adding constants for a new preference name (password_manager_pref_names.*)
2.Registring a new preference in a stotic method of password_manager.cc (it's a
place where all password related preferences are registered)
3.Adding |prefs| argument to PasswordStore->Init and passing prefs to
PasswordReusedDetector in PasswordStore->Init
4.Saving |prefs| to a member variable in PasswordReusedDetector
5.Saving an empty string to the new preference in
PasswordReusedDetector->SaveSyncPasswordHash: an empty string is saving because:
we can test that saving to a preference works in PasswordReusedDetector and to
avoid unused error.
6.Test in password_reuse_detector_unittest.cc that saving to the preference is
working. 
7.Changes in all others files are just adding prefs argument to all overrides of
Init method and passing nullptr in tests to this parameter.

BUG=657041
==========

to

==========
Creating a preference for storing a sync password hash.

This CL contains:
1.Adding constants for a new preference name (password_manager_pref_names.*)
2.Registring a new preference in a stotic method of password_manager.cc (it's a
place where all password related preferences are registered)
3.Adding |prefs| argument to PasswordStore->Init and passing prefs to
PasswordReusedDetector in PasswordStore->Init
4.Saving |prefs| to a member variable in PasswordReusedDetector
5.Saving an empty string to the new preference in
PasswordReusedDetector->SaveSyncPasswordHash: an empty string is saving because:
we can test that saving to a preference works in PasswordReusedDetector and to
avoid unused error.
6.Test in password_reuse_detector_unittest.cc that saving to the preference is
working. 
7.Changes in all others files are just adding prefs argument to all overrides of
Init method and passing nullptr in tests to this parameter.

BUG=657041

Review-Url: https://codereview.chromium.org/2846023003
Cr-Commit-Position: refs/heads/master@{#468628}
Committed:
https://chromium.googlesource.com/chromium/src/+/3e1ee15ee6d944585a2e616e3f5e...
==========

[commit-bot: I haz the power, 2017-05-02 14:26:38]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/3e1ee15ee6d944585a2e616e3f5e...