Replace direct access to kGoogleServicesUsername with calls to SigninManager.

Replace direct access to kGoogleServicesUsername with calls to SigninManager.  There is no functional change in this CL.

Some places outside SigninManager still use the pref, like one_click_signinin and ProfileIOData, but that is deprecated code that will go away.  I will remove that code in separate CLs.

BUG=107160
Committed: https://crrev.com/614597db84107c4e822210ad2d222fcaa3cd9c5b
Cr-Commit-Position: refs/heads/master@{#307042}

[Roger Tawa OOO till Jul 10th, 2014-12-02 15:25:24]

Patchset #2 (id:20001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-02 15:25:35]

Patchset #2 (id:40001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-02 15:25:42]

Patchset #2 (id:60001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-02 15:25:50]

Patchset #2 (id:80001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-02 15:25:57]

Patchset #2 (id:100001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-02 15:26:05]

Patchset #2 (id:110019) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-02 15:26:12]

Patchset #2 (id:130001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-02 15:26:21]

Patchset #2 (id:150001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-04 02:34:32]

Patchset #2 (id:170001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-04 02:34:44]

Patchset #2 (id:190001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-04 02:34:55]

Patchset #2 (id:210001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-04 02:35:09]

Patchset #2 (id:230001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-04 15:51:21]

Patchset #3 (id:260001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-04 15:51:28]

Patchset #2 (id:240032) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-04 15:51:36]

Patchset #1 (id:1) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-04 16:27:56]

rogerta@chromium.org changed reviewers:
+ kalman@chromium.org, noms@chromium.org, treib@chromium.org,
xiyuan@chromium.org, zea@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-12-04 16:27:57]

Hi all,

This is a cleanup CL, and does not change behaviour of chrome.

