Convert UserPolicySigninService to use OAuth2TokenService

Convert UserPolicySigninService to use OAuth2TokenService

Updated UserPolicySigninService to use OAuth2TokenService and refactored the
Android code to allow the android implementation to be shared with desktop.

Added a FakeProfileOAuth2TokenService and added associated refactorings of
OAuth2TokenService to support it.

Updated various unit tests to use new FakeProfileOAuth2TokenService instead of
rolling their own mocks.

Updated TestingProfile::Builder to enable setting testing factories before any
services are created - this is required because UserPolicySigninService is
instantiated at profile creation time, and this means there is no way to inject
a custom instance.

Updated TestingProfile::Builder to support setting a TestingProfile as incognito
at construction time. This is required because otherwise you have a situation
where services will be created at profile creation time, that should not exist
for incognito profiles.

BUG=265831
TBR=estade,isherman

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

[Andrew T Wilson (Slow), 2013-08-16 16:37:59]

mnissler, please review browser/policy (since joaodasilva is out)
fgorski, please review browser/signin
rsimha, PTAL at browser/sync
erg, PTAL at browser/profiles and the changes under chrome/test/base

https://codereview.chromium.org/23068005/diff/1/chrome/browser/extensions/men...
File chrome/browser/extensions/menu_manager_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/1/chrome/browser/extensions/men...
chrome/browser/extensions/menu_manager_unittest.cc:89: TestingProfile profile_;
Moved this down because otherwise it would be destroyed *after* the various
threads are destroyed.

[awong, 2013-08-16 17:53:06]

drive-by with a bunch of little nits.

FYI, I did the review from bottom up so if the comments read a little out of
order, that is why.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:98:
profile_->SetOffTheRecordProfile(scoped_ptr<Profile>(signin_profile_));
make_scoped_ptr perhaps?

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_prefs_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_prefs_unittest.cc:160:
scoped_ptr<TestingProfile> login_profile_owner = builder.Build();
scoped_ptr<TestingProfile> login_profile_owner(builder.Build());

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/content_se...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:159:
scoped_ptr<TestingProfile> otr_profile = otr_profile_builder.Build();
scoped_ptr<TestingProfile> otr_profile(otr_profile_builder.Build());

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc
(right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc:242:
token_helper_ = login_token_helper.PassAs<TokenHelper>();
I thought PassAs<> was only needed when calling a function. Am I wrong?

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:92: 
nit: spurious newline

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:113:
ASSERT_TRUE(!HasPendingRequest());
hmm...I wonder if the original author expected this to actually abort the whole
test on failure rather than just exit this single function...

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_destroyer_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_destroyer_unittest.cc:18:
scoped_ptr<PrefServiceSyncable>().Pass,
erp? Does this compile?

scoped_ptr<PrefServiceSyncable>() should work w/o the Pass()

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:46: const
GoogleServiceAuthError&, const std::string&, const base::Time&)>
nit: passing base::Time by value is oft preferred to by reference since it's
effectively a primitive int.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:97: const
std::string& username =
nit: you rely on RVO on line 110 but use a const & here...make consistent?

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/tok...
File chrome/browser/signin/token_service_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/tok...
chrome/browser/signin/token_service_unittest.cc:86: return
scoped_ptr<TestingProfile>(new TestingProfile());
not sure it's cleaner, but fyi people sometimes like

return make_scoped_ptr(new TetingProfile());

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_session_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_session_unittest.cc:220:
scoped_ptr<TestingProfile> profile = builder.Build().Pass();
scoped_ptr<TestingProfile> profile(builder.Build());
?

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:108: return
builder.Build().Pass();
I don't think you need the .Pass() here.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller_unittest.cc:297:
scoped_ptr<TestingProfile> profile2 = builder.Build();
nit: initialize like
  scoped_ptr<TestingProfile> profile2(builder.Build());

to avoid even the possibility of extra temporaries, etc.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:381: 
nit: spurious newline.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_unittest.cc:329:
GetProfile()->SetOffTheRecordProfile(builder.Build().Pass());
nit: you shouldn't need this. Pass().

[Elliot Glaysher, 2013-08-16 18:07:58]

