[Sync] Refactor GetEncryptedTypes usage.

[Sync] Refactor GetEncryptedTypes usage.

The cryptographer is now owned by the SyncEncryptionHandlerImpl, and
no longer needs any connection to the nigori handler or encrypted types.
Those are accessed via the directory, which now has a GetNigoriHandler
method.

Because of this, we can now clean up the thread safety guarantees in the
sync encryption handler impl, enforcing that everything except
GetEncryptedTypes (and IsUsingExplicitPassphrase, which will be fixed in a
followup patch) are invoked on the syncer thread. This lets us not have to
open unnecessary transactions.

At the chrome layer, encrypted types are accessed via BaseTransaction::
GetEncryptedTypes(). At the syncer layer, they're accessed via directory::
GetNigoriHandler()->GetEncryptedTypes(BaseTransaction* trans const).

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

[Nicolas Zea, 2012-08-21 01:13:18]

PTAL. Most of this is just call site changes and test related stuff.

[tim (not reviewing), 2012-08-23 18:38:09]

https://chromiumcodereview.appspot.com/10844005/diff/12001/sync/internal_api/...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/10844005/diff/12001/sync/internal_api/...
sync/internal_api/sync_encryption_handler_impl.cc:424:
OnCryptographerStateChanged(
Should we bubble the trans up through this observer? To make it obvious, and to
avoid observers shooting themselves in the foot (by doing something like we
cleverly avoid above by posting a task)?

https://chromiumcodereview.appspot.com/10844005/diff/12001/sync/internal_api/...
File sync/internal_api/sync_encryption_handler_impl.h (right):

https://chromiumcodereview.appspot.com/10844005/diff/12001/sync/internal_api/...
sync/internal_api/sync_encryption_handler_impl.h:32: class TransactionalHolder {
I don't think this needs to be exposed outside of the class below.

Also does the templating buy us much?  It requires that you don't access the
wrong member variable (since the unsafe_ ones are exposed as well). 

How about having a private struct (call it Vault, or something, like we used to
have in SyncerThread) that houses all of the unsafe_ variables, and then having
an internal accessor to get at the struct that performs the DCHECKs you want. It
still has the downside of "you might access illegally", but the paradigm of
going through an accessor with DCHECKs is a bit more common / obvious.

[Nicolas Zea, 2012-08-23 22:49:32]

PTAL

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

http://codereview.chromium.org/10844005/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.cc:424:
OnCryptographerStateChanged(
On 2012/08/23 18:38:10, timsteele wrote:
> Should we bubble the trans up through this observer? To make it obvious, and
to
> avoid observers shooting themselves in the foot (by doing something like we
> cleverly avoid above by posting a task)?

We'd have to do that with most other observer methods too. I think it's just
safer to state up front that all observer methods are assumed to be called while
a transaction is open. I don't feel too strongly either way though.

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

http://codereview.chromium.org/10844005/diff/12001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.h:32: class TransactionalHolder {
On 2012/08/23 18:38:10, timsteele wrote:
> I don't think this needs to be exposed outside of the class below.
> 
> Also does the templating buy us much?  It requires that you don't access the
> wrong member variable (since the unsafe_ ones are exposed as well). 
> 
> How about having a private struct (call it Vault, or something, like we used
to
> have in SyncerThread) that houses all of the unsafe_ variables, and then
having
> an internal accessor to get at the struct that performs the DCHECKs you want.
It
> still has the downside of "you might access illegally", but the paradigm of
> going through an accessor with DCHECKs is a bit more common / obvious.

Yeah, I guess they're pretty much the same. Done.

[tim (not reviewing), 2012-08-24 19:07:03]

Make sure you add the DCHECK to UnlockVaultMutable as well (and get a green test
run :P)

LGTM

[Nicolas Zea, 2012-08-24 20:27:38]

Done! Committing.

[commit-bot: I haz the power, 2012-08-24 20:27:58]

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

[commit-bot: I haz the power, 2012-08-24 22:54:21]

Change committed as 153335