Fill the un-encrypted metadata field in password specifics.

Fill the unencrypted metadata field in password specifics.

Fills the unencrypted metadata field when writing to the password entity is triggered, which should happen on every start up. This way it allows us to bootstrap the over wise empty field.

* Field should stay empty for custom passphrase users.
* The feature is guarded by the experiment, so the field should stay empty if experiment is disabled.
* The entity shouldn't be marked unsynced unless the change to the field has happened.

BUG=638963
Committed: https://crrev.com/72e34aa0b649fb40e03bc171da020bb4e9ccdef6
Cr-Commit-Position: refs/heads/master@{#427336}

[melandory, 2016-09-02 05:50:46]

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

[commit-bot: I haz the power, 2016-09-02 05:50:55]

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, 2016-09-02 06:00:38]

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

[commit-bot: I haz the power, 2016-09-02 06:00:40]

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

[melandory, 2016-09-02 06:18:10]

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

[commit-bot: I haz the power, 2016-09-02 06:18:26]

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, 2016-09-02 06:27:13]

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

[commit-bot: I haz the power, 2016-09-02 06:27:14]

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

[melandory, 2016-09-02 06:41:29]

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

[commit-bot: I haz the power, 2016-09-02 06:41:42]

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, 2016-09-02 07:04:48]

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

[commit-bot: I haz the power, 2016-09-02 07:04:49]

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...)

[melandory, 2016-09-09 12:37:13]

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

[commit-bot: I haz the power, 2016-09-09 12:37:24]

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, 2016-09-09 13:04:27]

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

[commit-bot: I haz the power, 2016-09-09 13:04: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...)

[melandory, 2016-09-23 13:29:53]

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

[commit-bot: I haz the power, 2016-09-23 13:30:06]

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, 2016-09-23 13:34:50]

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

[commit-bot: I haz the power, 2016-09-23 13:34:51]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[melandory, 2016-09-23 13:38:14]

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

[commit-bot: I haz the power, 2016-09-23 13:38:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2016-09-23 13:38:50]

Patchset #4 (id:60001) has been deleted

[melandory, 2016-09-23 13:38:50]

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

[melandory, 2016-09-23 13:38:59]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-09-23 13:39:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2016-09-23 13:39:18]

Patchset #3 (id:40001) has been deleted

[melandory, 2016-09-23 13:39:21]

Patchset #1 (id:1) has been deleted

[melandory, 2016-09-23 13:54:47]

Description was changed from

==========
Fill the un-encrypted metadata field in password specifics.

Fill the un-encrypted metadata field in password specifics when the password
entity is changed.

BUG=638963
==========

to

==========
Fill the unencrypted metadata field in password specifics.

Fill the unencrypted metadata field in password specifics.

Fills the unencrypted metadata field when writing to the password entity is
triggered, which should happen on every start up. This way it allows us to
bootstrap the over wise empty field.

* Field should stay empty for custom passphrase users.
* The feature is guarded by the experiment, so the field should stay empty if
experiment is disabled.
* The entity shouldn't be marked unsynced unless the change to the field has
happened. 

BUG=638963
==========

[melandory, 2016-09-23 13:55:01]

Description was changed from

==========
Fill the unencrypted metadata field in password specifics.

Fill the unencrypted metadata field in password specifics.

Fills the unencrypted metadata field when writing to the password entity is
triggered, which should happen on every start up. This way it allows us to
bootstrap the over wise empty field.

* Field should stay empty for custom passphrase users.
* The feature is guarded by the experiment, so the field should stay empty if
experiment is disabled.
* The entity shouldn't be marked unsynced unless the change to the field has
happened. 

BUG=638963
==========

to

==========
Fill the unencrypted metadata field in password specifics.

Fills the unencrypted metadata field when writing to the password entity is
triggered, which should happen on every start up. This way it allows us to
bootstrap the over wise empty field.

* Field should stay empty for custom passphrase users.
* The feature is guarded by the experiment, so the field should stay empty if
experiment is disabled.
* The entity shouldn't be marked unsynced unless the change to the field has
happened. 

BUG=638963
==========

