[Sync] Add SyncEncryptionHandler

[Sync] Add SyncEncryptionHandler

All sync-specific encryption state (types, encrypt everything, explicit
passphrase, keys) is now tracked within the new class SyncEncryptionHandler.
It's owned by the sync manager, and unifies some of the observer logic
we previously had. In addition, it's capable of creating its own
transactions, taking us a step closer to have a nigori datatype. 

In addition, we add a NigoriHandler to abstract the chrome-side of
encryption from the sync visible side of encryption.

BUG=139848
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151833

[Nicolas Zea, 2012-08-10 02:58:06]

Enjoy!

I've preserved all existing tests. The plan is to, as two separate subsequent
patches, have NigoriChangeProcessor implement ChangeProcessor and have the
update logic go through it, and also introduce a new interface for getting the
encrypted types (that's thread safe) and have the cryptographer no longer be the
access point for that. Then, nothing really needs to include cryptographer
that's not part of sync internals.

https://chromiumcodereview.appspot.com/10827266/diff/1/sync/engine/syncer_uni...
File sync/engine/syncer_unittest.cc (left):

https://chromiumcodereview.appspot.com/10827266/diff/1/sync/engine/syncer_uni...
sync/engine/syncer_unittest.cc:983: TEST_F(SyncerTest, ReceiveOldNigori) {
this test moved to SyncEncryptionHandlerImplTest

[tim (not reviewing), 2012-08-12 23:31:20]

http://codereview.chromium.org/10827266/diff/8001/chrome/browser/sync/glue/ni...
File chrome/browser/sync/glue/nigori_change_processor.h (right):

http://codereview.chromium.org/10827266/diff/8001/chrome/browser/sync/glue/ni...
chrome/browser/sync/glue/nigori_change_processor.h:37: // TODO(zea):
ChangeProcessor implementation.
Why is this called ChangeProcessor if it does mostly model association and
doesn't implement ChangeProcessor interface?

http://codereview.chromium.org/10827266/diff/8001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10827266/diff/8001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:205: NigoriChangeProcessor*
nigori_processor() { return nigori_processor_.get(); }
Can we avoid exposing this?  The only reason we expose nigori above SBH today is
for Migration, ideally we'd hide it completely.

[tim (not reviewing), 2012-08-13 20:01:00]

Following our offline discussion, let me know when NigoriChangeProcessor is
removed and comments below are addressed.  Thanks!

http://codereview.chromium.org/10827266/diff/8001/sync/internal_api/public/sy...
File sync/internal_api/public/sync_encryption_handler.h (right):

http://codereview.chromium.org/10827266/diff/8001/sync/internal_api/public/sy...
sync/internal_api/public/sync_encryption_handler.h:45: // All Observer methods
are done synchronously and are assumed to be called
Why is the word 'assumed' here?  

'All Observer methods are called synchronously within a transaction on the sync
thread."

is clearer.

http://codereview.chromium.org/10827266/diff/8001/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

http://codereview.chromium.org/10827266/diff/8001/sync/util/cryptographer.h#n...
sync/util/cryptographer.h:59: void
SetSyncEncryptionHandlerDelegate(SyncEncryptionHandler* delegate);
As discussed offline, it looks like the set of things the Cryptographer needs on
SyncEncryptionHandler are mutually exclusive with the set of things that the
chrome/glue code needs.  I think we can simplify responsibilities by having a
separate interface for each of those two sets (though they can still be
implemented in the same place / SyncEncryptionHandlerImpl).

[Nicolas Zea, 2012-08-13 22:56:38]

NigoriChangeProcessor has been removed, and I added a NigoriHandler interface in
syncable/, which the sync encryption handler impl implements.

A follow up patch will move the cryptographer into the encryption handler, and
have the nigori handler be what's accessible from the directory.

http://codereview.chromium.org/10827266/diff/8001/chrome/browser/sync/glue/ni...
File chrome/browser/sync/glue/nigori_change_processor.h (right):

http://codereview.chromium.org/10827266/diff/8001/chrome/browser/sync/glue/ni...
chrome/browser/sync/glue/nigori_change_processor.h:37: // TODO(zea):
ChangeProcessor implementation.
On 2012/08/12 23:31:20, timsteele wrote:
> Why is this called ChangeProcessor if it does mostly model association and
> doesn't implement ChangeProcessor interface?

Removed this file.

http://codereview.chromium.org/10827266/diff/8001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10827266/diff/8001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:205: NigoriChangeProcessor*
nigori_processor() { return nigori_processor_.get(); }
On 2012/08/12 23:31:20, timsteele wrote:
> Can we avoid exposing this?  The only reason we expose nigori above SBH today
is
> for Migration, ideally we'd hide it completely.  

Done.

http://codereview.chromium.org/10827266/diff/8001/sync/internal_api/public/sy...
File sync/internal_api/public/sync_encryption_handler.h (right):

http://codereview.chromium.org/10827266/diff/8001/sync/internal_api/public/sy...
sync/internal_api/public/sync_encryption_handler.h:45: // All Observer methods
are done synchronously and are assumed to be called
On 2012/08/13 20:01:00, timsteele wrote:
> Why is the word 'assumed' here?  
> 
> 'All Observer methods are called synchronously within a transaction on the
sync
> thread."
> 
> is clearer.

Done.

http://codereview.chromium.org/10827266/diff/8001/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

http://codereview.chromium.org/10827266/diff/8001/sync/util/cryptographer.h#n...
sync/util/cryptographer.h:59: void
SetSyncEncryptionHandlerDelegate(SyncEncryptionHandler* delegate);
On 2012/08/13 20:01:00, timsteele wrote:
> As discussed offline, it looks like the set of things the Cryptographer needs
on
> SyncEncryptionHandler are mutually exclusive with the set of things that the
> chrome/glue code needs.  I think we can simplify responsibilities by having a
> separate interface for each of those two sets (though they can still be
> implemented in the same place / SyncEncryptionHandlerImpl). 

Done.

[tim (not reviewing), 2012-08-14 02:32:20]

This patch looks cool.  I have some suggestions based mostly on initial
reactions to seeing a lot of this code in the same place for the first time.

http://codereview.chromium.org/10827266/diff/12001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10827266/diff/12001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:745: // TODO(zea): cache this
value at the PSS, then make the encryption handler
Which value?  The result of the whole expression below?  Any reason we wouldn't
cache it here instead of PSS (SyncBackendHost)?

http://codereview.chromium.org/10827266/diff/12001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1143: done_callback.Run();
I think you can remove all the done_callback plumbing and PostClosure (!), and
explicitly post to frontend_loop_ here via host_.Call.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:30: // because the encryption
keys don't match (per chrome instantiation).
Can you include a link to the bug that led to this or have more of an
explanation in the comment here?

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:58: void
SyncEncryptionHandlerImpl::ReloadNigori() {
Is InitNigori clearer?  Something to get the point that this is only called
once, at startup, across.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:78: void
SyncEncryptionHandlerImpl::UpdateFromNigori(
Why not make this take a transaction and dcheck on it?  I ask because if we did
that, this call to ApplyNigoriUpdate would be the same as the one above in
ReloadNigori from a transaction point of view, and we could call
WriteEncryptionStateToNigori from within ApplyNigoriUpdate itself, meaning it
could just return void.  The 'bool' return value from ApplyNigoriUpdate is
pretty confusing, and it seems like that function could perform the fallback
work itself.  We could also get rid of RewriteNigori.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:98: void
SyncEncryptionHandlerImpl::SetEncryptionPassphrase(
Was this + SetDecryption pretty much just carried over?  If so I won't
scrutinize line-by-line and save myself from going down this treacherous path
again... :)

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:453: bool
encrypted_types_need_update = !UpdateEncryptedTypesFromNigori(nigori);
Something feels... weird here.  The function on the RHS says it is going to
update encrypted types, and if it returns false, we set a flag noting "encrypted
types need update". 

Maybe remove the ! and make the flag 'encrypted_types_match_nigori'?  Something
to note that what needs updating (as you mention below) is nigori, not
encrypted_types_ (after UpdateEncryptedTypesFromNigori returns).

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:483: // yet, prompt the user
for a passphrase.
"... isn't ready yet or has pending keys, ..."

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:485: DVLOG(1) <<
"OnPassPhraseRequired Sent";
nit - Passphrase

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:499: // If so, we need to
update the nigori node.
"... If so, we need to add to the set of encrypted types in the nigori node." is
clearer / more accurate, I think?

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:503: needs_new_keys)
nit - { } around multi line ifs.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:543:
UpdateNigoriFromEncryptedTypes(&nigori);
Would it be bad to just call the util directly here?

I'd be just as happy to move the util implementation here, if you wanted to e.g.
keep the interface symmetric (UpdateNigoriFromEncrypted +
UpdateEncryptedFromNigori).

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:589:
encrypted_types.Put(APP_NOTIFICATIONS);
Should there be a compile-assert here on number of data types?  Second, it'd be
nice for readability / understandability to pull these ~30 lines into a
ModelTypeSet ExtractEncryptedTypesFrom(NigoriSpecifics& s); routine.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:592: // encrypt_everything
field. If anything more than the sensitive types were
What version was this, again?  At some point I think it'll be fair to deny
commitss from a particular version, maybe if encryption is requested.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:609: return false;
these 3 lines are equivalent to "return
encrypted_types_.Equals(encrypted_types);"

If they're not equal, though... what that means is encrypted_types_ is a
superset of encrypted_types.  Why does that mean we "need to update encrypted
types"?

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:675: void
SyncEncryptionHandlerImpl::MergeEncryptedTypes(
I think in this case it'd be better to have 

if (!encrypted_types_.HasAll(encrypted_types)) {
  e_t_ = e_t;
  FOR_EACH_OBSERVER(...)
}

inlined would be clearer and worth the tradeoff.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
File sync/internal_api/sync_encryption_handler_impl.h (right):

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.h:26: // GetEncryptedTypes. Need
to cache explicit passphrase state on the UI thread.
I think it would be useful to have a class comment / ascii art showing the
typical lifetime of this class is, what happens at startup, the fact that all
changes (from the user side and from the sync side) go through this gauntlet,
and which events trigger which functions.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.h:55:
DISALLOW_COPY_AND_ASSIGN(SyncEncryptionHandlerImpl);
nit - put this at the bottom.

http://codereview.chromium.org/10827266/diff/12001/sync/syncable/nigori_handl...
File sync/syncable/nigori_handler.h (right):

http://codereview.chromium.org/10827266/diff/12001/sync/syncable/nigori_handl...
sync/syncable/nigori_handler.h:19: // must be held by a caller whenever invoking
methods.
Convention would be to have a transaction pointer passed in and dchecked.  Is
that infeasible?

http://codereview.chromium.org/10827266/diff/12001/sync/syncable/nigori_handl...
sync/syncable/nigori_handler.h:27: virtual void UpdateFromNigori(const
sync_pb::NigoriSpecifics& nigori) = 0;
I think ApplyNigoriUpdate is a better name here... UpdateFromNigori on a pure
virtual interface doesn't tell you much.  It's also consistent with the
ApplyNigoriUpdate method on SyncEncryptionHandlerImpl, which this winds up
getting to -- maybe that one should be called ApplyNigoriUpdateImpl, and have
that be called by its implementation of ApplyNigoriUpdate (and the ReloadNigori
case).  

I'd also change the name on Cryptographer, so it's easier to connect the
which-code-leads-to-what dots in our brain :)

http://codereview.chromium.org/10827266/diff/12001/sync/syncable/nigori_util.cc
File sync/syncable/nigori_util.cc (right):

http://codereview.chromium.org/10827266/diff/12001/sync/syncable/nigori_util....
sync/syncable/nigori_util.cc:274: encrypted_types.Has(APP_NOTIFICATIONS));
Should there be a compile-assert here on the number of data types?

Also, do we need to have this method on *both* NigoriHandler and as a util here?
 When do you call one and not the other?

http://codereview.chromium.org/10827266/diff/12001/sync/test/fake_sync_encryp...
File sync/test/fake_sync_encryption_handler.cc (right):

http://codereview.chromium.org/10827266/diff/12001/sync/test/fake_sync_encryp...
sync/test/fake_sync_encryption_handler.cc:21: 
nit - extra newline

http://codereview.chromium.org/10827266/diff/12001/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

http://codereview.chromium.org/10827266/diff/12001/sync/util/cryptographer.h#...
sync/util/cryptographer.h:66: void UpdateFromNigori(const
sync_pb::NigoriSpecifics& nigori) ;
nit - extra space before ;

http://codereview.chromium.org/10827266/diff/12001/sync/util/cryptographer.h#...
sync/util/cryptographer.h:151: void SetDefaultKey(std::string key_name);
const string&

[tim (not reviewing), 2012-08-14 02:34:13]

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:609: return false;
On 2012/08/14 02:32:20, timsteele wrote:
> these 3 lines are equivalent to "return
> encrypted_types_.Equals(encrypted_types);"
> 
> If they're not equal, though... what that means is encrypted_types_ is a
> superset of encrypted_types.  Why does that mean we "need to update encrypted
> types"?

Ignore the second part of this comment, I get it now / addressed it with other
comments. First part still applies :)

[Nicolas Zea, 2012-08-14 23:24:51]

PTAL. I added a TODO to add a transaction param to the GetEncryptedTypes method,
since doing that in this patch would introduce substantially more changes.

http://codereview.chromium.org/10827266/diff/12001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10827266/diff/12001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:745: // TODO(zea): cache this
value at the PSS, then make the encryption handler
On 2012/08/14 02:32:20, timsteele wrote:
> Which value?  The result of the whole expression below?  Any reason we
wouldn't
> cache it here instead of PSS (SyncBackendHost)?

Comment updated, you're right, it's better to cache here.

http://codereview.chromium.org/10827266/diff/12001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1143: done_callback.Run();
On 2012/08/14 02:32:20, timsteele wrote:
> I think you can remove all the done_callback plumbing and PostClosure (!), and
> explicitly post to frontend_loop_ here via host_.Call.

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:30: // because the encryption
keys don't match (per chrome instantiation).
On 2012/08/14 02:32:20, timsteele wrote:
> Can you include a link to the bug that led to this or have more of an
> explanation in the comment here? 

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:58: void
SyncEncryptionHandlerImpl::ReloadNigori() {
On 2012/08/14 02:32:20, timsteele wrote:
> Is InitNigori clearer?  Something to get the point that this is only called
> once, at startup, across.

Went with Init, since otherwise it sounds like it's just writing to the nigori
node.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:78: void
SyncEncryptionHandlerImpl::UpdateFromNigori(
On 2012/08/14 02:32:20, timsteele wrote:
> Why not make this take a transaction and dcheck on it?  I ask because if we
did
> that, this call to ApplyNigoriUpdate would be the same as the one above in
> ReloadNigori from a transaction point of view, and we could call
> WriteEncryptionStateToNigori from within ApplyNigoriUpdate itself, meaning it
> could just return void.  The 'bool' return value from ApplyNigoriUpdate is
> pretty confusing, and it seems like that function could perform the fallback
> work itself.  We could also get rid of RewriteNigori.
> 

Rewrite nigori is necessary to open a new _syncapi_ transaction. Otherwise,
you'd reuse the same syncable transaction that apply updates creates, which
would get treated as a server side change.

Went ahead and forced passing the trans as a param though (forgot you could get
the wrapped trans).

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:98: void
SyncEncryptionHandlerImpl::SetEncryptionPassphrase(
On 2012/08/14 02:32:20, timsteele wrote:
> Was this + SetDecryption pretty much just carried over?  If so I won't
> scrutinize line-by-line and save myself from going down this treacherous path
> again... :)

Yep, all the same!

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:453: bool
encrypted_types_need_update = !UpdateEncryptedTypesFromNigori(nigori);
On 2012/08/14 02:32:20, timsteele wrote:
> Something feels... weird here.  The function on the RHS says it is going to
> update encrypted types, and if it returns false, we set a flag noting
"encrypted
> types need update". 
> 
> Maybe remove the ! and make the flag 'encrypted_types_match_nigori'? 
Something
> to note that what needs updating (as you mention below) is nigori, not
> encrypted_types_ (after UpdateEncryptedTypesFromNigori returns).

Renamed to nigori_types_need_update (I'd like to keep all the bools as "need
update"). Also renamed needs_new_keys to nigori_needs_new_keys.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:483: // yet, prompt the user
for a passphrase.
On 2012/08/14 02:32:20, timsteele wrote:
> "... isn't ready yet or has pending keys, ..."

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:485: DVLOG(1) <<
"OnPassPhraseRequired Sent";
On 2012/08/14 02:32:20, timsteele wrote:
> nit - Passphrase

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:499: // If so, we need to
update the nigori node.
On 2012/08/14 02:32:20, timsteele wrote:
> "... If so, we need to add to the set of encrypted types in the nigori node."
is
> clearer / more accurate, I think?

Well, it's also the passphrase/keybag info too, not just the types. Updated the
comment to make it clearer than its the general encryption state.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:503: needs_new_keys)
On 2012/08/14 02:32:20, timsteele wrote:
> nit - { } around multi line ifs.

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:543:
UpdateNigoriFromEncryptedTypes(&nigori);
On 2012/08/14 02:32:20, timsteele wrote:
> Would it be bad to just call the util directly here?
> 
> I'd be just as happy to move the util implementation here, if you wanted to
e.g.
> keep the interface symmetric (UpdateNigoriFromEncrypted +
> UpdateEncryptedFromNigori).

Need to keep it in nigori_util so the fake can use it too. Went ahead and called
the util method here though.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:589:
encrypted_types.Put(APP_NOTIFICATIONS);
On 2012/08/14 02:32:20, timsteele wrote:
> Should there be a compile-assert here on number of data types?  Second, it'd
be
> nice for readability / understandability to pull these ~30 lines into a
> ModelTypeSet ExtractEncryptedTypesFrom(NigoriSpecifics& s); routine.

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:592: // encrypt_everything
field. If anything more than the sensitive types were
On 2012/08/14 02:32:20, timsteele wrote:
> What version was this, again?  At some point I think it'll be fair to deny
> commitss from a particular version, maybe if encryption is requested.

That was m14 or m15 I believe. That said, since we're being defensive against
other parts of the nigori getting corrupted, I'd like to leave this logic in.
I've updated the comment though.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:609: return false;
On 2012/08/14 02:34:13, timsteele wrote:
> On 2012/08/14 02:32:20, timsteele wrote:
> > these 3 lines are equivalent to "return
> > encrypted_types_.Equals(encrypted_types);"
> > 
> > If they're not equal, though... what that means is encrypted_types_ is a
> > superset of encrypted_types.  Why does that mean we "need to update
encrypted
> > types"?
> 
> Ignore the second part of this comment, I get it now / addressed it with other
> comments. First part still applies :)

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:675: void
SyncEncryptionHandlerImpl::MergeEncryptedTypes(
On 2012/08/14 02:32:20, timsteele wrote:
> I think in this case it'd be better to have 
> 
> if (!encrypted_types_.HasAll(encrypted_types)) {
>   e_t_ = e_t;
>   FOR_EACH_OBSERVER(...)
> }
> 
> inlined would be clearer and worth the tradeoff.

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
File sync/internal_api/sync_encryption_handler_impl.h (right):

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.h:26: // GetEncryptedTypes. Need
to cache explicit passphrase state on the UI thread.
On 2012/08/14 02:32:20, timsteele wrote:
> I think it would be useful to have a class comment / ascii art showing the
> typical lifetime of this class is, what happens at startup, the fact that all
> changes (from the user side and from the sync side) go through this gauntlet,
> and which events trigger which functions. 

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.h:55:
DISALLOW_COPY_AND_ASSIGN(SyncEncryptionHandlerImpl);
On 2012/08/14 02:32:20, timsteele wrote:
> nit - put this at the bottom.

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/syncable/nigori_handl...
File sync/syncable/nigori_handler.h (right):

http://codereview.chromium.org/10827266/diff/12001/sync/syncable/nigori_handl...
sync/syncable/nigori_handler.h:19: // must be held by a caller whenever invoking
methods.
On 2012/08/14 02:32:20, timsteele wrote:
> Convention would be to have a transaction pointer passed in and dchecked.  Is
> that infeasible?

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/syncable/nigori_handl...
sync/syncable/nigori_handler.h:27: virtual void UpdateFromNigori(const
sync_pb::NigoriSpecifics& nigori) = 0;
On 2012/08/14 02:32:20, timsteele wrote:
> I think ApplyNigoriUpdate is a better name here... UpdateFromNigori on a pure
> virtual interface doesn't tell you much.  It's also consistent with the
> ApplyNigoriUpdate method on SyncEncryptionHandlerImpl, which this winds up
> getting to -- maybe that one should be called ApplyNigoriUpdateImpl, and have
> that be called by its implementation of ApplyNigoriUpdate (and the
ReloadNigori
> case).  
> 
> I'd also change the name on Cryptographer, so it's easier to connect the
> which-code-leads-to-what dots in our brain :)

Done for first part. What do you mean by change the name on cryptographer
though?

http://codereview.chromium.org/10827266/diff/12001/sync/syncable/nigori_util.cc
File sync/syncable/nigori_util.cc (right):

http://codereview.chromium.org/10827266/diff/12001/sync/syncable/nigori_util....
sync/syncable/nigori_util.cc:274: encrypted_types.Has(APP_NOTIFICATIONS));
On 2012/08/14 02:32:20, timsteele wrote:
> Should there be a compile-assert here on the number of data types?
> 
> Also, do we need to have this method on *both* NigoriHandler and as a util
here?
>  When do you call one and not the other? 

The nigori handler version will eventually call into this version (via
SyncEncryptionHandlerImpl). We have this separate though so that the
FakeSyncEncryptionHandler can use it.

http://codereview.chromium.org/10827266/diff/12001/sync/test/fake_sync_encryp...
File sync/test/fake_sync_encryption_handler.cc (right):

http://codereview.chromium.org/10827266/diff/12001/sync/test/fake_sync_encryp...
sync/test/fake_sync_encryption_handler.cc:21: 
On 2012/08/14 02:32:20, timsteele wrote:
> nit - extra newline

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

http://codereview.chromium.org/10827266/diff/12001/sync/util/cryptographer.h#...
sync/util/cryptographer.h:66: void UpdateFromNigori(const
sync_pb::NigoriSpecifics& nigori) ;
On 2012/08/14 02:32:20, timsteele wrote:
> nit - extra space before ;

Done.

http://codereview.chromium.org/10827266/diff/12001/sync/util/cryptographer.h#...
sync/util/cryptographer.h:151: void SetDefaultKey(std::string key_name);
On 2012/08/14 02:32:20, timsteele wrote:
> const string&

Done.

[tim (not reviewing), 2012-08-15 00:23:30]

http://codereview.chromium.org/10827266/diff/8002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10827266/diff/8002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:1334: AssociateNigori(js_backend);
Can we cache js_backend somewhere in this class to avoid having to thread it
through?  Now that the base::Bind is gone it looks even stranger as a param at
every function in the chain back to here...

http://codereview.chromium.org/10827266/diff/8002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:1513: void
PostClosure(MessageLoop* message_loop,
This can go too, I think!

http://codereview.chromium.org/10827266/diff/8002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10827266/diff/8002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:478: // Must be called on
|frontend_loop_|.  |done_callback| is called on
Comment references |done_callback|.

http://codereview.chromium.org/10827266/diff/8002/sync/engine/syncer_unittest.cc
File sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/10827266/diff/8002/sync/engine/syncer_unittest...
sync/engine/syncer_unittest.cc:259:
cryptographer(&trans)->SetNigoriHandler(&fake_encryption_handler_);
This accessor should really be called GetCryptographer if it takes a param.

http://codereview.chromium.org/10827266/diff/8002/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/10827266/diff/8002/sync/engine/syncer_util.cc#...
sync/engine/syncer_util.cc:220: cryptographer->ApplyNigoriUpdate(nigori, trans);
This was all I meant by "change cryptographer naming as well".  Looks good.

http://codereview.chromium.org/10827266/diff/8002/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10827266/diff/8002/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:73:
WriteEncryptionStateToNigori(&trans);
nit - { } around multi line ifs

http://codereview.chromium.org/10827266/diff/8002/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:78: if
(cryptographer->is_ready())
Comment about what happens if !is_ready

http://codereview.chromium.org/10827266/diff/8002/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:600: void
SyncEncryptionHandlerImpl::UpdateNigoriFromEncryptedTypes(
If we keep the nigori_util version, do we still need this one?

http://codereview.chromium.org/10827266/diff/8002/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

http://codereview.chromium.org/10827266/diff/8002/sync/util/cryptographer.h#n...
sync/util/cryptographer.h:63: // to a NigoriHandler.
This one could probably use a bug to track.

[Nicolas Zea, 2012-08-15 01:08:29]

All comments addressed.

http://codereview.chromium.org/10827266/diff/8002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10827266/diff/8002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:1334: AssociateNigori(js_backend);
On 2012/08/15 00:23:30, timsteele wrote:
> Can we cache js_backend somewhere in this class to avoid having to thread it
> through?  Now that the base::Bind is gone it looks even stranger as a param at
> every function in the chain back to here...

Done.

http://codereview.chromium.org/10827266/diff/8002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:1513: void
PostClosure(MessageLoop* message_loop,
On 2012/08/15 00:23:30, timsteele wrote:
> This can go too, I think!

Done.

http://codereview.chromium.org/10827266/diff/8002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10827266/diff/8002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:478: // Must be called on
|frontend_loop_|.  |done_callback| is called on
On 2012/08/15 00:23:30, timsteele wrote:
> Comment references |done_callback|. 

Done.

http://codereview.chromium.org/10827266/diff/8002/sync/engine/syncer_unittest.cc
File sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/10827266/diff/8002/sync/engine/syncer_unittest...
sync/engine/syncer_unittest.cc:259:
cryptographer(&trans)->SetNigoriHandler(&fake_encryption_handler_);
On 2012/08/15 00:23:30, timsteele wrote:
> This accessor should really be called GetCryptographer if it takes a param.

Done.

http://codereview.chromium.org/10827266/diff/8002/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/10827266/diff/8002/sync/engine/syncer_util.cc#...
sync/engine/syncer_util.cc:220: cryptographer->ApplyNigoriUpdate(nigori, trans);
On 2012/08/15 00:23:30, timsteele wrote:
> This was all I meant by "change cryptographer naming as well".  Looks good.

Done.

http://codereview.chromium.org/10827266/diff/8002/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10827266/diff/8002/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:73:
WriteEncryptionStateToNigori(&trans);
On 2012/08/15 00:23:30, timsteele wrote:
> nit - { } around multi line ifs

Done.

http://codereview.chromium.org/10827266/diff/8002/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:78: if
(cryptographer->is_ready())
On 2012/08/15 00:23:30, timsteele wrote:
> Comment about what happens if !is_ready

Done.

http://codereview.chromium.org/10827266/diff/8002/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:600: void
SyncEncryptionHandlerImpl::UpdateNigoriFromEncryptedTypes(
On 2012/08/15 00:23:30, timsteele wrote:
> If we keep the nigori_util version, do we still need this one?

This one accesses the local state (i.e. encrypted_types_ and
encrypt_everything_). The other one is just a helper method where you have to
provide the actual local state.

http://codereview.chromium.org/10827266/diff/8002/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

http://codereview.chromium.org/10827266/diff/8002/sync/util/cryptographer.h#n...
sync/util/cryptographer.h:63: // to a NigoriHandler.
On 2012/08/15 00:23:30, timsteele wrote:
> This one could probably use a bug to track.

Done.

[tim (not reviewing), 2012-08-15 01:39:02]

This is looking good!  Couple more comments and just finishing reviewing some
tests.

http://codereview.chromium.org/10827266/diff/14001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10827266/diff/14001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1342:
frontend_->OnBackendInitialized(js_backend_, true);
Can / should we reset the member variable here?

http://codereview.chromium.org/10827266/diff/14001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1509: void
SyncBackendHost::AssociateNigori() {
Okay, after all that, I think we should now just inline the PostTask below at
the single callsite.

http://codereview.chromium.org/10827266/diff/14001/sync/internal_api/sync_enc...
File sync/internal_api/sync_encryption_handler_impl_unittest.cc (right):

http://codereview.chromium.org/10827266/diff/14001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:334:
current_nigori_specifics,
nit - indent

http://codereview.chromium.org/10827266/diff/14001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:349: old_nigori,
nit - indent

[Nicolas Zea, 2012-08-15 20:56:49]

http://codereview.chromium.org/10827266/diff/14001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10827266/diff/14001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1342:
frontend_->OnBackendInitialized(js_backend_, true);
On 2012/08/15 01:39:02, timsteele wrote:
> Can / should we reset the member variable here?

Done. Also reset in shutdown now. I went ahead and got rid of the DCHECK that it
exists since it's not actually required we have a js backend at all times
(anything that might use it checks that it's initialized first)

http://codereview.chromium.org/10827266/diff/14001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1509: void
SyncBackendHost::AssociateNigori() {
On 2012/08/15 01:39:02, timsteele wrote:
> Okay, after all that, I think we should now just inline the PostTask below at
> the single callsite.

Done.

http://codereview.chromium.org/10827266/diff/14001/sync/internal_api/sync_enc...
File sync/internal_api/sync_encryption_handler_impl_unittest.cc (right):

http://codereview.chromium.org/10827266/diff/14001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:334:
current_nigori_specifics,
On 2012/08/15 01:39:02, timsteele wrote:
> nit - indent

Done.

http://codereview.chromium.org/10827266/diff/14001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:349: old_nigori,
On 2012/08/15 01:39:02, timsteele wrote:
> nit - indent

Done.

[tim (not reviewing), 2012-08-15 22:26:08]

LGTM

[commit-bot: I haz the power, 2012-08-16 00:42:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10827266/10047

[commit-bot: I haz the power, 2012-08-16 00:57:12]

Try job failure for 10827266-10047 (retry) on win for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-16 01:10:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10827266/169

[commit-bot: I haz the power, 2012-08-16 02:34:18]

Change committed as 151833

[Nicolas Zea, 2012-08-21 19:54:42]

actually, you can hold off on this. Found a significant issue I'll be addressing
a separate patchset.