Nicolas, please review:
chrome/browser/sync/*
chrome/browser/ui/webui/sync_internals_browsertest.js

Monica, please review:
chrome/browser/profiles/*
chrome/browser/signin/*

Marc, please review:
chrome/browser/supervised_user/*

Ben, please review:
chrome/browser/extensions/api/*

Xiyuan, please review:
chrome/browser/chromeos/*

Thanks.

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:261:
SetAuthenticatedUsername("test@chromium.org");
On cros, you can only call SetAuthenticatedUsername() once.  And once called,
you can't logout and then call it again with a different account.  So I can'y
just put a call in the SetUp() function at line 190 above, and then override
only in the tests that want to use a different email.  Each must call
SetAuthenticatedUsername() with the account it wants to use.

[not at google - send to devlin, 2014-12-04 16:32:31]

I'll take your word for it. lgtm for extensions modulo that 1 comment.

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:233:
SigninManagerFactory::GetForProfile(browser()->profile())->
Is there any harm in doing this in a SetUp for every
EPKPChallengeMachineKeyTest?

[noms (inactive), 2014-12-04 16:35:39]

profiles + signin lgtm

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:838: std::string
ProfileImpl::GetProfileName() {
Uhoh, yak attack: if this literally returns an email or an empty string, I don't
think it should be called GetProfileName anymore. Maybe GetProfileUsername()? I
can do this in a different CL.

Basically, I am talking out loud.

[Roger Tawa OOO till Jul 10th, 2014-12-04 16:35:58]

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:233:
SigninManagerFactory::GetForProfile(browser()->profile())->
On 2014/12/04 16:32:31, kalman wrote:
> Is there any harm in doing this in a SetUp for every
> EPKPChallengeMachineKeyTest?

See my comment at line 261 below :-)

[not at google - send to devlin, 2014-12-04 16:37:25]

kalman@chromium.org changed reviewers:
+ pneubeck@chromium.org

[not at google - send to devlin, 2014-12-04 16:37:26]

+pneubeck on that test. Maybe he will have a good idea.

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:261:
SetAuthenticatedUsername("test@chromium.org");
On 2014/12/04 16:27:57, Roger Tawa wrote:
> On cros, you can only call SetAuthenticatedUsername() once.  And once called,
> you can't logout and then call it again with a different account.  So I can'y
> just put a call in the SetUp() function at line 190 above, and then override
> only in the tests that want to use a different email.  Each must call
> SetAuthenticatedUsername() with the account it wants to use.

Well... bummer.

[pneubeck (no reviews), 2014-12-04 17:06:48]

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:261:
SetAuthenticatedUsername("test@chromium.org");
On 2014/12/04 16:37:26, kalman wrote:
> On 2014/12/04 16:27:57, Roger Tawa wrote:
> > On cros, you can only call SetAuthenticatedUsername() once.  And once
called,
> > you can't logout and then call it again with a different account.  So I
can'y
> > just put a call in the SetUp() function at line 190 above, and then override
> > only in the tests that want to use a different email.  Each must call
> > SetAuthenticatedUsername() with the account it wants to use.
> 
> Well... bummer.

How about a separate test-fixture that again overrides some function to setup
the non-default username?
That test-fixture should probably be called ...UnmanagedUser...

btw. this test was, I think, originally written by dkrahn@

[Marc Treib, 2014-12-04 17:20:36]

supervised_user lgtm

[Nicolas Zea, 2014-12-04 17:57:40]

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (left):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:2506:
profile_->GetPrefs()->GetString(prefs::kGoogleServicesUsername));
Is this no longer necessary? Was the signin manager previously not always fully
initialized, but it is now?

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_browsertest.js (right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_browsertest.js:234: // TODO(rogerta):
re-enable this test on non-cros platforms.  On cros, the
Were you planning to conditionally disable this as part of this patch? I think
you can do it using GEN calls (see for example browser_options_browsertest.js

[xiyuan, 2014-12-04 19:02:30]

lgtm with nits

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_manager_browsertest.cc (right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:1210:
chromeos::ProfileHelper::GetProfileByUserIdHash(info.hash))->
nit: fix indent

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc
(right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc:69:
SetAuthenticatedUsername(user->email());
I am not sure why we need to do this explicitly.
LoginManagerTest::AddUserToSession should have taken care of this. Maybe we
should just delete this line. Can you give that a try?

[Roger Tawa OOO till Jul 10th, 2014-12-04 21:31:36]

Patchset #2 (id:300001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-04 21:32:03]

Patchset #2 (id:300001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-04 22:26:47]

Thanks everyone.  Comments addressed, questions answered.  Please take another
look.

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_manager_browsertest.cc (right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:1210:
chromeos::ProfileHelper::GetProfileByUserIdHash(info.hash))->
On 2014/12/04 19:02:29, xiyuan wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:261:
SetAuthenticatedUsername("test@chromium.org");
On 2014/12/04 17:06:47, pneubeck wrote:
> On 2014/12/04 16:37:26, kalman wrote:
> > On 2014/12/04 16:27:57, Roger Tawa wrote:
> > > On cros, you can only call SetAuthenticatedUsername() once.  And once
> called,
> > > you can't logout and then call it again with a different account.  So I
> can'y
> > > just put a call in the SetUp() function at line 190 above, and then
override
> > > only in the tests that want to use a different email.  Each must call
> > > SetAuthenticatedUsername() with the account it wants to use.
> > 
> > Well... bummer.
> 
> How about a separate test-fixture that again overrides some function to setup
> the non-default username?
> That test-fixture should probably be called ...UnmanagedUser...
> 
> btw. this test was, I think, originally written by dkrahn@

Yeah, actually that is a lot better.  Done.

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:838: std::string
ProfileImpl::GetProfileName() {
On 2014/12/04 16:35:39, Monica Dinculescu wrote:
> Uhoh, yak attack: if this literally returns an email or an empty string, I
don't
> think it should be called GetProfileName anymore. Maybe GetProfileUsername()?
I
> can do this in a different CL.
> 
> Basically, I am talking out loud.

Agreed, function should be renamed, but in a separate CL.

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (left):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:2506:
profile_->GetPrefs()->GetString(prefs::kGoogleServicesUsername));
On 2014/12/04 17:57:39, Nicolas Zea wrote:
> Is this no longer necessary? Was the signin manager previously not always
fully
> initialized, but it is now?

Well, that's a question for you :-)  Pavel tracked down the introduction of this
to an old CL:

https://codereview.chromium.org/8332023/diff/8002/chrome/browser/sync/profile...

but the comment is no longer applicable.  Today, if you look at the
implementation of signin_->GetOriginal()->IsAuthenticated(), that is true iff
!authenticated_account_id_.empty().  That member is initialized either from:

1/ prefs::kGoogleServicesUsername, OR
2/ by a call to SetAuthenticatedUsername(), which sets
prefs::kGoogleServicesUsername

So if IsAuthenticated() is false, then prefs::kGoogleServicesUsername is empty,
and SetAuthenticatedUsername() would DCHECK.

I left a DCHECK to see if my assumption is wrong, but it was not hit in any of
the unit tests or browser tests, or in any of the manual testing I did.

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc
(right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc:69:
SetAuthenticatedUsername(user->email());
On 2014/12/04 19:02:29, xiyuan wrote:
> I am not sure why we need to do this explicitly.
> LoginManagerTest::AddUserToSession should have taken care of this. Maybe we
> should just delete this line. Can you give that a try?

Yup, you are correct. No need to set authenticated user at all.  Removed.

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_browsertest.js (right):

https://codereview.chromium.org/763673002/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_browsertest.js:234: // TODO(rogerta):
re-enable this test on non-cros platforms.  On cros, the
On 2014/12/04 17:57:39, Nicolas Zea wrote:
> Were you planning to conditionally disable this as part of this patch? I think
> you can do it using GEN calls (see for example browser_options_browsertest.js

Right, I want fix this TODO before commit.  Here is a fix based on our email
thread.

[Nicolas Zea, 2014-12-04 23:23:43]

sync LGTM

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

rogerta@chromium.org changed reviewers:
+ sky@chromium.org

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

Hi Scott,

Can you do an owner review for chrome/browser/ui/* ?  Thanks.

[pneubeck (no reviews), 2014-12-05 09:50:41]

lgtm for
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc

[sky, 2014-12-05 16:45:03]

LGTM

https://codereview.chromium.org/763673002/diff/400001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/763673002/diff/400001/chrome/browser/ui/webui...
chrome/browser/ui/webui/history_ui.cc:123: bool is_authenticated =
signin_manager != NULL &&
nullptr

[Roger Tawa OOO till Jul 10th, 2014-12-05 17:02:16]

Thanks Scott.

https://codereview.chromium.org/763673002/diff/400001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/763673002/diff/400001/chrome/browser/ui/webui...
chrome/browser/ui/webui/history_ui.cc:123: bool is_authenticated =
signin_manager != NULL &&
On 2014/12/05 16:45:03, sky wrote:
> nullptr

Done.

[Roger Tawa OOO till Jul 10th, 2014-12-05 17:02:31]

The CQ bit was checked by rogerta@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/763673002/420001

[commit-bot: I haz the power, 2014-12-05 17:57:42]

Committed patchset #7 (id:420001)

[commit-bot: I haz the power, 2014-12-05 17:58:30]

Patchset 7 (id:??) landed as
https://crrev.com/614597db84107c4e822210ad2d222fcaa3cd9c5b
Cr-Commit-Position: refs/heads/master@{#307042}