https://codereview.chromium.org/23068005/diff/98001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/test/base/testing_...
chrome/test/base/testing_profile.h:87:
BrowserContextKeyedServiceFactory::FactoryFunction callback);
I'm fine with you adding this part of the change, where a set of factories can
be injected before a TestingProfile is created. However, (and this may just may
be me getting confused in a giant patch), it seems like you're adding both
options discussed in the email: you're adding a way to inject testing factories
before profiles are created, and you're letting the user set a global factory.
Is there a reason you can't handle the global cases with this?

https://codereview.chromium.org/23068005/diff/98001/components/browser_contex...
File
components/browser_context_keyed_service/browser_context_dependency_manager.cc
(right):

https://codereview.chromium.org/23068005/diff/98001/components/browser_contex...
components/browser_context_keyed_service/browser_context_dependency_manager.cc:58:
"BrowserContextDependencyManager::CreateBrowserContextServices")
Update trace name too?

https://codereview.chromium.org/23068005/diff/98001/components/browser_contex...
File
components/browser_context_keyed_service/browser_context_keyed_service_factory.h
(right):

https://codereview.chromium.org/23068005/diff/98001/components/browser_contex...
components/browser_context_keyed_service/browser_context_keyed_service_factory.h:46:
void SetGlobalTestingFactory(FactoryFunction factory);
(Other half of code commented on.)

[fgorski, 2013-08-16 18:41:06]

