Separate access token caching logic from signaling connector.

Separate access token caching logic from signaling connector.

Right now signaling connector is the only user of OAuth tokens, but if/when we add more authenticated calls, this will allow them to share the cached access token (so that consecutive authenticated calls from different objects don't need to make separate refreshtoken calls).

BUG=

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

[Sergey Ulanov, 2014-02-08 03:16:35]

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
File remoting/host/oauth_token_getter.cc (right):

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
ditto

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:27: const std::string& login_value,
remove _value suffix for this and the next field or add it for the last one.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:44: }
nit: } can be moved to the previous line.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:57: }
same here

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:99:
callbacks.on_access_token.Run(user_email, oauth_access_token_);
Each callback handler may call this class again and add another callback into
the queue. It might not be problem here, but it will be a problem in error
cases. So instead of copying each separate callback inside the loop (which
doesn't give you anything) you need to move all pending_callbacks_ to a separate
container on stack and call them from there.
As a general callback hygiene measure callback callers should never touch (read
or write) internal state after calling a callback.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:107: while (!pending_callbacks_.empty()) {
If you use single callback for all results then this loop can be moved to a
helper function and not duplicated in three places.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:165: std::vector<std::string>
empty_scope_list;  // (Use scope from refresh token.)
nit: remove parentheses

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
File remoting/host/oauth_token_getter.h (right):

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
lose (c), 2014

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.h:56: const base::Closure& on_auth_error);
Do we need three separate callbacks? Maybe just add another enum parameter in
TokenCallback? That would simplify code. Also  CallWithToken() looks strange
with three callbacks passed to it.

https://codereview.chromium.org/141063009/diff/1/remoting/host/signaling_conn...
File remoting/host/signaling_connector.h (right):

https://codereview.chromium.org/141063009/diff/1/remoting/host/signaling_conn...
remoting/host/signaling_connector.h:43: void EnableOAuth(OAuthTokenGetter*
oauth_token_getter);
side note, not related to this CL: we should try to find out how many host are
out there that are still not using OAuth. If the number is (close to) 0 then we
could remove support for non-oauth auth and fold this method into constructor.

[rmsousa, 2014-02-10 23:07:39]

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
File remoting/host/oauth_token_getter.cc (right):

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/02/08 03:16:36, Sergey Ulanov wrote:
> ditto

Done.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:27: const std::string& login_value,
On 2014/02/08 03:16:36, Sergey Ulanov wrote:
> remove _value suffix for this and the next field or add it for the last one.

Done.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:44: }
On 2014/02/08 03:16:36, Sergey Ulanov wrote:
> nit: } can be moved to the previous line.

Class removed

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:57: }
On 2014/02/08 03:16:36, Sergey Ulanov wrote:
> same here

Done.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:99:
callbacks.on_access_token.Run(user_email, oauth_access_token_);
On 2014/02/08 03:16:36, Sergey Ulanov wrote:
> Each callback handler may call this class again and add another callback into
> the queue. It might not be problem here, but it will be a problem in error
> cases. So instead of copying each separate callback inside the loop (which
> doesn't give you anything) you need to move all pending_callbacks_ to a
separate
> container on stack and call them from there.
> As a general callback hygiene measure callback callers should never touch
(read
> or write) internal state after calling a callback.

Good point. Done.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:107: while (!pending_callbacks_.empty()) {
On 2014/02/08 03:16:36, Sergey Ulanov wrote:
> If you use single callback for all results then this loop can be moved to a
> helper function and not duplicated in three places.

Done.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.cc:165: std::vector<std::string>
empty_scope_list;  // (Use scope from refresh token.)
On 2014/02/08 03:16:36, Sergey Ulanov wrote:
> nit: remove parentheses

Done.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
File remoting/host/oauth_token_getter.h (right):

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/02/08 03:16:36, Sergey Ulanov wrote:
> lose (c), 2014

Done.

https://codereview.chromium.org/141063009/diff/1/remoting/host/oauth_token_ge...
remoting/host/oauth_token_getter.h:56: const base::Closure& on_auth_error);
On 2014/02/08 03:16:36, Sergey Ulanov wrote:
> Do we need three separate callbacks? Maybe just add another enum parameter in
> TokenCallback? That would simplify code. Also  CallWithToken() looks strange
> with three callbacks passed to it.

Done.

https://codereview.chromium.org/141063009/diff/1/remoting/host/signaling_conn...
File remoting/host/signaling_connector.h (right):

https://codereview.chromium.org/141063009/diff/1/remoting/host/signaling_conn...
remoting/host/signaling_connector.h:43: void EnableOAuth(OAuthTokenGetter*
oauth_token_getter);
On 2014/02/08 03:16:36, Sergey Ulanov wrote:
> side note, not related to this CL: we should try to find out how many host are
> out there that are still not using OAuth. If the number is (close to) 0 then
we
> could remove support for non-oauth auth and fold this method into constructor.


Acknowledged.

[Sergey Ulanov, 2014-02-10 23:24:30]

lgtm

https://codereview.chromium.org/141063009/diff/100001/remoting/host/oauth_tok...
File remoting/host/oauth_token_getter.cc (right):

https://codereview.chromium.org/141063009/diff/100001/remoting/host/oauth_tok...
remoting/host/oauth_token_getter.cc:137: if
(oauth_credentials_->is_service_account) {
nit: this can be replaced with a ?: expression

[rmsousa, 2014-02-10 23:31:24]

The CQ bit was checked by rmsousa@chromium.org

[commit-bot: I haz the power, 2014-02-10 23:33:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/141063009/100002

[commit-bot: I haz the power, 2014-02-10 23:59:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/141063009/100002

[commit-bot: I haz the power, 2014-02-11 01:32:19]

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

[commit-bot: I haz the power, 2014-02-11 01:32:20]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps,
check_deps2git, chromedriver_unittests, components_unittests,
content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests,
jingle_unittests, media_unittests, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests,
sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
telemetry_perf_unittests, telemetry_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[rmsousa, 2014-02-11 02:38:11]

The CQ bit was checked by rmsousa@chromium.org

[commit-bot: I haz the power, 2014-02-11 02:40:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/141063009/360002

[commit-bot: I haz the power, 2014-02-11 04:51:57]

Change committed as 250351