Add SyncAuthProvider interface and implementation

Add SyncAuthProvider interface and implementation

SyncAuthProvider declares interface for sync engine to request and
invalidate access token.

SyncThreadProxy is very trivial implementation of SyncAuthProvider
interface, the purpose is to post calls to UI thread without worrying
that ProfileSyncAuthProvider will be invalid.

ProfileSyncAuthProvider invokes corresponding functions from ProfileOAuth2TokenService.

BUG=259913
R=tim@chromium.org

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

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

https://codereview.chromium.org/250943003/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_auth_provider.cc (right):

https://codereview.chromium.org/250943003/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_auth_provider.cc:44: DetachFromThread();
Explain this?

https://codereview.chromium.org/250943003/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_auth_provider.cc:72: :
OAuth2TokenService::Consumer("sync_auth_provider"),
nit - make string a constant.

https://codereview.chromium.org/250943003/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_auth_provider.h (right):

https://codereview.chromium.org/250943003/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_auth_provider.h:26: class SyncThreadProxy;
This should be private:.

https://codereview.chromium.org/250943003/diff/1/sync/internal_api/public/bas...
File sync/internal_api/public/base/sync_auth_provider.h (right):

https://codereview.chromium.org/250943003/diff/1/sync/internal_api/public/bas...
sync/internal_api/public/base/sync_auth_provider.h:5: #ifndef
SYNC_INTERNAL_API_PUBLIC_BASE_SYNC_AUTH_PROVIDER_H_
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.]

https://codereview.chromium.org/250943003/diff/1/sync/internal_api/public/bas...
sync/internal_api/public/base/sync_auth_provider.h:16: class SyncAuthProvider {
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?

https://codereview.chromium.org/250943003/diff/1/sync/internal_api/public/bas...
sync/internal_api/public/base/sync_auth_provider.h:25: virtual void
RequestAccessToken(const RequestTokenCallback& callback) = 0;
newline after this.

[pavely, 2014-04-30 19:46:39]

https://codereview.chromium.org/250943003/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_auth_provider.cc (right):

https://codereview.chromium.org/250943003/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_auth_provider.cc:44: DetachFromThread();
SyncThreadProxy is created on UI thread but used only on sync thread.
ThreadCheckerImpl remembers which thread it was constructed on.
DetachFromThread is the way to rebind it to different thread.

I'll add comment.
On 2014/04/29 22:12:11, timsteele wrote:
> Explain this?

https://codereview.chromium.org/250943003/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_auth_provider.cc:72: :
OAuth2TokenService::Consumer("sync_auth_provider"),
On 2014/04/29 22:12:11, timsteele wrote:
> nit - make string a constant.

All other calls to OAuth2TokenService::Consumer just pass string literal. The
reason is that this string is only used to display token requests on
about:signin-internals. It is never used multiple times in the code or compared
to other values or used as key into collection.

I can still change it to constant if you want. Let me know.

https://codereview.chromium.org/250943003/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_auth_provider.h (right):

https://codereview.chromium.org/250943003/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_auth_provider.h:26: class SyncThreadProxy;
On 2014/04/29 22:12:11, timsteele wrote:
> This should be private:.

Done.

https://codereview.chromium.org/250943003/diff/1/sync/internal_api/public/bas...
File sync/internal_api/public/base/sync_auth_provider.h (right):

https://codereview.chromium.org/250943003/diff/1/sync/internal_api/public/bas...
sync/internal_api/public/base/sync_auth_provider.h:5: #ifndef
SYNC_INTERNAL_API_PUBLIC_BASE_SYNC_AUTH_PROVIDER_H_
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:
> 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.]

