sync: Introduce CryptographerProvider interface

sync: Introduce CryptographerProvider interface

Creates a CryptographerProvider interface that can be used to expose the
syncable::Directory's Cryptographer without exposing the directory
itself.  Since the directory's cryptographer is protected by a lock,
this requires the creation of several helper classes to manage the
acquistion of the lock.

This CL includes the interface, an implementationthat exposes the
directory's cryptographer (DirectoryCryptographerProvider) and
a SimpleCryptographerProvider that wraps a non-owned Cryptographer for
testing purposes.

These interfaces will be used to implement and test encryption of
non-blocking sync types.

BUG=351005
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285777

[rlarocque, 2014-07-23 20:33:05]

Here's the first patch on the road to encryption for non-blocking sync.

We should probably talk about the direction of the patch series before we start
committing too much code.  Let's meet today to talk about the overall plan.

[Nicolas Zea, 2014-07-24 22:16:42]

https://codereview.chromium.org/413833002/diff/20001/sync/engine/cryptographe...
File sync/engine/cryptographer_provider.cc (right):

https://codereview.chromium.org/413833002/diff/20001/sync/engine/cryptographe...
sync/engine/cryptographer_provider.cc:41: };
This is the close namespace right? Shouldn't that semicolon generate a compile
warning/error?

also nit: newline above and // namespace syncer

https://codereview.chromium.org/413833002/diff/20001/sync/engine/cryptographe...
File sync/engine/cryptographer_provider.h (right):

https://codereview.chromium.org/413833002/diff/20001/sync/engine/cryptographe...
sync/engine/cryptographer_provider.h:32: class SYNC_EXPORT_PRIVATE
ScopedCryptographer {
I find this naming a bit confusing. ScopedCryptographer to me implies that it
destroys the cryptographer when it goes out of scope (a la scoped_ptr).

How about SharedCryptographer?

https://codereview.chromium.org/413833002/diff/20001/sync/engine/directory_cr...
File sync/engine/directory_cryptographer_provider.cc (right):

https://codereview.chromium.org/413833002/diff/20001/sync/engine/directory_cr...
sync/engine/directory_cryptographer_provider.cc:37: };
here too

[rlarocque, 2014-07-24 22:23:13]

https://codereview.chromium.org/413833002/diff/20001/sync/engine/cryptographe...
File sync/engine/cryptographer_provider.cc (right):

https://codereview.chromium.org/413833002/diff/20001/sync/engine/cryptographe...
sync/engine/cryptographer_provider.cc:41: };
On 2014/07/24 22:16:42, Nicolas Zea wrote:
> This is the close namespace right? Shouldn't that semicolon generate a compile
> warning/error?
> 
> also nit: newline above and // namespace syncer

I think that was from a bad copy/paste.  Fixed.

Spurious semicolons seem to be allowed in C, even outside a block.  I guess that
makes sense.  One less thing to worry about when writing #define macros.

https://codereview.chromium.org/413833002/diff/20001/sync/engine/cryptographe...
File sync/engine/cryptographer_provider.h (right):

https://codereview.chromium.org/413833002/diff/20001/sync/engine/cryptographe...
sync/engine/cryptographer_provider.h:32: class SYNC_EXPORT_PRIVATE
ScopedCryptographer {
On 2014/07/24 22:16:42, Nicolas Zea wrote:
> I find this naming a bit confusing. ScopedCryptographer to me implies that it
> destroys the cryptographer when it goes out of scope (a la scoped_ptr).
> 
> How about SharedCryptographer?

I was trying to get across the idea that the Cryptographer returned with Get()
is only valid as long as this item is on the stack.  I don't think that
SharedCryptographer conveys that same meaning.

Maybe ScopedCryptographerAccessor?  ScopedCryptographerReference?

https://codereview.chromium.org/413833002/diff/20001/sync/engine/directory_cr...
File sync/engine/directory_cryptographer_provider.cc (right):

https://codereview.chromium.org/413833002/diff/20001/sync/engine/directory_cr...
sync/engine/directory_cryptographer_provider.cc:37: };
On 2014/07/24 22:16:42, Nicolas Zea wrote:
> here too

Done.

[Nicolas Zea, 2014-07-24 22:34:19]

https://codereview.chromium.org/413833002/diff/20001/sync/engine/cryptographe...
File sync/engine/cryptographer_provider.h (right):