[melandory, 2016-09-23 14:00:00]

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

[commit-bot: I haz the power, 2016-09-23 14:00:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2016-09-23 14:00:56]

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

[commit-bot: I haz the power, 2016-09-23 14:01:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2016-09-23 14:01:42]

melandory@chromium.org changed reviewers:
+ zea@chromium.org

[melandory, 2016-09-23 14:01:43]

PTAL

[commit-bot: I haz the power, 2016-09-23 14:21:08]

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

[commit-bot: I haz the power, 2016-09-23 14:21:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Nicolas Zea, 2016-09-23 23:10:37]

Is there any test that verifies that writing a new password with a custom
passphrase doesn't set the metadata?

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
File components/sync/core/password_metadata_filling.cc (right):

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
components/sync/core/password_metadata_filling.cc:10: "PasswordMetadaraFilling",
base::FEATURE_DISABLED_BY_DEFAULT};
PasswordMetadaraFilling -> PasswordMetadataFilling

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
File components/sync/core/password_metadata_filling.h (right):

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
components/sync/core/password_metadata_filling.h:5: #ifndef
COMPONENETS_SYNC_CORE_PASSWORDS_METADAT_FILLING_H_
Two typos (componenents and metadat) in this macro. I'm surprised the presubmit
didn't catch that though?

Also, what about calling this file sync_features.h, and moving it to sync/base?
We're likely to have other features in the future, so may as well centralize it.

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
components/sync/core/password_metadata_filling.h:12: extern const base::Feature
kPasswordsMetadataFilling;
I think it might be cleaner to just call this kFillPasswordMetadata

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/w...
File components/sync/core/write_node.cc (right):

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/w...
components/sync/core/write_node.cc:146: }
nit: newline after

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core_i...
File components/sync/core_impl/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core_i...
components/sync/core_impl/sync_manager_impl_unittest.cc:2235:
TEST_F(SyncManagerTest,
nit: To be consistent, would be good to add a comment explaining the purpose of
this test.

[melandory, 2016-10-02 19:43:34]

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

[commit-bot: I haz the power, 2016-10-02 19:43:43]

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, 2016-10-02 20:28:08]

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

[commit-bot: I haz the power, 2016-10-02 20:28:09]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-10-04 07:43:39]

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

[commit-bot: I haz the power, 2016-10-04 07:43:50]

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, 2016-10-04 08:53:08]

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

[commit-bot: I haz the power, 2016-10-04 08:53:09]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-10-04 10:13:04]