https://codereview.chromium.org/250943003/diff/1/sync/internal_api/public/bas...
sync/internal_api/public/base/sync_auth_provider.h:16: class SyncAuthProvider {
I'll look into this for my next change.
On 2014/04/29 22:12:11, timsteele wrote:
> 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?

https://codereview.chromium.org/250943003/diff/1/sync/internal_api/public/bas...
sync/internal_api/public/base/sync_auth_provider.h:25: virtual void
RequestAccessToken(const RequestTokenCallback& callback) = 0;
On 2014/04/29 22:12:11, timsteele wrote:
> newline after this.

Done.

[tim (not reviewing), 2014-04-30 20:52:41]

https://codereview.chromium.org/250943003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_auth_provider.cc (right):

https://codereview.chromium.org/250943003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_auth_provider.cc:45: // Detach NonThreadSafe
from UI thread so that it can reattach on first
finish sentence.

https://codereview.chromium.org/250943003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_auth_provider.cc:119:
callback_task_runner_->PostTask(
It's possible this response hits the client (SCM) after the client issues a
subsequent call to SyncThreadProxy::RequestAccessToken. Which means the client
will later get the response for the subsequent call with neither having been
explicitly cancelled by PSAP.

Is that correct? If so, do we need to bother returning REQUEST_CANCELLED at all
above?

[pavely, 2014-04-30 21:16:55]

https://codereview.chromium.org/250943003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_auth_provider.cc (right):

https://codereview.chromium.org/250943003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_auth_provider.cc:45: // Detach NonThreadSafe
from UI thread so that it can reattach on first
On 2014/04/30 20:52:41, timsteele wrote:
> finish sentence.

Done.

https://codereview.chromium.org/250943003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_auth_provider.cc:119:
callback_task_runner_->PostTask(
On 2014/04/30 20:52:41, timsteele wrote:
> It's possible this response hits the client (SCM) after the client issues a
> subsequent call to SyncThreadProxy::RequestAccessToken. Which means the client
> will later get the response for the subsequent call with neither having been
> explicitly cancelled by PSAP.
> 
> Is that correct? If so, do we need to bother returning REQUEST_CANCELLED at
all
> above?

The model I'm trying to follow is: For each RequestToken call there will be
exactly one callback invocation. The reason is the same, easier to reason about
what can or cannot happen in consumer code.

PSAP only keeps reference to last token request. If first request didn't finish
then starting second request will reset access_token_request_ and therefore no
callback from OAuth2TokenService will be called with regards to first request.
Responding with REQUEST_CANCELLED in this case is the way to follow above model.

[tim (not reviewing), 2014-04-30 21:59:08]

LGTM

[pavely, 2014-04-30 22:00:02]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:01:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/250943003/40001

[commit-bot: I haz the power, 2014-04-30 22:01:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:01:33]

Failed to apply patch for sync/sync_internal_api.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file sync/sync_internal_api.gypi
  Hunk #1 FAILED at 99.
  Hunk #2 FAILED at 135.
  2 out of 2 hunks FAILED -- saving rejects to file
sync/sync_internal_api.gypi.rej

Patch:       sync/sync_internal_api.gypi
Index: sync/sync_internal_api.gypi
diff --git a/sync/sync_internal_api.gypi b/sync/sync_internal_api.gypi
index
2e677fe494669cf3f1413e39893b3bf75b879ff0..e0c6a477e7aed55bb27dec872b1a9b053226e163
100644
--- a/sync/sync_internal_api.gypi
+++ b/sync/sync_internal_api.gypi
@@ -99,6 +99,7 @@
     'internal_api/public/sessions/model_neutral_state.h',
     'internal_api/public/sessions/sync_session_snapshot.cc',
     'internal_api/public/sessions/sync_session_snapshot.h',
+    'internal_api/public/sync_auth_provider.h',
     'internal_api/public/sync_core_proxy.h',
     'internal_api/public/sync_encryption_handler.cc',
     'internal_api/public/sync_encryption_handler.h',
@@ -135,9 +136,9 @@
     'internal_api/sync_manager_impl.cc',
     'internal_api/sync_manager_impl.h',
     'internal_api/sync_rollback_manager_base.cc',
-    'internal_api/sync_rollback_manager_base.h',    
+    'internal_api/sync_rollback_manager_base.h',
     'internal_api/sync_rollback_manager.cc',
-    'internal_api/sync_rollback_manager.h',    
+    'internal_api/sync_rollback_manager.h',
     'internal_api/syncapi_internal.cc',
     'internal_api/syncapi_internal.h',
     'internal_api/syncapi_server_connection_manager.cc',

[pavely, 2014-04-30 22:22:00]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:23:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/250943003/60001

[commit-bot: I haz the power, 2014-05-01 05:27:58]

Change committed as 267446