Refactor OAuth2TokenService to have profile- and device-based implementations.

Refactor OAuth2TokenService to have profile- and device-based implementations.

OAuth2TokenService was refactored into an abstract base class without
dependencies on Profile and the TokenService's OAuth2 login refresh token.

Added ProfileOAuth2TokenService as an implementation of OAuth2TokenService
that requires a Profile and the user's login refresh token from
TokenService. Migrated the existing OAuth2TokenServiceFactory and
OAuth2TokenServiceRequest to Profile* varients that use this new class.

Added DeviceOAuth2TokenService that serves up API access tokens for
enterprise enrolled devices (via a robot account with an any-api refresh
token). These tokens are global to all accounts on the device including
Public Accounts, and hence don't depend on a particular profile's login
credentials.

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

[David Roche, 2013-03-16 22:33:48]

Andrew, here is the new CL.

Looks like the trybots are still having issues with this CL, but at least the
status is valid for each file.  The unit tests pass locally...

[Andrew T Wilson (Slow), 2013-03-19 19:58:43]

LGTM after addressing nits/comments below.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:41:
g_browser_process->local_state()->SetString(kRobotAnyApiRefreshTokenPrefKey,
Consider injecting the PrefRegistry in the constructor instead of hard-coding
local_state().

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:34: virtual
~DeviceOAuth2TokenService();
Hrm. constructor is private, but destructor is public? Maybe make this private
too?

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:38: void
SaveRefreshToken(const std::string& refresh_token);
This is fine as-is, but consider making this "SetRefreshToken()" or
"SetAndSaveRefreshToken()" to make it clear it's a setter.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:26:
friend struct base::DefaultLazyInstanceTraits<
This is fine, but consider moving it to the top of the private: section which
seems more common.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service.cc:75: consumer_->
OnGetTokenSuccess(this, access_token, expiration_date);
nit: please remove the space after -> here too.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service.cc:224: // The caching may be skipped
if ShouldCacheForRefreshToken() returns
stale comment: ShouldCacheForRefreshToken() does not exist.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service.h:84: // is not deleted.  The allowed
scopes may vary -- see subclass comments.
I think this comment is stale - the allowed scopes don't vary.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service.h:92: // For tests
Remove "For tests" and instead describe what this function does. Also, make this
const.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service.h:93: int cache_size();
nit: rename "cache_size_for_testing()"

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
File chrome/browser/signin/oauth2_token_service_test_util.cc (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service_test_util.cc:24: last_token_ = token;
consider just tracking tokens in a vector, that way tests can see all tokens and
just count the size of the vector.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service_test_util.cc:31: last_error_ = error;
consider tracking errors in a vector rather than counting num errors separately
and only tracking the last.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
File chrome/browser/signin/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service_unittest.cc:66: void
refresh_token(const std::string& refresh_token) {
call setters something like "set_refresh_token()". Or just inject in
constructor.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service_unittest.cc:79:
OAuth2TokenServiceTest() {
This is fine, but consider not editing this line in your patch to keep the file
history cleaner.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service_unittest.cc:87: new
net::TestURLRequestContextGetter(
Does this leak the TestURLRequestContextGetter() when the test completes?

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/prof...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/prof...
chrome/browser/signin/profile_oauth2_token_service.cc:64: return "";
return std::string();

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/prof...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/prof...
chrome/browser/signin/profile_oauth2_token_service.h:94: bool
ShouldCacheForRefreshToken(TokenService *token_service,
nit: declare member functions before data members.

[David Roche, 2013-03-21 00:12:02]

Also addressed comments that mnissler made in CL 12759002.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:34: virtual
~DeviceOAuth2TokenService();
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> Hrm. constructor is private, but destructor is public? Maybe make this private
> too?

Added comment that it's public due to use of scoped_ptr in factory, as
discussed.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:38: void
SaveRefreshToken(const std::string& refresh_token);
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> This is fine as-is, but consider making this "SetRefreshToken()" or
> "SetAndSaveRefreshToken()" to make it clear it's a setter.

Done.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:26:
friend struct base::DefaultLazyInstanceTraits<
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> This is fine, but consider moving it to the top of the private: section which
> seems more common.

Done.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service.cc:75: consumer_->
OnGetTokenSuccess(this, access_token, expiration_date);
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> nit: please remove the space after -> here too.

Done.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service.cc:224: // The caching may be skipped
if ShouldCacheForRefreshToken() returns
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> stale comment: ShouldCacheForRefreshToken() does not exist.

Fixed, thx.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service.h:84: // is not deleted.  The allowed
scopes may vary -- see subclass comments.
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> I think this comment is stale - the allowed scopes don't vary.

Removed, thx.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service.h:92: // For tests
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> Remove "For tests" and instead describe what this function does. Also, make
this
> const.

Done.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service.h:93: int cache_size();
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> nit: rename "cache_size_for_testing()"

Done.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
File chrome/browser/signin/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service_unittest.cc:66: void
refresh_token(const std::string& refresh_token) {
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> call setters something like "set_refresh_token()". Or just inject in
> constructor.

Done.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service_unittest.cc:79:
OAuth2TokenServiceTest() {
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> This is fine, but consider not editing this line in your patch to keep the
file
> history cleaner.

Done.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/oaut...
chrome/browser/signin/oauth2_token_service_unittest.cc:87: new
net::TestURLRequestContextGetter(
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> Does this leak the TestURLRequestContextGetter() when the test completes?

Done.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/prof...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/prof...
chrome/browser/signin/profile_oauth2_token_service.cc:64: return "";
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> return std::string();

Done.

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/prof...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/12647008/diff/2001/chrome/browser/signin/prof...
chrome/browser/signin/profile_oauth2_token_service.h:94: bool
ShouldCacheForRefreshToken(TokenService *token_service,
On 2013/03/19 19:58:44, Andrew T Wilson wrote:
> nit: declare member functions before data members.

Done.

[Mattias Nissler (ping if slow), 2013-03-21 06:59:15]

https://codereview.chromium.org/12647008/diff/20001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
(right):

https://codereview.chromium.org/12647008/diff/20001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:36:
g_browser_process->system_request_context()));
Ha, so the factory design is broken here after all. You're actually depending on
g_browser_process being initialized already, which breaks the general
expectation towards singletons that they're stand-alone and without
dependencies.

I suggest you look into whether we can hook the DeviceOAuth2TokenService
instance off g_browser_process or something similar.

[David Roche, 2013-03-21 20:36:31]

https://codereview.chromium.org/12647008/diff/20001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
(right):

https://codereview.chromium.org/12647008/diff/20001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:36:
g_browser_process->system_request_context()));
On 2013/03/21 06:59:15, Mattias Nissler wrote:
> Ha, so the factory design is broken here after all. You're actually depending
on
> g_browser_process being initialized already, which breaks the general
> expectation towards singletons that they're stand-alone and without
> dependencies.
> 
> I suggest you look into whether we can hook the DeviceOAuth2TokenService
> instance off g_browser_process or something similar.

The factory singleton doesn't have an external dependency.  The Get() method
that returns an instance of the DeviceOAuth2TokenService has a dependency on the
IO thread being created.  Moving this to g_browser_process wouldn't change that
requirement: it would simply be moved into a lazy factory method that can't be
invoked until the IO thread is up.

Indeed, the global browser process singleton has this exact same problem in many
of its methods: the g_browser_process pointer value is set to a valid value in
its constructor, but many of its factory methods would fail if called
immediately.  Instead they depend on the fact that they won't be invoked until
after the browser has come up, the threads have been started, etc.

I'm not sure that moving this into g_browser_process really buys us anything
here.  Perhaps it would be better to simply document the requirement on the
IOThread, and have the Get() method return null if the IOThread isn't up yet? 
This would behave the same as the g_browser_process implementation, but would
avoid having to extend that monolithic global object with yet-more CrOS-only
logic.

[David Roche, 2013-03-26 01:42:23]

Moved DeviceOAuth2TokenService factory method to g_browser_process, per offline
discussion.  Mattias, PTAL.

[Mattias Nissler (ping if slow), 2013-03-26 12:04:32]

I'm mostly good with this, so LGTM pending the cleanups.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:447: return NULL;
Are you sure about this change? Previously, we crashed reliably here, which at
least got us crash reports. Now, we may no longer crash immediately but
somewhere else instead, which will make bugs harder to track down.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:472: return NULL;
I think we should just CHECK() here so this code crashes when called in the
wrong order.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:473: if
(!device_oauth2_token_service_.get())
Style guide requires curlies around multi-line conditional bodies.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:42:
g_browser_process->local_state()->GetString(
Seems like the local state pointer could be passed in as well here now that
we're hosted in g_browser_process. Less globals, better testability.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:227: // tasks.
https://chromiumcodereview.appspot.com/11312124/
nit: Can we rather put the commit revision / bug number as a reference here as
the more canonical identifier for a bug / code change?

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:311: :
request_context_getter_(getter) {
nit: 4 space indentation

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service_test_util.h (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_test_util.h:24: 
nit: It is comment convention to place a comment before overriden member
functions stating which class you implement, e.g.:

// OAuth2TokenService::Consumer:

Also, we then usually omit newlines between the declarations  of a member
functions in a block of overrides.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_test_util.h:33: int
number_of_correct_tokens_;
nit: maybe |number_of_successful_tokens_| would be a more accurate name?

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_unittest.cc:27: : public
OAuth2TokenService::Consumer {
drive-by-nit: Seems like this class is mostly redundant and could be simplified
by deriving from TestingOAuth2ServiceConsumer.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_unittest.cc:63: :
OAuth2TokenService(getter) {
nit: 4 spaces indentation.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_unittest.cc:64: }
nit: newline

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_unittest.cc:68: }
nit: newline

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_unittest.cc:72: }
nit: newline:

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:5: #include
"base/stringprintf.h"
nit: not needed

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:10: #include
"chrome/browser/signin/token_service_factory.h"
needed?

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:12: #include
"chrome/common/chrome_notification_types.h"
not needed?

Please clean up includes in this file.

[David Roche, 2013-03-27 05:09:51]

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:447: return NULL;
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> Are you sure about this change? Previously, we crashed reliably here, which at
> least got us crash reports. Now, we may no longer crash immediately but
> somewhere else instead, which will make bugs harder to track down.

Okay, I'll revert this.  I made the change based on the fact that the
browser_process.h documentation for this method says that it can return null.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:472: return NULL;
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> I think we should just CHECK() here so this code crashes when called in the
> wrong order.

Okay, in that case I can just leave this check out, and it will crash in
system_request_context when it accesses the null io_thread() result.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:473: if
(!device_oauth2_token_service_.get())
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> Style guide requires curlies around multi-line conditional bodies.

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:42:
g_browser_process->local_state()->GetString(
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> Seems like the local state pointer could be passed in as well here now that
> we're hosted in g_browser_process. Less globals, better testability.

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:227: // tasks.
https://chromiumcodereview.appspot.com/11312124/
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> nit: Can we rather put the commit revision / bug number as a reference here as
> the more canonical identifier for a bug / code change?

I linked the code review because the comments there have an extensive discussion
of this issue.  Linking to the commit revision would be missing this info and
therefore be much harder to understand.  There was no bug associated with this
code review.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:311: :
request_context_getter_(getter) {
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> nit: 4 space indentation

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service_test_util.h (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_test_util.h:24: 
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> nit: It is comment convention to place a comment before overriden member
> functions stating which class you implement, e.g.:
> 
> // OAuth2TokenService::Consumer:
> 
> Also, we then usually omit newlines between the declarations  of a member
> functions in a block of overrides.

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_test_util.h:33: int
number_of_correct_tokens_;
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> nit: maybe |number_of_successful_tokens_| would be a more accurate name?

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_unittest.cc:27: : public
OAuth2TokenService::Consumer {
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> drive-by-nit: Seems like this class is mostly redundant and could be
simplified
> by deriving from TestingOAuth2ServiceConsumer.

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_unittest.cc:63: :
OAuth2TokenService(getter) {
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> nit: 4 spaces indentation.

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_unittest.cc:64: }
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> nit: newline

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_unittest.cc:68: }
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> nit: newline

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service_unittest.cc:72: }
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> nit: newline:

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:5: #include
"base/stringprintf.h"
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> nit: not needed

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:10: #include
"chrome/browser/signin/token_service_factory.h"
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/12647008/diff/27001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service_unittest.cc:12: #include
"chrome/common/chrome_notification_types.h"
On 2013/03/26 12:04:32, Mattias Nissler wrote:
> not needed?
> 
> Please clean up includes in this file.

Done.

[David Roche, 2013-03-27 21:41:16]

brettw: can you please do an OWNERS review for the chrome/browser/ and
chrome/browser/history/ changes?  Thanks!

[brettw, 2013-03-28 20:18:32]

https://codereview.chromium.org/12647008/diff/8002/chrome/browser/browser_pro...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/12647008/diff/8002/chrome/browser/browser_pro...
chrome/browser/browser_process.h:121: virtual
chromeos::DeviceOAuth2TokenService* device_oauth2_token_service() = 0;
We're trying to kill BrowserProcess. If your class is a singleton, just make it
a Singleton using that infrastructure.

https://codereview.chromium.org/12647008/diff/8002/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/12647008/diff/8002/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:22: class
Profile;
Your CL description and this code references Profile, but you don't seem to use
it. Are these references out-of-date?

[Mattias Nissler (ping if slow), 2013-03-28 21:02:12]

On Mar 28, 2013 9:18 PM, <brettw@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/12647008/diff/8002/chrome/browser/browser_pro...
> File chrome/browser/browser_process.h (right):
>
>
https://codereview.chromium.org/12647008/diff/8002/chrome/browser/browser_pro...
> chrome/browser/browser_process.h:121: virtual
> chromeos::DeviceOAuth2TokenService* device_oauth2_token_service() = 0;
> We're trying to kill BrowserProcess. If your class is a singleton, just
> make it a Singleton using that infrastructure.

lol

He had that before and I pushed for browser process because of the
dependencies to local state and system request context and the shutdown
order problems if you reference them from singletons. Do we have a solution
for that?

>
>
https://codereview.chromium.org/12647008/diff/8002/chrome/browser/chromeos/se...
> File chrome/browser/chromeos/settings/device_oauth2_token_service.h
> (right):
>
>
https://codereview.chromium.org/12647008/diff/8002/chrome/browser/chromeos/se...
> chrome/browser/chromeos/settings/device_oauth2_token_service.h:22: class
> Profile;
> Your CL description and this code references Profile, but you don't seem
> to use it. Are these references out-of-date?
>
> https://codereview.chromium.org/12647008/

[brettw, 2013-03-28 23:46:35]

I think the way we're going with the extensions service and other things is to
have a desctruction observer for the stuff that you don't want to outlive.

I believe the request contexts implement SupportsUserData. John suggested adding
an adapter in there that notifies you when it gets deleted (which will happen
when the context is deleted). If we need this more often I would be supportive
of having a more explicit interface for observing destruction, but for now this
seems like an OK way to get what you want.

[David Roche, 2013-04-02 00:04:08]

Brett, 

It doesn't look like the request context implements SupportsUserData.  The
dependencies that DeviceOAuth2TokenService has is on local state and the global
system request context.  Would an acceptable alternative be to listen for
NOTIFICATION_APP_TERMINATING and do cleanup then (cancel all in-flight requests
and set a flag to disallow future requests)?

Note that users of DeviceOAuth2TokenService receive a Request object that they
can delete in order to cancel the request.  So the cleanup we're talking about
here is for the edge case where clients live too long or somehow leak Requests.

[Mattias Nissler (ping if slow), 2013-04-02 13:17:19]

On 2013/04/02 00:04:08, David Roche wrote:
> Brett, 
> 
> It doesn't look like the request context implements SupportsUserData.  The
> dependencies that DeviceOAuth2TokenService has is on local state and the
global
> system request context.  Would an acceptable alternative be to listen for
> NOTIFICATION_APP_TERMINATING and do cleanup then (cancel all in-flight
requests
> and set a flag to disallow future requests)?
> 
> Note that users of DeviceOAuth2TokenService receive a Request object that they
> can delete in order to cancel the request.  So the cleanup we're talking about
> here is for the edge case where clients live too long or somehow leak
Requests.

Another option I can see would be to switch back to using a singleton and then
making explicit Initialize()/Shutdown() calls from
ChromeBrowserMainPartsChromeOS.

[David Roche, 2013-04-03 01:20:28]

Okay, I removed the g_browser_process integration per Brett's comments, and
added a factory that is explicitly initialized and shutdown per Mattias'
suggestion.  PTAL.

[Mattias Nissler (ping if slow), 2013-04-03 11:33:16]

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
(right):

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:28:
return factory.GetImpl();
It seems like a simpler implementation could be to avoid creating a
DeviceOAuth2TokenServiceFactory instances at all and just keeping a global
g_device_oauth2_token_service_ pointer that Get()/Initialize()/Shutdown()
maintains.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:56: if
(device_oauth2_token_service_.get()) {
nit: no need for curlies here.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:57:
device_oauth2_token_service_->Shutdown();
Shouldn't we just destroy the object here?

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:24: //
TODO: Move back...
Where?

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc
(right):

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:49: 
remove stray blank line.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:53:
InSequence s;  // Enforces that expected calls are in order.
Is this really necessary? I think this makes the test overly restrictive.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:68:
DeviceOAuth2TokenService oauth2_service(
indentation

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:28: // tokens.  Concrete subclasses
should implement GetRefreshToken to return
nitty nit: This file seems to use only one space before a new sentence starts.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:97: void Shutdown();
Can't this just be done in the dtor?

[David Roche, 2013-04-03 16:34:09]

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
(right):

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:28:
return factory.GetImpl();
On 2013/04/03 11:33:16, Mattias Nissler wrote:
> It seems like a simpler implementation could be to avoid creating a
> DeviceOAuth2TokenServiceFactory instances at all and just keeping a global
> g_device_oauth2_token_service_ pointer that Get()/Initialize()/Shutdown()
> maintains.

Done.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:56: if
(device_oauth2_token_service_.get()) {
On 2013/04/03 11:33:16, Mattias Nissler wrote:
> nit: no need for curlies here.

Done.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:57:
device_oauth2_token_service_->Shutdown();
On 2013/04/03 11:33:16, Mattias Nissler wrote:
> Shouldn't we just destroy the object here?

Done.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:24: //
TODO: Move back...
On 2013/04/03 11:33:16, Mattias Nissler wrote:
> Where?

To private: after I figured out why the friend class declaration wasn't working
(typo, with no compiler error ... fun).

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc
(right):

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:49: 
On 2013/04/03 11:33:16, Mattias Nissler wrote:
> remove stray blank line.

Done.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:53:
InSequence s;  // Enforces that expected calls are in order.
On 2013/04/03 11:33:16, Mattias Nissler wrote:
> Is this really necessary? I think this makes the test overly restrictive.

Done.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:68:
DeviceOAuth2TokenService oauth2_service(
On 2013/04/03 11:33:16, Mattias Nissler wrote:
> indentation

Done.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:28: // tokens.  Concrete subclasses
should implement GetRefreshToken to return
On 2013/04/03 11:33:16, Mattias Nissler wrote:
> nitty nit: This file seems to use only one space before a new sentence starts.

Done.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:97: void Shutdown();
On 2013/04/03 11:33:16, Mattias Nissler wrote:
> Can't this just be done in the dtor?

I was concerned about clients that were holding the pointer from the Factory's
Get() method, and then called StartRequest during shutdown.  But, I think you're
right here ... we need to destroy this instance in the
DeviceOAuth2TokenServiceFactory::Shutdown() method, or we'll leak memory on
every browser restart.  I've updated the docs for Get() to say that the returned
pointer shouldn't be stored, and null should be checked for.

[Mattias Nissler (ping if slow), 2013-04-04 13:11:11]

LGTM w/ nits.

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/12647008/diff/47001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:97: void Shutdown();
On 2013/04/03 16:34:09, David Roche wrote:
> On 2013/04/03 11:33:16, Mattias Nissler wrote:
> > Can't this just be done in the dtor?
> 
> I was concerned about clients that were holding the pointer from the Factory's
> Get() method, and then called StartRequest during shutdown.  But, I think
you're
> right here ... we need to destroy this instance in the
> DeviceOAuth2TokenServiceFactory::Shutdown() method, or we'll leak memory on
> every browser restart.  I've updated the docs for Get() to say that the
returned
> pointer shouldn't be stored, and null should be checked for.

Well, lifetime management isn't simple in C++ :)

https://codereview.chromium.org/12647008/diff/68002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/12647008/diff/68002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:27:
virtual ~DeviceOAuth2TokenServiceFactory();
no need for the virtual, in fact you can drop the dtor entirely.

https://codereview.chromium.org/12647008/diff/68002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc
(right):

https://codereview.chromium.org/12647008/diff/68002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:31:
local_state_(scoped_testing_local_state_.Get()),
any reason to keep a separate pointer member as a shortcut given that you use it
only once?

https://codereview.chromium.org/12647008/diff/68002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:46:
MessageLoop message_loop_;
message_loop_ and ui_thread_ should be first, so they get destroyed last.

[David Roche, 2013-04-04 14:52:48]

brettw: I removed the g_browser_process method and created a separate factory
class.  Can you take a look for OWNERS on these files?

    chrome/browser/history/DEPS
    chrome/browser/history/web_history_service.cc
    chrome/browser/chromeos/chrome_browser_main_chromeos.cc
    chrome/chrome_browser.gypi
    chrome/chrome_browser_chromeos.gypi

Thanks!

https://codereview.chromium.org/12647008/diff/68002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/12647008/diff/68002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:27:
virtual ~DeviceOAuth2TokenServiceFactory();
On 2013/04/04 13:11:11, Mattias Nissler wrote:
> no need for the virtual, in fact you can drop the dtor entirely.

Done.

https://codereview.chromium.org/12647008/diff/68002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc
(right):

https://codereview.chromium.org/12647008/diff/68002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:31:
local_state_(scoped_testing_local_state_.Get()),
On 2013/04/04 13:11:11, Mattias Nissler wrote:
> any reason to keep a separate pointer member as a shortcut given that you use
it
> only once?

Done.

https://codereview.chromium.org/12647008/diff/68002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:46:
MessageLoop message_loop_;
On 2013/04/04 13:11:11, Mattias Nissler wrote:
> message_loop_ and ui_thread_ should be first, so they get destroyed last.

Done.

[brettw, 2013-04-05 21:58:03]

LGTM, this integration looks better to me. (Previously, I got resource contexts
and requests contexts confused which led to my bad advice.)

https://codereview.chromium.org/12647008/diff/56002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/12647008/diff/56002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:30: //
available (local state and request context getter).
Can you state the requirements for when shutdown should be called in the comment
here?

[commit-bot: I haz the power, 2013-04-09 00:45:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/12647008/86001

[commit-bot: I haz the power, 2013-04-09 01:02:37]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-04-11 18:22:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/12647008/120001

[commit-bot: I haz the power, 2013-04-11 18:54:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-11 22:23:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/12647008/140001

[commit-bot: I haz the power, 2013-04-12 03:05:49]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-04-12 04:01:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/12647008/111034

[commit-bot: I haz the power, 2013-04-12 04:44:24]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[David Roche, 2013-04-16 18:02:37]

@dubroy: Patrick, you recently integrated Android support into
OAuth2TokenService.  I've refactored OAuth2TokenService to support
profile-scoped tokens and device-scoped tokens (representing a Chrome OS device,
even when no user is logged in).

I initially moved your ifdef'd Android code into ProfileOAuth2TokenService, but
became convinced that this wasn't the right way to do it when fixing the tests. 
With my refactored OAuth2TokenService base class, it's pretty easy to create an
Android-specific specialization of the ProfileOAuth2TokenService, so that is
what I did.  Benefits:

1. The code is simpler (only a single ifdef in the factory)
2. The android implementation is completely different than the non-android impl.
 Having this in a separate class makes this clear, and also makes it clear that
there are there are no associated unit tests for the android-specific bits.

Another nice side-effect of the new OAuth2TokenService base class is that the
base class tests can be re-enabled on Android (only the few tests specific to
ProfileOAuth2TokenService need to remain disabled, since that portion of the
implementation still doesn't make sense on Android).

I've verified that the tests compile and pass on Android.  I've never done any
Chrome for Android development, so I haven't gone through the process of
actually building and running Chrome for Android with these changes.  Can you
take a look and let me know if you think it's okay to submit as-is, or if you
think I should test anything else?

In particular, you'll want to looks at the new AndroidProfileOAuth2TokenService
class and the changes to the factory to return it on Android:

https://codereview.chromium.org/12647008/diff/174001/chrome/browser/signin/an...
https://codereview.chromium.org/12647008/diff2/111034:174001/chrome/browser/s...

[Patrick Dubroy, 2013-04-16 18:20:44]

Awesome! Thanks a lot for doing this. The changes I made for Android were
definitely a hack to get things working, but this is much nicer.

The two parts you linked to lgtm. If you don't mind waiting a day to land this,
I can test on Android first thing tomorrow.

On 2013/04/16 18:02:37, David Roche wrote:
> @dubroy: Patrick, you recently integrated Android support into
> OAuth2TokenService.  I've refactored OAuth2TokenService to support
> profile-scoped tokens and device-scoped tokens (representing a Chrome OS
device,
> even when no user is logged in).
> 
> I initially moved your ifdef'd Android code into ProfileOAuth2TokenService,
but
> became convinced that this wasn't the right way to do it when fixing the
tests. 
> With my refactored OAuth2TokenService base class, it's pretty easy to create
an
> Android-specific specialization of the ProfileOAuth2TokenService, so that is
> what I did.  Benefits:
> 
> 1. The code is simpler (only a single ifdef in the factory)
> 2. The android implementation is completely different than the non-android
impl.
>  Having this in a separate class makes this clear, and also makes it clear
that
> there are there are no associated unit tests for the android-specific bits.
> 
> Another nice side-effect of the new OAuth2TokenService base class is that the
> base class tests can be re-enabled on Android (only the few tests specific to
> ProfileOAuth2TokenService need to remain disabled, since that portion of the
> implementation still doesn't make sense on Android).
> 
> I've verified that the tests compile and pass on Android.  I've never done any
> Chrome for Android development, so I haven't gone through the process of
> actually building and running Chrome for Android with these changes.  Can you
> take a look and let me know if you think it's okay to submit as-is, or if you
> think I should test anything else?
> 
> In particular, you'll want to looks at the new
AndroidProfileOAuth2TokenService
> class and the changes to the factory to return it on Android:
> 
>
https://codereview.chromium.org/12647008/diff/174001/chrome/browser/signin/an...
>
https://codereview.chromium.org/12647008/diff2/111034:174001/chrome/browser/s...

[davidroche, 2013-04-16 18:27:44]

Sure, I can wait a day to commit.  Thanks!


On Tue, Apr 16, 2013 at 11:20 AM, <dubroy@chromium.org> wrote:

> Awesome! Thanks a lot for doing this. The changes I made for Android were
> definitely a hack to get things working, but this is much nicer.
>
> The two parts you linked to lgtm. If you don't mind waiting a day to land
> this,
> I can test on Android first thing tomorrow.
>
>
> On 2013/04/16 18:02:37, David Roche wrote:
>
>> @dubroy: Patrick, you recently integrated Android support into
>> OAuth2TokenService.  I've refactored OAuth2TokenService to support
>> profile-scoped tokens and device-scoped tokens (representing a Chrome OS
>>
> device,
>
>> even when no user is logged in).
>>
>
>  I initially moved your ifdef'd Android code into
>> ProfileOAuth2TokenService,
>>
> but
>
>> became convinced that this wasn't the right way to do it when fixing the
>>
> tests.
>
>> With my refactored OAuth2TokenService base class, it's pretty easy to
>> create
>>
> an
>
>> Android-specific specialization of the ProfileOAuth2TokenService, so that
>> is
>> what I did.  Benefits:
>>
>
>  1. The code is simpler (only a single ifdef in the factory)
>> 2. The android implementation is completely different than the non-android
>>
> impl.
>
>>   Having this in a separate class makes this clear, and also makes it
>> clear
>>
> that
>
>> there are there are no associated unit tests for the android-specific
>> bits.
>>
>
>  Another nice side-effect of the new OAuth2TokenService base class is that
>> the
>> base class tests can be re-enabled on Android (only the few tests
>> specific to
>> ProfileOAuth2TokenService need to remain disabled, since that portion of
>> the
>> implementation still doesn't make sense on Android).
>>
>
>  I've verified that the tests compile and pass on Android.  I've never
>> done any
>> Chrome for Android development, so I haven't gone through the process of
>> actually building and running Chrome for Android with these changes.  Can
>> you
>> take a look and let me know if you think it's okay to submit as-is, or if
>> you
>> think I should test anything else?
>>
>
>  In particular, you'll want to looks at the new
>>
> AndroidProfileOAuth2TokenServi**ce
>
>> class and the changes to the factory to return it on Android:
>>
>
>
> https://codereview.chromium.**org/12647008/diff/174001/**
>
chrome/browser/signin/android_**profile_oauth2_token_service.h<https://codereview.chromium.org/12647008/diff/174001/chrome/browser/signin/android_profile_oauth2_token_service.h>
>
> https://codereview.chromium.**org/12647008/diff2/111034:**
>
174001/chrome/browser/signin/**profile_oauth2_token_service_**factory.cc<https://codereview.chromium.org/12647008/diff2/111034:174001/chrome/browser/signin/profile_oauth2_token_service_factory.cc>
>
>
>
>
https://codereview.chromium.**org/12647008/<https://codereview.chromium.org/1...
>

[Patrick Dubroy, 2013-04-17 11:15:37]

On 2013/04/16 18:27:44, davidroche_google.com wrote:
> Sure, I can wait a day to commit.  Thanks!

Ok, I gave this a quick test on Android and everything appears to work. Commit
away!
 
> On Tue, Apr 16, 2013 at 11:20 AM, <mailto:dubroy@chromium.org> wrote:
> 
> > Awesome! Thanks a lot for doing this. The changes I made for Android were
> > definitely a hack to get things working, but this is much nicer.
> >
> > The two parts you linked to lgtm. If you don't mind waiting a day to land
> > this,
> > I can test on Android first thing tomorrow.
> >
> >
> > On 2013/04/16 18:02:37, David Roche wrote:
> >
> >> @dubroy: Patrick, you recently integrated Android support into
> >> OAuth2TokenService.  I've refactored OAuth2TokenService to support
> >> profile-scoped tokens and device-scoped tokens (representing a Chrome OS
> >>
> > device,
> >
> >> even when no user is logged in).
> >>
> >
> >  I initially moved your ifdef'd Android code into
> >> ProfileOAuth2TokenService,
> >>
> > but
> >
> >> became convinced that this wasn't the right way to do it when fixing the
> >>
> > tests.
> >
> >> With my refactored OAuth2TokenService base class, it's pretty easy to
> >> create
> >>
> > an
> >
> >> Android-specific specialization of the ProfileOAuth2TokenService, so that
> >> is
> >> what I did.  Benefits:
> >>
> >
> >  1. The code is simpler (only a single ifdef in the factory)
> >> 2. The android implementation is completely different than the non-android
> >>
> > impl.
> >
> >>   Having this in a separate class makes this clear, and also makes it
> >> clear
> >>
> > that
> >
> >> there are there are no associated unit tests for the android-specific
> >> bits.
> >>
> >
> >  Another nice side-effect of the new OAuth2TokenService base class is that
> >> the
> >> base class tests can be re-enabled on Android (only the few tests
> >> specific to
> >> ProfileOAuth2TokenService need to remain disabled, since that portion of
> >> the
> >> implementation still doesn't make sense on Android).
> >>
> >
> >  I've verified that the tests compile and pass on Android.  I've never
> >> done any
> >> Chrome for Android development, so I haven't gone through the process of
> >> actually building and running Chrome for Android with these changes.  Can
> >> you
> >> take a look and let me know if you think it's okay to submit as-is, or if
> >> you
> >> think I should test anything else?
> >>
> >
> >  In particular, you'll want to looks at the new
> >>
> > AndroidProfileOAuth2TokenServi**ce
> >
> >> class and the changes to the factory to return it on Android:
> >>
> >
> >
> > https://codereview.chromium.**org/12647008/diff/174001/**
> >
>
chrome/browser/signin/android_**profile_oauth2_token_service.h<https://codereview.chromium.org/12647008/diff/174001/chrome/browser/signin/android_profile_oauth2_token_service.h>
> >
> > https://codereview.chromium.**org/12647008/diff2/111034:**
> >
>
174001/chrome/browser/signin/**profile_oauth2_token_service_**factory.cc<https://codereview.chromium.org/12647008/diff2/111034:174001/chrome/browser/signin/profile_oauth2_token_service_factory.cc>
> >
> >
> >
> >
>
https://codereview.chromium.**org/12647008/%3Chttps://codereview.chromium.org...>
> >

[commit-bot: I haz the power, 2013-04-18 01:39:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/12647008/188002

[commit-bot: I haz the power, 2013-04-18 06:02:20]

Change committed as 194796