Add SyncAuthProvider interface and implementation

sync/internal_api/public/base/sync_auth_provider.h

Index: sync/internal_api/public/base/sync_auth_provider.h
diff --git a/sync/internal_api/public/base/sync_auth_provider.h b/sync/internal_api/public/base/sync_auth_provider.h
new file mode 100644
index 0000000000000000000000000000000000000000..99843082cff70175d80a0f149f555777ad82a1c1
--- /dev/null
+++ b/sync/internal_api/public/base/sync_auth_provider.h
@@ -0,0 +1,32 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SYNC_INTERNAL_API_PUBLIC_BASE_SYNC_AUTH_PROVIDER_H_

[tim (not reviewing), 2014/04/29 22:12:11]

It was helpful to see and consider this class on its own in this review, thanks.
In the other CL amidst all the files, it seemed like adding the DEPS include of
/public was wrong (which it is), however I don't think the fix is to move this
to /base after all.

SyncAuthProvider is how ServerConnectionManager requests auth tokens from the
embedder.  Just like HttpPostProviderInterface is how ServerConnectionManager
makes network requests through the embedder.

These interfaces / classes are both internal_api specific implementation
details; that is, they are driven by the needs of internal_api to proxy back and
forth with the embedder.  To the extent possible, there is a goal to have code
in sync/engine (and sync/syncable, sync/sessions, etc) operate without any
knowledge of the embedder or the fact that different embeddings exist, and focus
only on sync protocol specifics.

This is why SyncapiServerConnectionManager exists. It handles the
embedder-specific parts of the ServerConnectionManager contract.  I think the
right design would be to move the SyncAuthProvider to internal_api/public, and
move all use of it to SyncapiServerConnectionManager to match HttpPostProvider.

[Note, we could alternatively decide that this aforementioned goal is overkill
and eliminate SyncapiServerConnectionManager by pushing it down to SCM and  move
the internal_api dependencies from internal_api/public to internal_api/base. So
far I remain not in favor of this option.]

[pavely, 2014/04/30 19:46:40]

I'll move this file to internal_api/public. On the next change I'll see if it
breaks anything. Worst case, I'll just move it once more.


On 2014/04/29 22:12:11, timsteele wrote:

+#define SYNC_INTERNAL_API_PUBLIC_BASE_SYNC_AUTH_PROVIDER_H_
+
+#include "base/callback.h"
+
+class GoogleServiceAuthError;
+
+namespace syncer {
+
+// SyncAuthProvider is interface to access token related functions from sync
+// engine.
+class SyncAuthProvider {

[tim (not reviewing), 2014/04/29 22:12:11]

Not for this CL, but I think we should combine this into
syncer::NetworkResources, and pass a single object into Init for {HttpPost,
SyncAuth}Provider.  We want to stop adding to this endless list of Init
parameters, and there's even more reason to do so now with multiple SyncManager
implementations. 

So after this CL lands, perhaps a quick one to change the SyncManager::Init
signature, and then it'll be easy to add SAP to all the SyncManagers in the
third CL?

[pavely, 2014/04/30 19:46:40]

I'll look into this for my next change.
On 2014/04/29 22:12:11, timsteele wrote:

+ public:
+  typedef base::Callback<void(const GoogleServiceAuthError& error,
+                              const std::string& token)> RequestTokenCallback;
+
+  virtual ~SyncAuthProvider() {}
+
+  // Request access token for sync. Callback will be called with error and
+  // access token. If error is anything other than NONE then token is invalid.
+  virtual void RequestAccessToken(const RequestTokenCallback& callback) = 0;

[tim (not reviewing), 2014/04/29 22:12:11]

newline after this.

[pavely, 2014/04/30 19:46:40]

Done.

+  // Invalidate access token that was rejected by sync server.
+  virtual void InvalidateAccessToken(const std::string& token) = 0;
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_INTERNAL_API_PUBLIC_BASE_SYNC_AUTH_PROVIDER_H_