User cloud policy token forwarder: Replace PO2TS::GetPrimaryAccountId() with SMB::GetAuthenticatedAc

User cloud policy token forwarder: Replace PO2TS::GetPrimaryAccountId() with SMB::GetAuthenticatedAccountId.

The functionality is the same, just moving the code to a new place.

BUG=333995, 107160
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255298

[Roger Tawa OOO till Jul 10th, 2014-03-01 15:50:06]

Hi Joao, Xiyuan,

Please do overall review.

Xiyuan: please review changes in login_utils.cc

Thanks.

[xiyuan, 2014-03-01 16:45:28]

+nkostylev to double check login_utils.cc

https://codereview.chromium.org/137553007/diff/1670001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/137553007/diff/1670001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_utils.cc:465:
signin_manager->SetAuthenticatedUsername(user_id);
This makes SigninManagerBase be created earlier because InitProfilePreferences
is called in CREATE_STATUS_CREATED notification. Part of the profile is not
initialized (such as extension system). It appears to be working but want to see
if it rings bells for anyone.

Second, SigninManagerBase does not update kGoogleServicesUsername. It only
updates kGoogleServicesLastUsername. Would this be okay?

[Roger Tawa OOO till Jul 10th, 2014-03-01 18:30:04]

https://codereview.chromium.org/137553007/diff/1670001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/137553007/diff/1670001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_utils.cc:465:
signin_manager->SetAuthenticatedUsername(user_id);
On 2014/03/01 16:45:28, xiyuan wrote:
> This makes SigninManagerBase be created earlier because InitProfilePreferences
> is called in CREATE_STATUS_CREATED notification. Part of the profile is not
> initialized (such as extension system). It appears to be working but want to
see
> if it rings bells for anyone.
> 
> Second, SigninManagerBase does not update kGoogleServicesUsername. It only
> updates kGoogleServicesLastUsername. Would this be okay?

This line is not causing SM to be created earlier.  The change in the
UserCloudPolicyTokenForwarderFactory is doing that.  By the time execution gets
here, SM is already created and does not know the user_id.

That's a good catch for the second point.  Several options:

1/ set the pref and call SetAuthenticatedUsername() too.  I just quickly tested
that and it works for the test.
2/ Change SetAuthenticatedUsername() to set the pref iff its different.  I took
a look at all other places that call SetAuthenticatedUsername() and they are
doing so after noticing that the pref is set but SM is not.
3/ Move the setting of the pref from here to some place even earlier, before
UserCloudPolicyTokenForwarder or SM is created.

Option 1 makes the code here more consistent with the other code that calls
SetAuthenticatedUsername().  Option 2 might be safer though to prevent this from
happening in new code.  I don't think option 3 is great, just listed it for
completeness.

[xiyuan, 2014-03-01 18:40:35]

https://codereview.chromium.org/137553007/diff/1670001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/137553007/diff/1670001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_utils.cc:465:
signin_manager->SetAuthenticatedUsername(user_id);
I like option 2 better because ideally we should let SM handle the pref and all
other places should not worry about that.

[Joao da Silva, 2014-03-03 19:53:30]

policy lgtm

[Roger Tawa OOO till Jul 10th, 2014-03-04 16:14:24]

Done as Xiyuan suggested, and fixing broken tests not too bad at all :-)

Nicolas: please review the GCM and sync harness changes.  Thanks.

[xiyuan, 2014-03-04 17:20:06]

Cool. LGTM

[Nikita (slow), 2014-03-05 16:38:49]

lgtm

[rlarocque, 2014-03-05 18:54:45]

lgtm

[fgorski, 2014-03-05 19:00:28]

services/gcm lgtm mod comments.

Please update the code as it is incorrect before your change

https://codereview.chromium.org/137553007/diff/1770001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/137553007/diff/1770001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:142:
profile()->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);
Could you change the code to this:
  const std::string& username = GetAuthenticatedUsername();
  clear_authenticated_username();
  profile_->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);

https://codereview.chromium.org/137553007/diff/1770001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:144:
GoogleServiceSignoutDetails details(GetAuthenticatedUsername());
And use username here.

[Roger Tawa OOO till Jul 10th, 2014-03-05 19:12:46]

Thanks Filip, Richard.  Command addressed, changes uploaded.

https://codereview.chromium.org/137553007/diff/1770001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/137553007/diff/1770001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:142:
profile()->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);
On 2014/03/05 19:00:28, Filip Gorski wrote:
> Could you change the code to this:
>   const std::string& username = GetAuthenticatedUsername();
>   clear_authenticated_username();
>   profile_->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);	     
> 

Good catch!  Done.

https://codereview.chromium.org/137553007/diff/1770001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:144:
GoogleServiceSignoutDetails details(GetAuthenticatedUsername());
On 2014/03/05 19:00:28, Filip Gorski wrote:
> And use username here.

Done.

[Roger Tawa OOO till Jul 10th, 2014-03-05 19:13:12]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-03-05 19:13:21]

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

[commit-bot: I haz the power, 2014-03-05 20:39:40]

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

[commit-bot: I haz the power, 2014-03-05 20:39:41]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Roger Tawa OOO till Jul 10th, 2014-03-05 21:29:22]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-03-05 21:30:03]

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

[commit-bot: I haz the power, 2014-03-06 00:02:15]

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

[commit-bot: I haz the power, 2014-03-06 02:43:30]

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

[commit-bot: I haz the power, 2014-03-06 03:12:17]

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

[commit-bot: I haz the power, 2014-03-06 10:39:03]

Change committed as 255298