Issue 14630003: signin: move SigninManagerBase::Signout to SigninManager.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+436 lines, -416 lines)			Patch
M	chrome/browser/policy/cloud/user_cloud_policy_store_unittest.cc	View	1 2 3	2 chunks	+5 lines, -1 line	0 comments	Download
M	chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/signin/fake_signin_manager.h	View	1 2 3	1 chunk	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/signin/fake_signin_manager.cc	View	1 2 3	2 chunks	+3 lines, -21 lines	0 comments	Download
M	chrome/browser/signin/signin_global_error_unittest.cc	View	1 2 3	3 chunks	+5 lines, -1 line	0 comments	Download
M	chrome/browser/signin/signin_manager.h	View	1	6 chunks	+31 lines, -1 line	0 comments	Download
M	chrome/browser/signin/signin_manager.cc	View	1	2 chunks	+112 lines, -1 line	0 comments	Download
M	chrome/browser/signin/signin_manager_base.h	View	1	4 chunks	+12 lines, -31 lines	0 comments	Download
M	chrome/browser/signin/signin_manager_base.cc	View	1	7 chunks	+2 lines, -108 lines	0 comments	Download
D	chrome/browser/signin/signin_manager_base_unittest.cc	View		1 chunk	+0 lines, -92 lines	0 comments	Download
M	chrome/browser/signin/signin_manager_unittest.cc	View	1	1 chunk	+45 lines, -0 lines	0 comments	Download
M	chrome/browser/signin/signin_tracker_unittest.cc	View	1 2 3	3 chunks	+2 lines, -3 lines	0 comments	Download
M	chrome/browser/sync/profile_sync_service.cc	View	1	1 chunk	+11 lines, -2 lines	0 comments	Download
M	chrome/browser/sync/profile_sync_service_startup_unittest.cc	View	1 2 3	14 chunks	+125 lines, -83 lines	0 comments	Download
M	chrome/browser/sync/profile_sync_service_unittest.cc	View	1	2 chunks	+7 lines, -1 line	0 comments	Download
M	chrome/browser/sync/sync_ui_util_unittest.cc	View	1 2 3	8 chunks	+16 lines, -10 lines	0 comments	Download
M	chrome/browser/ui/sync/one_click_signin_helper.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/sync/one_click_signin_helper_unittest.cc	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/ui/webui/sync_setup_handler.h	View		2 chunks	+6 lines, -6 lines	0 comments	Download
M	chrome/browser/ui/webui/sync_setup_handler.cc	View	1	5 chunks	+10 lines, -1 line	0 comments	Download
M	chrome/browser/ui/webui/sync_setup_handler_unittest.cc	View	1 2	12 chunks	+32 lines, -48 lines	0 comments	Download
M	chrome/chrome_tests_unit.gypi	View	1	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/test/base/chrome_render_view_host_test_harness.cc	View	1 2 3	1 chunk	+6 lines, -2 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

Andrew T Wilson (Slow)

LGTM - ideally we'd move IsSigninAllowed() out of SigninManagerBase too, but I'm OK with leaving ...

7 years, 7 months ago (2013-05-06 09:06:00 UTC) #2

tim (not reviewing)

Addressed comments + rebased, running through try bots. Also, I'll amend the per-file settings in ...

7 years, 7 months ago (2013-05-06 17:28:57 UTC) #3

Addressed comments + rebased, running through try bots.  

Also, I'll amend the per-file settings in the c/b/ui/webui/ OWNERS file to
account for sync_setup_handler_unittest (I think it currently only captures the
.h and .cc files.)

+James for TBR OWNERS stamp on that file in the meantime.

https://codereview.chromium.org/14630003/diff/5001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/14630003/diff/5001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:375: TokenService* token_service =
TokenServiceFactory::GetForProfile(profile_);
On 2013/05/06 09:06:00, Andrew T Wilson wrote:
> Maybe move this down to line 380 - no reason to grab it here before sending
out
> the notification.

Done.

https://codereview.chromium.org/14630003/diff/5001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager_base.h (right):

https://codereview.chromium.org/14630003/diff/5001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager_base.h:78: // TODO(tim): If the
kSigninAllowed pref was non-Chrome OS-only, this
On 2013/05/06 09:06:00, Andrew T Wilson wrote:
> I'm confused by this comment. I believe that kSigninAllowed is only supported
on
> non-ChromeOS platforms (i.e. IsSigninAllowed() should not be called on
ChromeOS
> since it's meaningless on that platform). Can you clarify the wording?
> 
> Maybe add a TODO/log a bug to move this to SigninManager too?

Done. Using bug 174927.

https://codereview.chromium.org/14630003/diff/5001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager_base.h:119: void
clear_authenticated_username();
On 2013/05/06 09:06:00, Andrew T Wilson wrote:
> Should document here why one would call this and not
SetAuthenticatedUsername().

Done.

https://codereview.chromium.org/14630003/diff/5001/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/14630003/diff/5001/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.cc:1133: // TODO(tim): Is this path
even reachable on Chrome OS?
On 2013/05/06 09:06:00, Andrew T Wilson wrote:
> Possibly - the way I'd guess is:
> 
> 1) User does dashboard clear
> 2) User clicks to re-enable sync, going through Advanced path.
> 3) User cancels out of Advanced dialog.
> 
> I think doing nothing here on ChromeOS is OK.

Done.