browser/signin lgtm with a few nits.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/fak...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.h:50: //
OnRefreshTokensLoaded() will always be invoked (so tests can mimic a
Could this comment be a bit clearer?
I understand that testing for a missing refresh token should be done by passing
an empty token, but what is the OnRefreshTokensLoaded() portion for?

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.h:59: // request from
our queue of pending requests).
nit: I get why you make the remove part of the comment, but a thought that
stroke me was: No it doesn't (remove from our queue), unless you access the
queue through |GetPendingRequests|.
And I get that that is the public interface.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.h:60: void
IssueTokenForScope(const ScopeSet& scope,
nit: s/scope/scopes/

Applies elsewhere in this class.

[Andrew T Wilson (Slow), 2013-08-19 12:15:56]

Albert, thanks for the drive-by!

erg: PTAL

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:98:
profile_->SetOffTheRecordProfile(scoped_ptr<Profile>(signin_profile_));
On 2013/08/16 17:53:16, awong wrote:
> make_scoped_ptr perhaps?
The API takes a scoped_ptr<Profile> not a scoped_ptr<TestingProfile> so
make_scoped_ptr() doesn't help.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_prefs_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_prefs_unittest.cc:160:
scoped_ptr<TestingProfile> login_profile_owner = builder.Build();
On 2013/08/16 17:53:16, awong wrote:
> scoped_ptr<TestingProfile> login_profile_owner(builder.Build());

Done, but is there a reason why this is better? I personally prefer the a = b
syntax for variable initialization, but I'm happy to be convinced that I'm Doing
It Wrong.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/content_se...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:159:
scoped_ptr<TestingProfile> otr_profile = otr_profile_builder.Build();
On 2013/08/16 17:53:16, awong wrote:
> scoped_ptr<TestingProfile> otr_profile(otr_profile_builder.Build());

Done.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc
(right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc:242:
token_helper_ = login_token_helper.PassAs<TokenHelper>();
Ah, I can use Pass() here. Thx.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:92: 
On 2013/08/16 17:53:16, awong wrote:
> nit: spurious newline

Done.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:113:
ASSERT_TRUE(!HasPendingRequest());
On 2013/08/16 17:53:16, awong wrote:
> hmm...I wonder if the original author expected this to actually abort the
whole
> test on failure rather than just exit this single function...

Yeah, he probably expects it to abort the whole test. This whole class is going
to go away in the next month or so (get folded into FakeProfileOAuthTokenService
once the base APIs support multi-login) so I'm not too concerned.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_destroyer_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_destroyer_unittest.cc:18:
scoped_ptr<PrefServiceSyncable>().Pass,
On 2013/08/16 17:53:16, awong wrote:
> erp? Does this compile?
> 
> scoped_ptr<PrefServiceSyncable>() should work w/o the Pass()
Fixed.
Turns out that this file isn't actually compiled in currently - I've filed a bug
and mad@ is working on fixing the bitrot in these tests.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/and...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:46: const
GoogleServiceAuthError&, const std::string&, const base::Time&)>
On 2013/08/16 17:53:16, awong wrote:
> nit: passing base::Time by value is oft preferred to by reference since it's
> effectively a primitive int.

This callback basically matches
OAuth2TokenService::RequestImpl::InformConsumer(), so I'm not going to change
the API in this CL.

At some point we'll change RequestImpl to generate its own Callback and not
expose InformConsumer() and we can tweak the code then.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/and...
chrome/browser/signin/android_profile_oauth2_token_service.cc:97: const
std::string& username =
On 2013/08/16 17:53:16, awong wrote:
> nit: you rely on RVO on line 110 but use a const & here...make consistent?

Done.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/fak...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.h:50: //
OnRefreshTokensLoaded() will always be invoked (so tests can mimic a
On 2013/08/16 18:41:07, Filip Gorski wrote:
> Could this comment be a bit clearer?
> I understand that testing for a missing refresh token should be done by
passing
> an empty token, but what is the OnRefreshTokensLoaded() portion for?
Rewrote this - hopefully it's clearer.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.h:59: // request from
our queue of pending requests).
On 2013/08/16 18:41:07, Filip Gorski wrote:
> nit: I get why you make the remove part of the comment, but a thought that
> stroke me was: No it doesn't (remove from our queue), unless you access the
> queue through |GetPendingRequests|.
> And I get that that is the public interface. 

You're right - I wrote the comment before writing the code and now it's stale -
fixed.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/fak...
chrome/browser/signin/fake_profile_oauth2_token_service.h:60: void
IssueTokenForScope(const ScopeSet& scope,
On 2013/08/16 18:41:07, Filip Gorski wrote:
> nit: s/scope/scopes/
> 
> Applies elsewhere in this class.

Done.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/tok...
File chrome/browser/signin/token_service_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/signin/tok...
chrome/browser/signin/token_service_unittest.cc:86: return
scoped_ptr<TestingProfile>(new TestingProfile());
On 2013/08/16 17:53:16, awong wrote:
> not sure it's cleaner, but fyi people sometimes like
> 
> return make_scoped_ptr(new TetingProfile());

It's smaller, which is good enough for me :) Done.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_session_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_session_unittest.cc:220:
scoped_ptr<TestingProfile> profile = builder.Build().Pass();
On 2013/08/16 17:53:16, awong wrote:
> scoped_ptr<TestingProfile> profile(builder.Build());
> ?

Done.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:108: return
builder.Build().Pass();
On 2013/08/16 17:53:16, awong wrote:
> I don't think you need the .Pass() here.

Done.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller_unittest.cc:297:
scoped_ptr<TestingProfile> profile2 = builder.Build();
On 2013/08/16 17:53:16, awong wrote:
> nit: initialize like
>   scoped_ptr<TestingProfile> profile2(builder.Build());
> 
> to avoid even the possibility of extra temporaries, etc.

Done.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:381: 
On 2013/08/16 17:53:16, awong wrote:
> nit: spurious newline.

Done.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_unittest.cc:329:
GetProfile()->SetOffTheRecordProfile(builder.Build().Pass());
On 2013/08/16 17:53:16, awong wrote:
> nit: you shouldn't need this. Pass().

Done.

https://codereview.chromium.org/23068005/diff/98001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/test/base/testing_...
chrome/test/base/testing_profile.h:87:
BrowserContextKeyedServiceFactory::FactoryFunction callback);
On 2013/08/16 18:07:59, Elliot Glaysher wrote:
> I'm fine with you adding this part of the change, where a set of factories can
> be injected before a TestingProfile is created. However, (and this may just
may
> be me getting confused in a giant patch), it seems like you're adding both
> options discussed in the email: you're adding a way to inject testing
factories
> before profiles are created, and you're letting the user set a global factory.
> Is there a reason you can't handle the global cases with this?

So I don't actually like SetGlobalTestingFactory() since it has the potential to
leak to other tests as you pointed out, but since I'd already written it and
it's potentially useful for BrowserTests (which don't use TestingProfile) I left
it in.

I'm happy to remove SetGlobalTestingFactory() if you like (let me know - I'm not
using it in this CL), but I'd like to keep
TestingProfile::Builder::AddTestingFactory() since I think that's actually a
much cleaner way to inject factories than either SetGlobalTestingFactory() or
adding them after the profile is created.