On 2016/09/23 23:10:37, Nicolas Zea wrote:
> Is there any test that verifies that writing a new password with a custom
> passphrase doesn't set the metadata?
Added to the test, which sets up the custom passphrase.
> 
>
https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
> File components/sync/core/password_metadata_filling.cc (right):
> 
>
https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
> components/sync/core/password_metadata_filling.cc:10:
"PasswordMetadaraFilling",
> base::FEATURE_DISABLED_BY_DEFAULT};
> PasswordMetadaraFilling -> PasswordMetadataFilling
> 
>
https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
> File components/sync/core/password_metadata_filling.h (right):
> 
>
https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
> components/sync/core/password_metadata_filling.h:5: #ifndef
> COMPONENETS_SYNC_CORE_PASSWORDS_METADAT_FILLING_H_
> Two typos (componenents and metadat) in this macro. I'm surprised the
presubmit
> didn't catch that though?
> 
> Also, what about calling this file sync_features.h, and moving it to
sync/base?
> We're likely to have other features in the future, so may as well centralize
it.
> 
>
https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
> components/sync/core/password_metadata_filling.h:12: extern const
base::Feature
> kPasswordsMetadataFilling;
> I think it might be cleaner to just call this kFillPasswordMetadata
> 
>
https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/w...
> File components/sync/core/write_node.cc (right):
> 
>
https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/w...
> components/sync/core/write_node.cc:146: }
> nit: newline after
> 
>
https://codereview.chromium.org/2294913009/diff/140001/components/sync/core_i...
> File components/sync/core_impl/sync_manager_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2294913009/diff/140001/components/sync/core_i...
> components/sync/core_impl/sync_manager_impl_unittest.cc:2235:
> TEST_F(SyncManagerTest,
> nit: To be consistent, would be good to add a comment explaining the purpose
of
> this test.

[melandory, 2016-10-04 10:13:12]

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
File components/sync/core/password_metadata_filling.cc (right):

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
components/sync/core/password_metadata_filling.cc:10: "PasswordMetadaraFilling",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/09/23 23:10:37, Nicolas Zea wrote:
> PasswordMetadaraFilling -> PasswordMetadataFilling

Done.

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
File components/sync/core/password_metadata_filling.h (right):

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
components/sync/core/password_metadata_filling.h:5: #ifndef
COMPONENETS_SYNC_CORE_PASSWORDS_METADAT_FILLING_H_
On 2016/09/23 23:10:37, Nicolas Zea wrote:
> Two typos (componenents and metadat) in this macro. I'm surprised the
presubmit
> didn't catch that though?
Yep, weird.
> 
> Also, what about calling this file sync_features.h, and moving it to
sync/base?
> We're likely to have other features in the future, so may as well centralize
it.
Done.

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/p...
components/sync/core/password_metadata_filling.h:12: extern const base::Feature
kPasswordsMetadataFilling;
On 2016/09/23 23:10:37, Nicolas Zea wrote:
> I think it might be cleaner to just call this kFillPasswordMetadata

Done.

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/w...
File components/sync/core/write_node.cc (right):

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core/w...
components/sync/core/write_node.cc:146: }
On 2016/09/23 23:10:37, Nicolas Zea wrote:
> nit: newline after

Done.

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core_i...
File components/sync/core_impl/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2294913009/diff/140001/components/sync/core_i...
components/sync/core_impl/sync_manager_impl_unittest.cc:2235:
TEST_F(SyncManagerTest,
On 2016/09/23 23:10:37, Nicolas Zea wrote:
> nit: To be consistent, would be good to add a comment explaining the purpose
of
> this test.

Done.

[Nicolas Zea, 2016-10-04 20:58:12]

LGTM!

https://codereview.chromium.org/2294913009/diff/180001/components/sync/core/s...
File components/sync/core/sync_features.cc (right):

https://codereview.chromium.org/2294913009/diff/180001/components/sync/core/s...
components/sync/core/sync_features.cc:9: const base::Feature
kFillPasswordMetadata{"PasswordMetadataFilling",
nit: Add comment describing what this feature is about. Maybe reference a bug
number too?

https://codereview.chromium.org/2294913009/diff/180001/components/sync/core_i...
File components/sync/core_impl/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2294913009/diff/180001/components/sync/core_i...
components/sync/core_impl/sync_manager_impl_unittest.cc:2155: // Check that
writing new password doesn't set the metadata.
nit: newline above

[melandory, 2016-10-14 13:21:45]

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

[commit-bot: I haz the power, 2016-10-14 13:22:08]

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, 2016-10-14 14:18:13]

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

[commit-bot: I haz the power, 2016-10-14 14:18:14]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-10-18 08:35:20]

https://codereview.chromium.org/2294913009/diff/180001/components/sync/core/s...
File components/sync/core/sync_features.cc (right):

https://codereview.chromium.org/2294913009/diff/180001/components/sync/core/s...
components/sync/core/sync_features.cc:9: const base::Feature
kFillPasswordMetadata{"PasswordMetadataFilling",
On 2016/10/04 20:58:11, Nicolas Zea wrote:
> nit: Add comment describing what this feature is about. Maybe reference a bug
> number too?

Done.

https://codereview.chromium.org/2294913009/diff/180001/components/sync/core_i...
File components/sync/core_impl/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2294913009/diff/180001/components/sync/core_i...
components/sync/core_impl/sync_manager_impl_unittest.cc:2155: // Check that
writing new password doesn't set the metadata.
On 2016/10/04 20:58:11, Nicolas Zea wrote:
> nit: newline above

Done.

[melandory, 2016-10-18 08:35:27]

The CQ bit was checked by melandory@chromium.org

[melandory, 2016-10-18 08:35:28]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2294913009/#ps220001
(title: "address comments")

[commit-bot: I haz the power, 2016-10-18 08:35:45]

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, 2016-10-18 08:37:34]

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