https://codereview.chromium.org/413833002/diff/20001/sync/engine/cryptographe...
sync/engine/cryptographer_provider.h:32: class SYNC_EXPORT_PRIVATE
ScopedCryptographer {
On 2014/07/24 22:23:13, rlarocque wrote:
> On 2014/07/24 22:16:42, Nicolas Zea wrote:
> > I find this naming a bit confusing. ScopedCryptographer to me implies that
it
> > destroys the cryptographer when it goes out of scope (a la scoped_ptr).
> > 
> > How about SharedCryptographer?
> 
> I was trying to get across the idea that the Cryptographer returned with Get()
> is only valid as long as this item is on the stack.  I don't think that
> SharedCryptographer conveys that same meaning.
> 
> Maybe ScopedCryptographerAccessor?  ScopedCryptographerReference?

I see. Yeah, ScopedCryptographerRef seems better in that case.

[rlarocque, 2014-07-25 17:07:57]

Renamed to ScopedCryptographerRef.

[Nicolas Zea, 2014-07-25 20:03:31]

LGTM with some naming suggestions.

https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
File sync/engine/cryptographer_provider.h (right):

https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
sync/engine/cryptographer_provider.h:29: // A concrete class representing a
cryptographer.
nit: perhaps "representing a cryptographer" should be "representing a
cryptographer reference"

https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
sync/engine/cryptographer_provider.h:54: class SYNC_EXPORT_PRIVATE
ScopedCryptographerImpl {
nit: this is the actual getter, not so much an impl in my mind.
CryptographerGetter might make more sense as a name.

https://codereview.chromium.org/413833002/diff/60001/sync/engine/directory_cr...
File sync/engine/directory_cryptographer_provider.h (right):

https://codereview.chromium.org/413833002/diff/60001/sync/engine/directory_cr...
sync/engine/directory_cryptographer_provider.h:34: class
ScopedDirectoryCryptographerImpl : public ScopedCryptographerImpl {
Along the lines of the previous naming suggestion, I think this would make more
sense as DirectoryCryptographerGetter

https://codereview.chromium.org/413833002/diff/60001/sync/test/engine/simple_...
File sync/test/engine/simple_cryptographer_provider.h (right):

https://codereview.chromium.org/413833002/diff/60001/sync/test/engine/simple_...
sync/test/engine/simple_cryptographer_provider.h:30: class
SimpleScopedCryptographerImpl : public ScopedCryptographerImpl {
SimpleCryptographerGetter?

[rlarocque, 2014-07-25 20:18:17]

https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
File sync/engine/cryptographer_provider.h (right):

https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
sync/engine/cryptographer_provider.h:29: // A concrete class representing a
cryptographer.
On 2014/07/25 20:03:30, Nicolas Zea wrote:
> nit: perhaps "representing a cryptographer" should be "representing a
> cryptographer reference"

Done.

https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
sync/engine/cryptographer_provider.h:54: class SYNC_EXPORT_PRIVATE
ScopedCryptographerImpl {
On 2014/07/25 20:03:30, Nicolas Zea wrote:
> nit: this is the actual getter, not so much an impl in my mind.
> CryptographerGetter might make more sense as a name.

Maybe Impl isn't the right suffix, given how we tend to use it for implementers
of interfaces.  I don't think Getter makes sense either, though.  In the
Directory case, the ScopedCryptographerImpl embeds the scoped transaction.  That
seems like more than a getter.

How about an "Internal(s)" suffix?

[Nicolas Zea, 2014-07-25 20:19:44]

On 2014/07/25 20:18:17, rlarocque wrote:
>
https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
> File sync/engine/cryptographer_provider.h (right):
> 
>
https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
> sync/engine/cryptographer_provider.h:29: // A concrete class representing a
> cryptographer.
> On 2014/07/25 20:03:30, Nicolas Zea wrote:
> > nit: perhaps "representing a cryptographer" should be "representing a
> > cryptographer reference"
> 
> Done.
> 
>
https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
> sync/engine/cryptographer_provider.h:54: class SYNC_EXPORT_PRIVATE
> ScopedCryptographerImpl {
> On 2014/07/25 20:03:30, Nicolas Zea wrote:
> > nit: this is the actual getter, not so much an impl in my mind.
> > CryptographerGetter might make more sense as a name.
> 
> Maybe Impl isn't the right suffix, given how we tend to use it for
implementers
> of interfaces.  I don't think Getter makes sense either, though.  In the
> Directory case, the ScopedCryptographerImpl embeds the scoped transaction. 
That
> seems like more than a getter.
> 
> How about an "Internal(s)" suffix?

Sure, Internal makes sense.

[rlarocque, 2014-07-25 20:59:54]

On 2014/07/25 20:19:44, Nicolas Zea wrote:
> On 2014/07/25 20:18:17, rlarocque wrote:
> >
>
https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
> > File sync/engine/cryptographer_provider.h (right):
> > 
> >
>
https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
> > sync/engine/cryptographer_provider.h:29: // A concrete class representing a
> > cryptographer.
> > On 2014/07/25 20:03:30, Nicolas Zea wrote:
> > > nit: perhaps "representing a cryptographer" should be "representing a
> > > cryptographer reference"
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/413833002/diff/60001/sync/engine/cryptographe...
> > sync/engine/cryptographer_provider.h:54: class SYNC_EXPORT_PRIVATE
> > ScopedCryptographerImpl {
> > On 2014/07/25 20:03:30, Nicolas Zea wrote:
> > > nit: this is the actual getter, not so much an impl in my mind.
> > > CryptographerGetter might make more sense as a name.
> > 
> > Maybe Impl isn't the right suffix, given how we tend to use it for
> implementers
> > of interfaces.  I don't think Getter makes sense either, though.  In the
> > Directory case, the ScopedCryptographerImpl embeds the scoped transaction. 
> That
> > seems like more than a getter.
> > 
> > How about an "Internal(s)" suffix?
> 
> Sure, Internal makes sense.

Done.

[rlarocque, 2014-07-25 21:00:44]

The CQ bit was checked by rlarocque@chromium.org

[Nicolas Zea, 2014-07-25 21:01:21]

lgtm

[commit-bot: I haz the power, 2014-07-25 21:02:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/413833002/80001

[commit-bot: I haz the power, 2014-07-25 23:46:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-07-26 18:34:36]

Change committed as 285777