https://codereview.chromium.org/23068005/diff/98001/components/browser_contex...
File
components/browser_context_keyed_service/browser_context_dependency_manager.cc
(right):

https://codereview.chromium.org/23068005/diff/98001/components/browser_contex...
components/browser_context_keyed_service/browser_context_dependency_manager.cc:58:
"BrowserContextDependencyManager::CreateBrowserContextServices")
On 2013/08/16 18:07:59, Elliot Glaysher wrote:
> Update trace name too?

Done.

https://codereview.chromium.org/23068005/diff/98001/components/browser_contex...
File
components/browser_context_keyed_service/browser_context_keyed_service_factory.h
(right):

https://codereview.chromium.org/23068005/diff/98001/components/browser_contex...
components/browser_context_keyed_service/browser_context_keyed_service_factory.h:46:
void SetGlobalTestingFactory(FactoryFunction factory);
On 2013/08/16 18:07:59, Elliot Glaysher wrote:
> (Other half of code commented on.)

So, yeah, I'm happy to revert this part of my CL (adding
Set/ResetGlobalTestingFactory()) if you don't think it's valuable. For unit
tests, I think it's far superior to use TestingProfile::Builder to inject
dependencies - as it stands, this code is unused in this CL.

[Andrew T Wilson (Slow), 2013-08-19 13:11:38]

