sync: remove GetAuthenticatedUsername from ProfileSyncService/SyncBackendHost.

sync: remove GetAuthenticatedUsername from ProfileSyncService/SyncBackendHost.

Removing as it's not necessary and simplifies code to make it easier to ultimately rid ProfileSyncService of cros_user,
and ultimately convert it to a ProfileKeyedService.

Before this change the two values only differ 1) on chrome os, where kGoogleServicesUsername doesn't seem to be set today (and in some cases we use cros_user as a crutch), and 2) when kGoogleServicesUsername is cleared, but that only happens on DisableForUser.  In the future I'd imagine this value to be allowed to outlive that call as well, which would be OK.

BUG=88109, 93922
TEST=bits of UI that show usernames continue to show usernames


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112643

[tim (not reviewing), 2011-11-23 20:13:41]

I'm in the process of trying this out on chromeos, but what with turkey day upon
us, thought I'd get this out for a peek if you have questions!

http://codereview.chromium.org/8665011/diff/2001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util.cc (left):

http://codereview.chromium.org/8665011/diff/2001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:81:
DCHECK(service.GetAuthenticatedUsername().empty());
AFAICT, this was a bogus dcheck (except in the initial-sync-setup case). 
Reviewers tell me if I'm wrong!

[Andrew T Wilson (Slow), 2011-11-24 01:48:16]

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_flow.cc (right):

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_flow.cc:107: prefs::kGoogleServicesUsername));
I'm slightly nervous about this, as it's not 100% clear to me what the lifetime
of kGoogleServicesUsername is (I want to make sure we don't get into a state
where kGoogleServicesUsername gets set but sync can't sign in, and we're left in
a permanent state where the user can't log in as a different user).

Looks like we're guaranteed to clear out kGoogleServicesUsername if the user
cancels out of the setup dialog before sync setup completes
(PSS::OnUserCancelledDialog), but I'm not sure how this works in the case that
the user gets an auth error back from the server *after* sync setup has already
completed once.

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util.cc (left):

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:81:
DCHECK(service.GetAuthenticatedUsername().empty());
Not sure if this is a bogus DCHECK(), but I think it's fine to omit it.

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:144: prefs::kGoogleServicesUsername));
DCHECK(!username.empty()) ?

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util_unittest.cc (right):

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util_unittest.cc:55: return profile;
I wonder if this could be refactored into ProfileSyncServiceMock somehow, since
this seems identical to the code in sync_global_error_unittest.cc file?

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util_unittest.cc:136:
SCOPED_TRACE(base::StringPrintf("iteration: %d", (int)i));
Did you mean to leave this in?

[stevenjb, 2011-11-24 03:31:26]

I applied this patch, ran it on a chromeos device, and induced a browser crash.
The sync section of the settings page was visible and the about:sync Summary
says 'READY', so no regression, woot!

[tim (not reviewing), 2011-11-24 03:59:42]

Thanks for the review + big thanks for trying the patch Steven!

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_flow.cc (right):

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_flow.cc:107: prefs::kGoogleServicesUsername));
On 2011/11/24 01:48:16, Andrew T Wilson wrote:
> I'm slightly nervous about this, as it's not 100% clear to me what the
lifetime
> of kGoogleServicesUsername is (I want to make sure we don't get into a state
> where kGoogleServicesUsername gets set but sync can't sign in, and we're left
in
> a permanent state where the user can't log in as a different user).
> 
> Looks like we're guaranteed to clear out kGoogleServicesUsername if the user
> cancels out of the setup dialog before sync setup completes
> (PSS::OnUserCancelledDialog), but I'm not sure how this works in the case that
> the user gets an auth error back from the server *after* sync setup has
already
> completed once.

But in that case the username has to stay the same, so I think it's okay.  I
summarized my take in the cl description, I think that this only gets cleared on
DisableForUser which gets called (as you point out) if you cancel out of setup
or choose to stop syncing.

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util_unittest.cc (right):

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util_unittest.cc:55: return profile;
On 2011/11/24 01:48:16, Andrew T Wilson wrote:
> I wonder if this could be refactored into ProfileSyncServiceMock somehow,
since
> this seems identical to the code in sync_global_error_unittest.cc file?

Yeahh, in fact that method above is duplicated as well :(  I'll see what I can
do!

[Andrew T Wilson (Slow), 2011-11-24 17:14:08]

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_flow.cc (right):

http://codereview.chromium.org/8665011/diff/5001/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_flow.cc:107: prefs::kGoogleServicesUsername));
OK, if you're saying the user always has the option to sign out of sync (even in
the presence of an auth error) then this is fine.

[tim (not reviewing), 2011-12-01 08:03:19]

Addressed comments, I kind of took the lazy way to refactor into
ProfileSyncServiceMock, but let me know if you had something else in mind.

[Andrew T Wilson (Slow), 2011-12-01 23:08:07]

LGTM with a couple of nits.

http://codereview.chromium.org/8665011/diff/12001/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/8665011/diff/12001/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:140: string16 label;
Do we still need label?

http://codereview.chromium.org/8665011/diff/12001/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:145: prefs::kGoogleServicesUsername));
Might want to DCHECK(!user_name.empty())

http://codereview.chromium.org/8665011/diff/12001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc (right):

http://codereview.chromium.org/8665011/diff/12001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc:121:
prefs::kGoogleServicesUsername));
Do we need to check sync_initialized() here too? Or are we guaranteed that a
sync failure will always clear HasSyncSetupCompleted()?

[tim (not reviewing), 2011-12-02 02:19:58]

Thanks.  Fixed some tests that were added on rebase, will give 'em a whirl at
the trybots before committing.

http://codereview.chromium.org/8665011/diff/12001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc (right):

http://codereview.chromium.org/8665011/diff/12001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc:121:
prefs::kGoogleServicesUsername));
On 2011/12/01 23:08:07, Andrew T Wilson wrote:
> Do we need to check sync_initialized() here too? Or are we guaranteed that a
> sync failure will always clear HasSyncSetupCompleted()?

I don't know of a case where we leave sync setup completed = true but clear out
the username, so I think we're good.  Let me know if I'm missing your point...

[commit-bot: I haz the power, 2011-12-02 04:07:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/8665011/16001

[commit-bot: I haz the power, 2011-12-02 05:25:40]

Change committed as 112643