Supplimentary identifier for passwords specific

Supplimentary identifier for passwords specific.

Adds new message field to the Password Scpecific proto and implements clearing of the data in case custom passphrase is turned on.
The population of the metadata is not implemented in this CL.

BUG=638963
Committed: https://crrev.com/71d9b4ade725d81ff9a8a85e68aeeab9bfb79f4a
Cr-Commit-Position: refs/heads/master@{#415699}

[melandory, 2016-08-26 16:01:27]

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

[melandory, 2016-08-26 16:01:28]

PTAL.

This CL is part of CLs chain described in last mail here:
https://codereview.chromium.org/2260953002

[Nicolas Zea, 2016-08-26 18:28:30]

https://codereview.chromium.org/2278333002/diff/20001/components/sync/core/wr...
File components/sync/core/write_node.cc (right):

https://codereview.chromium.org/2278333002/diff/20001/components/sync/core/wr...
components/sync/core/write_node.cc:170: if
(!UpdateEntryWithEncryption(GetTransaction()->GetWrappedTrans(),
Given you have access to the transaction now, I don't think you need to plumb
the passphrase in anymore?

I think that might imply you don't need UNDEFINED as a value either

[melandory, 2016-08-26 22:58:38]

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

[commit-bot: I haz the power, 2016-08-26 22:59:10]

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

[melandory, 2016-08-26 23:12:20]

Patchset #3 (id:40001) has been deleted

[melandory, 2016-08-26 23:14:48]

Patchset #3 (id:60001) has been deleted

[melandory, 2016-08-26 23:16:16]

Patchset #3 (id:80001) has been deleted

[melandory, 2016-08-26 23:16:59]

https://codereview.chromium.org/2278333002/diff/20001/components/sync/core/wr...
File components/sync/core/write_node.cc (right):

https://codereview.chromium.org/2278333002/diff/20001/components/sync/core/wr...
components/sync/core/write_node.cc:170: if
(!UpdateEntryWithEncryption(GetTransaction()->GetWrappedTrans(),
On 2016/08/26 18:28:30, Nicolas Zea wrote:
> Given you have access to the transaction now, I don't think you need to plumb
> the passphrase in anymore?
> 
> I think that might imply you don't need UNDEFINED as a value either

Oh, definitelly!

Fixed and updated CL https://codereview.chromium.org/2279753003/

[Nicolas Zea, 2016-08-26 23:29:12]

Looks like the changes to writenode aren't here? Did you mean to include them?

[melandory, 2016-08-26 23:33:05]

On 2016/08/26 23:29:12, Nicolas Zea wrote:
> Looks like the changes to writenode aren't here? Did you mean to include them?

But they were for plumbing. Currently I don't need them, since I can do cleanup
in nigori_util and access PassphraseType via NigoriHandler.
I might need those change when will populate the data, but then I'll add them in
the future.

[Nicolas Zea, 2016-08-26 23:46:56]

On 2016/08/26 23:33:05, melandory wrote:
> On 2016/08/26 23:29:12, Nicolas Zea wrote:
> > Looks like the changes to writenode aren't here? Did you mean to include
them?
> 
> But they were for plumbing. Currently I don't need them, since I can do
cleanup
> in nigori_util and access PassphraseType via NigoriHandler.
> I might need those change when will populate the data, but then I'll add them
in
> the future.

Ah, ok, I was wondering about populating the metadata from the
PasswordSpecificsData. If you're leaving that for a later CL that's fine though.

[Nicolas Zea, 2016-08-26 23:47:01]

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

https://codereview.chromium.org/2278333002/diff/100001/components/sync/core_i...
components/sync/core_impl/sync_manager_impl_unittest.cc:547: TEST_F(SyncApiTest,
WriteDoesntChangeUnencryptedPasswordMetadata) {
What's the purpose of this test? Just that the metadata gets persisted? It seems
like the test above could do that too (once the population of the metadata logic
is added)

https://codereview.chromium.org/2278333002/diff/100001/components/sync/core_i...
components/sync/core_impl/sync_manager_impl_unittest.cc:2113: // passphrase
updates the data.
nit: mention that it also clears the metadata

[melandory, 2016-08-26 23:59:47]

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

https://codereview.chromium.org/2278333002/diff/100001/components/sync/core_i...
components/sync/core_impl/sync_manager_impl_unittest.cc:547: TEST_F(SyncApiTest,
WriteDoesntChangeUnencryptedPasswordMetadata) {
On 2016/08/26 23:47:01, Nicolas Zea wrote:
> What's the purpose of this test? Just that the metadata gets persisted? It
seems
> like the test above could do that too (once the population of the metadata
logic
> is added)

Fixed. My idea was to have test for persisting of metadata and test above to
check that it is not currently populated.

https://codereview.chromium.org/2278333002/diff/100001/components/sync/core_i...
components/sync/core_impl/sync_manager_impl_unittest.cc:2113: // passphrase
updates the data.
On 2016/08/26 23:47:01, Nicolas Zea wrote:
> nit: mention that it also clears the metadata

Done.

[Nicolas Zea, 2016-08-27 00:05:30]

lgtm

[Nicolas Zea, 2016-08-27 00:05:54]

Before you commit, could you update the commit message to explain that this adds
clearing of the metadata, but not the populating of it?

[melandory, 2016-08-31 17:06:01]

Description was changed from

==========
Supplimentary identifier for passwords specific

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

to

==========
Supplimentary identifier for passwords specific.

Adds new message field to the Password Scpecific proto and implements clearing
of the data in case custom passphrase is turned on.

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

[melandory, 2016-08-31 17:06:42]

Description was changed from

==========
Supplimentary identifier for passwords specific.

Adds new message field to the Password Scpecific proto and implements clearing
of the data in case custom passphrase is turned on.

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

to

==========
Supplimentary identifier for passwords specific.

Adds new message field to the Password Scpecific proto and implements clearing
of the data in case custom passphrase is turned on.
The population of the metadata is not implemented in this CL.

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

[melandory, 2016-08-31 17:08:18]

The CQ bit was checked by melandory@chromium.org

[melandory, 2016-08-31 17:08:18]

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/2278333002/#ps140001
(title: "Supplimentary identifier for passwords specific.")

[commit-bot: I haz the power, 2016-08-31 17:08:35]

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-08-31 18:48:35]

Description was changed from

==========
Supplimentary identifier for passwords specific.

Adds new message field to the Password Scpecific proto and implements clearing
of the data in case custom passphrase is turned on.
The population of the metadata is not implemented in this CL.

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

to

==========
Supplimentary identifier for passwords specific.

Adds new message field to the Password Scpecific proto and implements clearing
of the data in case custom passphrase is turned on.
The population of the metadata is not implemented in this CL.

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

[commit-bot: I haz the power, 2016-08-31 18:48:36]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-08-31 18:50:21]

Description was changed from

==========
Supplimentary identifier for passwords specific.

Adds new message field to the Password Scpecific proto and implements clearing
of the data in case custom passphrase is turned on.
The population of the metadata is not implemented in this CL.

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

to

==========
Supplimentary identifier for passwords specific.

Adds new message field to the Password Scpecific proto and implements clearing
of the data in case custom passphrase is turned on.
The population of the metadata is not implemented in this CL.

BUG=638963

Committed: https://crrev.com/71d9b4ade725d81ff9a8a85e68aeeab9bfb79f4a
Cr-Commit-Position: refs/heads/master@{#415699}
==========

[commit-bot: I haz the power, 2016-08-31 18:50:23]

Patchset 5 (id:??) landed as
https://crrev.com/71d9b4ade725d81ff9a8a85e68aeeab9bfb79f4a
Cr-Commit-Position: refs/heads/master@{#415699}