finnur: PTAL at extensions/*
bauerb: PTAL at managed_mode/*
dbeam: PTAL at ui/autofill
sky: PTAL at ui/* and also if you could OWNERS stamp test/base

[Andrew T Wilson (Slow), 2013-08-19 13:12:36]

Actually added dbeam/sky to the reviewers list :)

[Finnur, 2013-08-19 13:19:12]

Extensions LGTM.

[Bernhard Bauer, 2013-08-19 14:00:05]

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc
(right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:29:
const char kOAuth2RefreshToken[] = "refreshtoken";
Could you move this after kManagedUserToken?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:172:
ASSERT_GE(1u, requests.size());
This asserts that 1 >= requests.size(). I think you want the other way around?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:175:
EXPECT_EQ(1u, scopes.count(GaiaUrls::GetInstance()->oauth1_login_scope()));
Would it make sense to extract the code up until here (which is shared with the
method below) into a method that just verifies the state?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:39: std::string
client_id;
Make the member variables private?

[Mattias Nissler (ping if slow), 2013-08-19 14:03:21]

Lots of comments :)

This review should really be split into a TestingProfile refactor part and the
ProfileOAuth2TokenService changes. git is your friend :)

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc
(right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc:28: class
CloudPolicyClientRegistrationHelper::TokenHelper {
Empty base class? Why?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service.cc:39:
ProfileOAuth2TokenServiceFactory::GetForProfile(profile);
Can we inject this pointer from the factory via a ctor parameter? This helps
prevent cycles in the dependency graph, because the PKSF can't grab pointers for
not-yet-created services.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service.cc:67:
ProfileOAuth2TokenServiceFactory::GetForProfile(profile());
pass pointer via ctor (also below)?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service.cc:120:
SigninManagerFactory::GetForProfile(profile());
ditto, can we pass the pointer via ctor (also below)?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:94: //
usernames.
I'd love to see this! Do we have a tracking bug that we can reference here?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:95: class
FakeAndroidProfileOAuth2TokenService :
nit: colon goes on next line.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/an...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/an...
chrome/browser/signin/android_profile_oauth2_token_service.cc:67: scopes);
nit: This doesn't seem to require any line breaks?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/an...
chrome/browser/signin/android_profile_oauth2_token_service.cc:98:
GetAuthenticatedUsername();
Shouldn't this require listing SigninManager as a dependency in the
ProfileOAuth2TokenServiceFactory then (I'm not seeing it declared there)?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
File chrome/browser/signin/fake_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:118:
pending_requests_.push_back(pending_request);
I don't see any place in the code where elements get removed from
pending_requests_. Is that intentional? I guess CompleteRequests() should remove
them?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:3: // found in the
LICENSE file.
newline

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:42:
base::WeakPtr<RequestImpl> request;
#include "base/memory/weak_ptr.h"

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:68: 
nit: if no newline on line #63, then none here either.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:87: virtual
net::URLRequestContextGetter* GetRequestContext() OVERRIDE;
#include "base/compiler_specific.h"

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:99:
std::vector<PendingRequest> pending_requests_;
#include <vector>

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:100: std::string
refresh_token_;
#include <string>

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/oa...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/oa...
chrome/browser/signin/oauth2_token_service.h:225: const ScopeSet& scopes);
nit: newline

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.h:95: virtual
net::URLRequestContextGetter* GetRequestContext() OVERRIDE;
If you change declaration order, you should probably also adjust definition
order in profile_oauth2_token_service.cc

https://codereview.chromium.org/23068005/diff/123001/components/browser_conte...
File
components/browser_context_keyed_service/browser_context_dependency_manager.h
(right):

https://codereview.chromium.org/23068005/diff/123001/components/browser_conte...
components/browser_context_keyed_service/browser_context_dependency_manager.h:75:

nit: remove extra blank line

https://codereview.chromium.org/23068005/diff/123001/components/browser_conte...
File
components/browser_context_keyed_service/browser_context_keyed_service_factory.cc
(right):

https://codereview.chromium.org/23068005/diff/123001/components/browser_conte...
components/browser_context_keyed_service/browser_context_keyed_service_factory.cc:16:
content::BrowserContext* context, FactoryFunction factory) {
drive-by nit: args on separate lines

https://codereview.chromium.org/23068005/diff/123001/components/browser_conte...
components/browser_context_keyed_service/browser_context_keyed_service_factory.cc:17:
// Should not set a testing factory if we already have a global factory.
why?

[Elliot Glaysher, 2013-08-19 18:04:29]

https://codereview.chromium.org/23068005/diff/98001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/test/base/testing_...
chrome/test/base/testing_profile.h:87:
BrowserContextKeyedServiceFactory::FactoryFunction callback);
On 2013/08/19 12:15:56, Andrew T Wilson wrote:
> On 2013/08/16 18:07:59, Elliot Glaysher wrote:
> > I'm fine with you adding this part of the change, where a set of factories
can
> > be injected before a TestingProfile is created. However, (and this may just
> may
> > be me getting confused in a giant patch), it seems like you're adding both
> > options discussed in the email: you're adding a way to inject testing
> factories
> > before profiles are created, and you're letting the user set a global
factory.
> > Is there a reason you can't handle the global cases with this?
> 
> So I don't actually like SetGlobalTestingFactory() since it has the potential
to
> leak to other tests as you pointed out, but since I'd already written it and
> it's potentially useful for BrowserTests (which don't use TestingProfile) I
left
> it in.
> 
> I'm happy to remove SetGlobalTestingFactory() if you like (let me know - I'm
not
> using it in this CL), but I'd like to keep
> TestingProfile::Builder::AddTestingFactory() since I think that's actually a
> much cleaner way to inject factories than either SetGlobalTestingFactory() or
> adding them after the profile is created.

Yeah, I'm fine with AddTestingFactory() as long as SetGlobalTestingFactory()
gets removed.

[awong, 2013-08-19 19:47:20]

lgtm for drive-by with 1 more nit.

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_prefs_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/98001/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_prefs_unittest.cc:160:
scoped_ptr<TestingProfile> login_profile_owner = builder.Build();
On 2013/08/19 12:15:56, Andrew T Wilson wrote:
> On 2013/08/16 17:53:16, awong wrote:
> > scoped_ptr<TestingProfile> login_profile_owner(builder.Build());
> 
> Done, but is there a reason why this is better? I personally prefer the a = b
> syntax for variable initialization, but I'm happy to be convinced that I'm
Doing
> It Wrong.

Guess it doesn't really matter too much. In an optimized build it's equiv. In a
debug build, because of the way move semantics are being emulated, I think this
will yield 2 extra type conversions. You're right though that it shouldn't
really matter.  Sorry for the noise.

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_session_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_session_unittest.cc:220:
scoped_ptr<TestingProfile> profile(builder.Build().Pass());
.Pass() on return values are always superfluous.

[Dan Beam, 2013-08-19 20:03:31]

c/b/ui/autofill lgtm

[Raghu Simha, 2013-08-20 00:16:28]

chrome/browser/sync lgtm.

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_password_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_password_unittest.cc:35: #include
"chrome/test/base/profile_mock.h"
This include seems unnecessary now.

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:41: #include
"chrome/test/base/profile_mock.h"
This include seems unnecessary now.

[Andrew T Wilson (Slow), 2013-08-20 09:28:34]

isherman: PTAL for owners rubberstamp on components/autofill

bauerb/mnissler: PTAL
erg: Removed Set/ResetGlobalTestingFactory() - PTAL

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc
(right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:29:
const char kOAuth2RefreshToken[] = "refreshtoken";
On 2013/08/19 14:00:06, Bernhard Bauer wrote:
> Could you move this after kManagedUserToken?

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:172:
ASSERT_GE(1u, requests.size());
On 2013/08/19 14:00:06, Bernhard Bauer wrote:
> This asserts that 1 >= requests.size(). I think you want the other way around?

OK, changed it to the stricter EQ instead.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher_unittest.cc:175:
EXPECT_EQ(1u, scopes.count(GaiaUrls::GetInstance()->oauth1_login_scope()));
On 2013/08/19 14:00:06, Bernhard Bauer wrote:
> Would it make sense to extract the code up until here (which is shared with
the
> method below) into a method that just verifies the state?

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc
(right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_client_registration_helper.cc:28: class
CloudPolicyClientRegistrationHelper::TokenHelper {
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> Empty base class? Why?

Because on Desktop we need either a TokenServiceHelper or a LoginTokenHelper,
neither of which are related, and only one of which needs to be stored at any
given time, and I want to keep a pointer to exactly one of them in my member
data.

Probably it's cleaner just to have two separate member variables. I'll do that.
Does Chrome have a general-purpose way to delete an arbitrary object (say, a
generic base class with a virtual destructor that I could use with a
scoped_ptr<>)?

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service.cc:39:
ProfileOAuth2TokenServiceFactory::GetForProfile(profile);
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> Can we inject this pointer from the factory via a ctor parameter? This helps
> prevent cycles in the dependency graph, because the PKSF can't grab pointers
for
> not-yet-created services.

Yep, we discussed this and I was planning to do that in a future CL, but I
suppose I can do it in this one.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service.cc:67:
ProfileOAuth2TokenServiceFactory::GetForProfile(profile());
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> pass pointer via ctor (also below)?

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service.cc:120:
SigninManagerFactory::GetForProfile(profile());
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> ditto, can we pass the pointer via ctor (also below)?

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:94: //
usernames.
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> I'd love to see this! Do we have a tracking bug that we can reference here?
Not AFAICT. It's still in the design phase/captured by the big project mirror
umbrella bug.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:95: class
FakeAndroidProfileOAuth2TokenService :
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> nit: colon goes on next line.

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/an...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/an...
chrome/browser/signin/android_profile_oauth2_token_service.cc:67: scopes);
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> nit: This doesn't seem to require any line breaks?

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/an...
chrome/browser/signin/android_profile_oauth2_token_service.cc:98:
GetAuthenticatedUsername();
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> Shouldn't this require listing SigninManager as a dependency in the
> ProfileOAuth2TokenServiceFactory then (I'm not seeing it declared there)?

Good point. Done (this isn't a new dependency, btw - it already existed but just
wasn't ever declared).

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
File chrome/browser/signin/fake_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:118:
pending_requests_.push_back(pending_request);
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> I don't see any place in the code where elements get removed from
> pending_requests_. Is that intentional? I guess CompleteRequests() should
remove
> them?

It's intentional - what we have is good enough for a mock class.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:3: // found in the
LICENSE file.
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> newline

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:39: std::string
client_id;
On 2013/08/19 14:00:06, Bernhard Bauer wrote:
> Make the member variables private?

Made this a struct instead.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:42:
base::WeakPtr<RequestImpl> request;
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> #include "base/memory/weak_ptr.h"

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:68: 
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> nit: if no newline on line #63, then none here either.

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:87: virtual
net::URLRequestContextGetter* GetRequestContext() OVERRIDE;
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> #include "base/compiler_specific.h"

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:99:
std::vector<PendingRequest> pending_requests_;
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> #include <vector>

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:100: std::string
refresh_token_;
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> #include <string>

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/oa...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/oa...
chrome/browser/signin/oauth2_token_service.h:225: const ScopeSet& scopes);
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> nit: newline

Done.

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.h:95: virtual
net::URLRequestContextGetter* GetRequestContext() OVERRIDE;
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> If you change declaration order, you should probably also adjust definition
> order in profile_oauth2_token_service.cc

Oh, god, I hadn't realized that someone was actually keeping the .h and .cc
orderings in sync. People do the silliest things.

https://codereview.chromium.org/23068005/diff/123001/components/browser_conte...
File
components/browser_context_keyed_service/browser_context_dependency_manager.h
(right):

https://codereview.chromium.org/23068005/diff/123001/components/browser_conte...
components/browser_context_keyed_service/browser_context_dependency_manager.h:75:

On 2013/08/19 14:03:22, Mattias Nissler wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/23068005/diff/123001/components/browser_conte...
File
components/browser_context_keyed_service/browser_context_keyed_service_factory.cc
(right):

https://codereview.chromium.org/23068005/diff/123001/components/browser_conte...
components/browser_context_keyed_service/browser_context_keyed_service_factory.cc:16:
content::BrowserContext* context, FactoryFunction factory) {
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> drive-by nit: args on separate lines

As a rule I don't fix style violations on already-existing lines of code I'm not
changing. But I'll bend my rule for you :-P

https://codereview.chromium.org/23068005/diff/123001/components/browser_conte...
components/browser_context_keyed_service/browser_context_keyed_service_factory.cc:17:
// Should not set a testing factory if we already have a global factory.
On 2013/08/19 14:03:22, Mattias Nissler wrote:
> why?

Because one clobbers the other - having a test set one and then overwrite it
with another seems like an error (conflicting test library routines, for
example).

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_password_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_password_unittest.cc:35: #include
"chrome/test/base/profile_mock.h"
On 2013/08/20 00:16:29, rsimha wrote:
> This include seems unnecessary now.

Done.

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_session_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_session_unittest.cc:220:
scoped_ptr<TestingProfile> profile(builder.Build().Pass());
On 2013/08/19 19:47:21, awong wrote:
> .Pass() on return values are always superfluous.

Thanks! I appreciate the drive-by - I learned things! Like not to sprinkle
Pass() around my code as if it were salt.

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/36001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:41: #include
"chrome/test/base/profile_mock.h"
On 2013/08/20 00:16:29, rsimha wrote:
> This include seems unnecessary now.

Done.

[Bernhard Bauer, 2013-08-20 09:39:02]

c/b/managed_mode LGTM

https://codereview.chromium.org/23068005/diff/183001/chrome/browser/signin/fa...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/183001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:44: ~PendingRequest();
Nit: Add a newline?

[Andrew T Wilson (Slow), 2013-08-20 12:24:20]

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/23068005/diff/123001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service.cc:120:
SigninManagerFactory::GetForProfile(profile());
On 2013/08/20 09:28:35, Andrew T Wilson wrote:
> On 2013/08/19 14:03:22, Mattias Nissler wrote:
> > ditto, can we pass the pointer via ctor (also below)?
> 
> Done.

Actually, I take it back - we can't inject this at construction time because
this pulls in a bunch of other dependencies (like TokenService, which relies on
the existence of a UI thread) and this breaks tests that just want to create a
TestingProfile without creating threads.

I've logged crbug.com/276270 to track this issue - we're getting rid of
TokenService and once that's complete we can inject SigninManager also.

https://codereview.chromium.org/23068005/diff/183001/chrome/browser/signin/fa...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/183001/chrome/browser/signin/fa...
chrome/browser/signin/fake_profile_oauth2_token_service.h:44: ~PendingRequest();
On 2013/08/20 09:39:03, Bernhard Bauer wrote:
> Nit: Add a newline?

Done.

[Mattias Nissler (ping if slow), 2013-08-20 16:00:27]

Sorry for the delay... but dude, this review is still too damn big and I find
myself having to switch mental context to one of the several logical changes for
every new file I look at.

Anyhow, LGTM.

https://codereview.chromium.org/23068005/diff/189001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/23068005/diff/189001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:58: virtual
net::URLRequestContextGetter* GetRequestContext() OVERRIDE;
This probably also requires definition order adjustments in the .cc file (I
thought we had consistent ordering as a general rule?)

[Andrew T Wilson (Slow), 2013-08-20 16:18:54]

On 2013/08/20 16:00:27, Mattias Nissler wrote:
> Sorry for the delay... but dude, this review is still too damn big and I find
> myself having to switch mental context to one of the several logical changes
for
> every new file I look at.

Agreed, I should have broken this into 3 CLs. I had hoped that since the changes
were mostly orthogonal to one another that I could have reviewers review
directories individually and not have it be too burdensome, but in retrospect I
shouldn't have been lazy and just did the right thing. Thanks for the careful
review of the whole thing.

[Elliot Glaysher, 2013-08-20 19:20:41]

profiles lgtm

[Andrew T Wilson (Slow), 2013-08-21 07:07:36]

sky: PING for owners stamp on ui/* changes and changes under chrome/test/base
toyoshim: PTAL at changes under chrome/browser/translate
derat: PTAL at chrome/browser/chromeos/power/*
avi: PTAL at changes under browser/tab_contents

[Andrew T Wilson (Slow), 2013-08-21 07:10:06]

sky: PING for owners stamp on ui/* changes and changes under chrome/test/base
toyoshim: owners stamp for changes under chrome/browser/translate
nkostylev: owners stamp for chrome/browser/chromeos/power/*
avi: owners stamp for changes under browser/tab_contents

[Takashi Toyoshima, 2013-08-21 07:41:38]

translate/ lgtm

[sky, 2013-08-21 13:48:30]

https://codereview.chromium.org/23068005/diff/203001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/203001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:379: return
builder.Build().release();
Who ends up owning this?

https://codereview.chromium.org/23068005/diff/203001/chrome/test/base/testing...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/23068005/diff/203001/chrome/test/base/testing...
chrome/test/base/testing_profile.h:208: // DEPRECATED.
What is wrong with this approach for setting incognito?

[Avi (use Gerrit), 2013-08-21 14:44:47]

browser/tab_contents LGTM

[Andrew T Wilson (Slow), 2013-08-21 15:25:49]

sky: PTAL. I've updated the documentation for ForceIncognito() to explain
exactly why it's deprecated.

https://codereview.chromium.org/23068005/diff/203001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/23068005/diff/203001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:379: return
builder.Build().release();
On 2013/08/21 13:48:32, sky wrote:
> Who ends up owning this?

RenderViewHostTestHarness does, according to the method documentation (I didn't
duplicate the method documentation in this class, figuring that indicating I was
overriding a RenderViewHostTestHarness method was enough documentation):

  // Derived classes should override this method to use a custom BrowserContext.
  // It is invoked by SetUp after threads were started.
  // RenderViewHostTestHarness will take ownership of the returned
  // BrowserContext.
  virtual BrowserContext* CreateBrowserContext();

https://codereview.chromium.org/23068005/diff/203001/chrome/test/base/testing...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/23068005/diff/203001/chrome/test/base/testing...
chrome/test/base/testing_profile.h:208: // DEPRECATED.
On 2013/08/21 13:48:32, sky wrote:
> What is wrong with this approach for setting incognito?

It's fundamentally broken because some PKSs should never be created for
incognito profiles, but if you set the profile incognito *after* the profile is
already created, then the PKS is often already created.

Here's a case that I ran into several times:

1) PKS A depends on PKS B, so it should not be possible for PKS A to be created
if PKS B does not exist (so, they are both NULL in incognito mode).

2) Test creates a profile. PKS A is created and registers for some
notifications.

3) Test sets the profile as incognito.

4) PKS A gets a notification, and tries to look up PKS B from the profile, and
gets a NULL pointer and crashes.

Basically, it's a test anti-pattern. If a test needs an incognito profile, then
it should create an incognito profile rather than taking a non-incognito profile
and dynamically changing the return value of HasOffTheRecordProfile().

[Andrew T Wilson (Slow), 2013-08-21 15:30:36]

estade: OWNERS stamp for components/autofill
tim: OWNERS stamp for chrome/browser/password_manager

Thanks!

[Nikita (slow), 2013-08-21 15:56:30]

On 2013/08/21 07:10:06, Andrew T Wilson wrote:
> nkostylev: owners stamp for chrome/browser/chromeos/power/*

lgtm

[sky, 2013-08-21 15:59:17]

Fair enough, LGTM - but add a TODO and file a bug to nuke the deprecated
function (maybe cc tfarina on it as he may want to fix it).

[commit-bot: I haz the power, 2013-08-22 08:31:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/23068005/203002

[commit-bot: I haz the power, 2013-08-22 10:49:44]

Change committed as 218990