[commit-bot: I haz the power, 2016-10-18 08:37:35]

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

[melandory, 2016-10-24 09:21:32]

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

[commit-bot: I haz the power, 2016-10-24 09:21:46]

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, 2016-10-24 09:30:50]

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

[commit-bot: I haz the power, 2016-10-24 09:30:51]

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

[melandory, 2016-10-24 10:32:38]

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

[commit-bot: I haz the power, 2016-10-24 10:33: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, 2016-10-24 12:42:06]

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

[commit-bot: I haz the power, 2016-10-24 12:42:07]

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

[melandory, 2016-10-24 14:12:08]

Nicolas, can you check that components/sync/base is the right place for
sync_feature?

[Nicolas Zea, 2016-10-24 16:25:43]

zea@chromium.org changed reviewers:
+ maxbogue@chromium.org

[Nicolas Zea, 2016-10-24 16:25:44]

yep, LGTM (+max just in case though)

[maxbogue, 2016-10-24 17:04:35]

sync_features being in base/ lgtm. The only other place we have for them right
now is in driver, but I'd been considering moving that to base and your stuff is
used lower in the hierarchy than driver:

https://cs.chromium.org/chromium/src/components/sync/driver/sync_driver_switc...

[melandory, 2016-10-25 13:13:09]

The CQ bit was checked by melandory@chromium.org

[commit-bot: I haz the power, 2016-10-25 13:13:24]

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, 2016-10-25 14:03:34]

Description was changed from

==========
Fill the unencrypted metadata field in password specifics.

Fills the unencrypted metadata field when writing to the password entity is
triggered, which should happen on every start up. This way it allows us to
bootstrap the over wise empty field.

* Field should stay empty for custom passphrase users.
* The feature is guarded by the experiment, so the field should stay empty if
experiment is disabled.
* The entity shouldn't be marked unsynced unless the change to the field has
happened. 

BUG=638963
==========

to

==========
Fill the unencrypted metadata field in password specifics.

Fills the unencrypted metadata field when writing to the password entity is
triggered, which should happen on every start up. This way it allows us to
bootstrap the over wise empty field.

* Field should stay empty for custom passphrase users.
* The feature is guarded by the experiment, so the field should stay empty if
experiment is disabled.
* The entity shouldn't be marked unsynced unless the change to the field has
happened. 

BUG=638963
==========

[commit-bot: I haz the power, 2016-10-25 14:03:36]

Committed patchset #11 (id:260001)

[commit-bot: I haz the power, 2016-10-25 14:06:10]

Description was changed from

==========
Fill the unencrypted metadata field in password specifics.

Fills the unencrypted metadata field when writing to the password entity is
triggered, which should happen on every start up. This way it allows us to
bootstrap the over wise empty field.

* Field should stay empty for custom passphrase users.
* The feature is guarded by the experiment, so the field should stay empty if
experiment is disabled.
* The entity shouldn't be marked unsynced unless the change to the field has
happened. 

BUG=638963
==========

to

==========
Fill the unencrypted metadata field in password specifics.

Fills the unencrypted metadata field when writing to the password entity is
triggered, which should happen on every start up. This way it allows us to
bootstrap the over wise empty field.

* Field should stay empty for custom passphrase users.
* The feature is guarded by the experiment, so the field should stay empty if
experiment is disabled.
* The entity shouldn't be marked unsynced unless the change to the field has
happened.

BUG=638963

Committed: https://crrev.com/72e34aa0b649fb40e03bc171da020bb4e9ccdef6
Cr-Commit-Position: refs/heads/master@{#427336}
==========

[commit-bot: I haz the power, 2016-10-25 14:06:11]

Patchset 11 (id:??) landed as
https://crrev.com/72e34aa0b649fb40e03bc171da020bb4e9ccdef6
Cr-Commit-Position: refs/heads/master@{#427336}