This is the second CL of several that will eventually replace TokenService with

This is the second CL of several that will eventually replace TokenService with
ProfileOAuth2TokenService. In this CL, the dependencies on TS are removed
from SigninManager and UbertokenFetcher.

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

[Roger Tawa OOO till Jul 10th, 2013-11-13 19:16:29]

Hi Filip, Drew,

Please take a look.  Thanks.

[Roger Tawa OOO till Jul 10th, 2013-11-13 20:16:03]

Hi Hui,

Please take a look.  Thanks.

[fgorski, 2013-11-13 20:36:48]

Roger, overall it looks ok. There are a few things I think you should look at.

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.cc (left):

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.cc:561:
DCHECK(!GetAuthenticatedUsername().empty());
Do we no longer care about this DCHECK, or was it simply not necessary in the
first place?

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager_base.cc (left):

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager_base.cc:68: // OAuthLoginManager once the
rest of the Profile is fully initialized.
Does Chrome OS depend on the code above in any way?

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager_unittest.cc (right):

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager_unittest.cc:27: #include
"content/public/browser/notification_source.h"
did you use those two includes anywhere?

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

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/tok...
chrome/browser/signin/token_service_unittest.cc:26:
TokenServiceTestHarness::TokenServiceTestHarness() {}
: signin_manager_(NULL), service_(NULL) {

Unless you missed that on purpose.

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/tok...
chrome/browser/signin/token_service_unittest.cc:79: 
nit: delete the new line.

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/ube...
File chrome/browser/signin/ubertoken_fetcher_unittest.cc (right):

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/ube...
chrome/browser/signin/ubertoken_fetcher_unittest.cc:13: #include
"chrome/browser/signin/token_service_unittest.h"
Is this include still needed?

[Roger Tawa OOO till Jul 10th, 2013-11-13 20:56:12]

Thanks Filip.  Comments addressed, changes uploaded.

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.cc (left):

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.cc:561:
DCHECK(!GetAuthenticatedUsername().empty());
On 2013/11/13 20:36:48, Filip Gorski wrote:
> Do we no longer care about this DCHECK, or was it simply not necessary in the
> first place?

No, good to have the check.  Put it back in.

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager_base.cc (left):

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager_base.cc:68: // OAuthLoginManager once the
rest of the Profile is fully initialized.
On 2013/11/13 20:36:48, Filip Gorski wrote:
> Does Chrome OS depend on the code above in any way?

This code only used to initialize the old token service.  PO2TS initializes
itself, so need to do anything for chromeos.

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager_unittest.cc (right):

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager_unittest.cc:27: #include
"content/public/browser/notification_source.h"
On 2013/11/13 20:36:48, Filip Gorski wrote:
> did you use those two includes anywhere?

Yes.  For example line 81 for notification_source, line 240 for
notification_tracker.

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

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/tok...
chrome/browser/signin/token_service_unittest.cc:26:
TokenServiceTestHarness::TokenServiceTestHarness() {}
On 2013/11/13 20:36:48, Filip Gorski wrote:
> : signin_manager_(NULL), service_(NULL) {
> 
> Unless you missed that on purpose.

Good catch.  Done.

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/tok...
chrome/browser/signin/token_service_unittest.cc:79: 
On 2013/11/13 20:36:48, Filip Gorski wrote:
> nit: delete the new line.

Done.

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/ube...
File chrome/browser/signin/ubertoken_fetcher_unittest.cc (right):

https://codereview.chromium.org/71723002/diff/80001/chrome/browser/signin/ube...
chrome/browser/signin/ubertoken_fetcher_unittest.cc:13: #include
"chrome/browser/signin/token_service_unittest.h"
On 2013/11/13 20:36:48, Filip Gorski wrote:
> Is this include still needed?

Done.

[Andrew T Wilson (Slow), 2013-11-14 10:34:57]

lgtm, with one question around whether we really want SM to initiate token
loading for PO2TS.

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.cc:126: token_service->LoadCredentials();
I thought the plan was to have PO2TS call LoadCredentials() on itself from
Initialize() - did we change this for some reason? It's just weird to have SM be
responsible for calling it since in general we still want to load tokens even if
SM is not signed in (think of the ManagedUser case).

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.cc:339:
token_service->RevokeAllCredentials();
I have no problem keeping this tight dependency between PO2TS and SM. That said,
one alternative would be for PO2TS to listen for SIGNED_OUT notifications and
then you don't have this explicit dependency.

One nice side effect of doing it the way you have in this CL is you're
guaranteed that everyone gets the SIGNED_OUT notification before any credentials
are revoked, which might be useful (at the very least it's more deterministic).

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager_base.cc (left):

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_base.cc:67: // Note: ChromeOS will kick off
TokenService::LoadTokensFromDB from
Should we get rid of InitTokenService() in SigninManagerBase() and instead just
define it as a private method on SigninManager() and call it from
SigninManager::Initialize()? Since there's no base-class functionality and no
need for polymorphism we shouldn't keep it as a virtual method.

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager_unittest.cc (right):

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_unittest.cc:95: profile_.reset();
nit: probably don't need these two reset() calls any more.

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_unittest.cc:239: scoped_ptr<TestingProfile>
profile_;
BTW, you *could* just make this:

TestingProfile profile_;

[Roger Tawa OOO till Jul 10th, 2013-11-14 17:04:07]

Thanks Drew.  Comments addressed, changes uploaded.

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.cc:126: token_service->LoadCredentials();
On 2013/11/14 10:34:57, Andrew T Wilson wrote:
> I thought the plan was to have PO2TS call LoadCredentials() on itself from
> Initialize() - did we change this for some reason? It's just weird to have SM
be
> responsible for calling it since in general we still want to load tokens even
if
> SM is not signed in (think of the ManagedUser case).

In the first CL of this series, you pointed out that loading credentials at
initialization time would likely cause startup perf issues.  That's why we do
that today with old TokenService.  So that CL did not change the load timing.  I
opened crbug.com/219361 to track the work of moving LoadCredentials() into PO2TS
itself.

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.cc:339:
token_service->RevokeAllCredentials();
On 2013/11/14 10:34:57, Andrew T Wilson wrote:
> I have no problem keeping this tight dependency between PO2TS and SM. That
said,
> one alternative would be for PO2TS to listen for SIGNED_OUT notifications and
> then you don't have this explicit dependency.
> 
> One nice side effect of doing it the way you have in this CL is you're
> guaranteed that everyone gets the SIGNED_OUT notification before any
credentials
> are revoked, which might be useful (at the very least it's more
deterministic).

Agreed that your suggestion also would work.  At sign in time, the order between
the token being available and the notification is important.  I don't know if
that applies here as well; I think I'll keep it like this though since I don't
think there should be dependencies from PO2TS to SM.

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager_base.cc (left):

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_base.cc:67: // Note: ChromeOS will kick off
TokenService::LoadTokensFromDB from
On 2013/11/14 10:34:57, Andrew T Wilson wrote:
> Should we get rid of InitTokenService() in SigninManagerBase() and instead
just
> define it as a private method on SigninManager() and call it from
> SigninManager::Initialize()? Since there's no base-class functionality and no
> need for polymorphism we shouldn't keep it as a virtual method.

Yes good point, done.

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager_unittest.cc (right):

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_unittest.cc:95: profile_.reset();
On 2013/11/14 10:34:57, Andrew T Wilson wrote:
> nit: probably don't need these two reset() calls any more.

Actually these are sort of needed.  See comment below for more details.

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_unittest.cc:239: scoped_ptr<TestingProfile>
profile_;
On 2013/11/14 10:34:57, Andrew T Wilson wrote:
> BTW, you *could* just make this:
> 
> TestingProfile profile_;

Could do the same for TestingPrefServiceSimple below.  However, there is a order
dependency of destruction: TestingProfile needs to be destroyed before
TestBrowserThreadBundle.

While the C++ standard does specify destruction order of data member, it seems
to me depending on this is fragile.  Using scoped_ptr allow the test to control
order of destruction in TearDown().

[Andrew T Wilson (Slow), 2013-11-14 17:11:53]

still lgtm

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager_unittest.cc (right):

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_unittest.cc:95: profile_.reset();
On 2013/11/14 17:04:08, Roger Tawa wrote:
> On 2013/11/14 10:34:57, Andrew T Wilson wrote:
> > nit: probably don't need these two reset() calls any more.
> 
> Actually these are sort of needed.  See comment below for more details.

Should probably document why we're doing it then.

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_unittest.cc:239: scoped_ptr<TestingProfile>
profile_;
On 2013/11/14 17:04:08, Roger Tawa wrote:
> On 2013/11/14 10:34:57, Andrew T Wilson wrote:
> > BTW, you *could* just make this:
> > 
> > TestingProfile profile_;
> 
> Could do the same for TestingPrefServiceSimple below.  However, there is a
order
> dependency of destruction: TestingProfile needs to be destroyed before
> TestBrowserThreadBundle.
> 
> While the C++ standard does specify destruction order of data member, it seems
> to me depending on this is fragile.  Using scoped_ptr allow the test to
control
> order of destruction in TearDown().

That's fine with me. BTW, there is definitely other code in the system that
relies on the construction/destruction order of data members, so it wouldn't be
too unusual to rely on it here. But it's probably cleaner to explicitly shutdown
objects if you depend on the destruction ordering.

[Roger Tawa OOO till Jul 10th, 2013-11-14 18:09:32]

Thanks Drew.

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager_unittest.cc (right):

https://codereview.chromium.org/71723002/diff/180001/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_unittest.cc:95: profile_.reset();
On 2013/11/14 17:11:54, Andrew T Wilson wrote:
> On 2013/11/14 17:04:08, Roger Tawa wrote:
> > On 2013/11/14 10:34:57, Andrew T Wilson wrote:
> > > nit: probably don't need these two reset() calls any more.
> > 
> > Actually these are sort of needed.  See comment below for more details.
> 
> Should probably document why we're doing it then.

Done.

[guohui, 2013-11-14 19:08:27]

lgtm

[fgorski, 2013-11-14 19:38:49]

lgtm

[commit-bot: I haz the power, 2013-11-15 17:55:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/71723002/760001

[commit-bot: I haz the power, 2013-11-15 18:36:36]

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

[Roger Tawa OOO till Jul 10th, 2013-11-15 18:40:53]

Hi Scott,

Can you do an onwer review for chrome_notification_types.h?  Just a comment
change.  Thanks.

[sky, 2013-11-15 20:31:55]

LGTM

[commit-bot: I haz the power, 2013-11-15 22:24:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/71723002/760001

[commit-bot: I haz the power, 2013-11-16 01:04:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/71723002/760001

[commit-bot: I haz the power, 2013-11-16 03:23:15]

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

[commit-bot: I haz the power, 2013-11-23 03:38:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/71723002/1540001

[commit-bot: I haz the power, 2013-11-23 03:38:18]

Failed to apply patch for
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
  Hunk #1 FAILED at 519.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc.rej

Patch:      
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
Index:
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
diff --git
a/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
b/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
index
bf5b7448910bd6321baf419e98e2540393658470..fe984c53b7d4b232ac3d80b63a38e5c29e664858
100644
---
a/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
+++
b/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
@@ -519,6 +519,7 @@ TEST_F(UserCloudPolicyManagerChromeOSTest,
NonBlockingFirstFetch) {
   // Set a fake refresh token at the TokenService.
   TokenService* token_service = TokenServiceFactory::GetForProfile(profile_);
   ASSERT_TRUE(token_service);
+  token_service->Initialize("test_source", profile_);
   GaiaAuthConsumer::ClientOAuthResult tokens("refresh", "access", 3600);
   EXPECT_FALSE(token_service->HasOAuthLoginToken());
   token_service->UpdateCredentialsWithOAuth2(tokens);

[commit-bot: I haz the power, 2013-11-23 15:33:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/71723002/1660001

[commit-bot: I haz the power, 2013-11-23 19:05:03]

Change committed as 236955