signin: pull basic SigninManager functionality into new SigninManagerBase class.

signin: pull basic SigninManager functionality into new SigninManagerBase class.

Removes chrome os ifdefs from SigninManager. SigninManagerBase provides
all necessary functionality for that platform.

I plan to change most callers of SigninManagerFactory::GetForProfile to use
GetBaseForProfile in a follow up CL, and ultimately have the factory only
create an SMB (versus a full SigninManager) on Chrome OS.

TBR=sky@chromium.org
for chrome/, chrome/browser, chrome/browser/ui, and chrome/test.

TBR=zelidrag@chromium.org
for chrome/browser/chromeos/login

TBR=mpcomplete@chromium.org
for chrome/browser/extensions/api/identity

BUG=181451, 174927
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195410

[tim (not reviewing), 2013-03-23 20:30:48]

Still tinkering, but ready for a first look!
(atwilson@ FYI / TBR when he returns from OOO).

As mentioned in the CL description, this is part one of a larger effort to
remove SigninManager on Chrome OS (and elsewhere, possibly?) in favor of
SigninManagerBase. This CL just factors out a base class, but doesn't change any
functionality as SigninManagerFactory still creates full SigninManagers. I'll
change that in my next CL.

Also note signin_manager_base doesn't contain many interesting changes (it's
mostly copy paste).  Initialize() and SignOut() are exceptions to that rule,
however.

[tim (not reviewing), 2013-03-26 17:37:00]

On 2013/03/23 20:30:48, timsteele wrote:
> Still tinkering, but ready for a first look!
> (atwilson@ FYI / TBR when he returns from OOO).
> 
> As mentioned in the CL description, this is part one of a larger effort to
> remove SigninManager on Chrome OS (and elsewhere, possibly?) in favor of
> SigninManagerBase. This CL just factors out a base class, but doesn't change
any
> functionality as SigninManagerFactory still creates full SigninManagers. I'll
> change that in my next CL.
> 
> Also note signin_manager_base doesn't contain many interesting changes (it's
> mostly copy paste).  Initialize() and SignOut() are exceptions to that rule,
> however.

ping @roger

[Roger Tawa OOO till Jul 10th, 2013-03-27 17:36:33]

Hi Tim, some questions below.

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

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:125: SigninManager::~SigninManager() { }
should } be on next line?

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.h (left):

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.h:8: // and the results are stored in the
TokenService.
Since the ClientLogin is staying as part of this class I wonder this upper part
of the comment should remain here.

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

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager_base.h:110: static void
DisableOneClickSignIn(Profile* profile);
one-click is not used for chromeos, so should probably be moved to derived
class.

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager_base.h:138: // it).
do we need to say weak here?

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager_factory.h (right):

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager_factory.h:25: static SigninManagerBase*
GetBaseForProfile(Profile* profile);
Instead of two methods GetBaseForProfile() and GetForProfile(), why not
something like:

#ifdef chromeos
  static SigninManagerBase* GetForProfile();
#else
  static SigninManager* GetForProfile();
#endif

Same for XXXIfExists().  I guess you're thinking that on desktop some callers
only need the "basic" functionality and could call GetBaseForProfile().  Not
sure if the extra complexity is worth it.

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager_factory.h:36: // rename these (to e.g
GetExtendedForProfile) and ifdef out on chrome os.
Why change BuildServiceInstanceFor() in a second step?

https://codereview.chromium.org/12502017/diff/2001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/12502017/diff/2001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:1710: 'browser/signin/signin_manager.h',
don't you need to add lines below that now exlude signin_manager.cc|h from
chromeos builds?

[tim (not reviewing), 2013-03-29 18:03:15]

Haven't uploaded patch set yet.  Was going to, but I pulled the thread of a
sweater and it unraveled :)  Stand by.

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

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:125: SigninManager::~SigninManager() { }
On 2013/03/27 17:36:33, Roger Tawa wrote:
> should } be on next line?

Done.

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.h (left):

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.h:8: // and the results are stored in the
TokenService.
On 2013/03/27 17:36:33, Roger Tawa wrote:
> Since the ClientLogin is staying as part of this class I wonder this upper
part
> of the comment should remain here.

Done.

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

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager_base.h:110: static void
DisableOneClickSignIn(Profile* profile);
On 2013/03/27 17:36:33, Roger Tawa wrote:
> one-click is not used for chromeos, so should probably be moved to derived
> class.

Actually I had already pulled it down but forgot to remove it here :( Good
catch. Fixed.

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager_base.h:138: // it).
On 2013/03/27 17:36:33, Roger Tawa wrote:
> do we need to say weak here?

Done.

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager_factory.h (right):

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager_factory.h:25: static SigninManagerBase*
GetBaseForProfile(Profile* profile);
On 2013/03/27 17:36:33, Roger Tawa wrote:
> Instead of two methods GetBaseForProfile() and GetForProfile(), why not
> something like:
> 
> #ifdef chromeos
>   static SigninManagerBase* GetForProfile();
> #else
>   static SigninManager* GetForProfile();
> #endif
> 
> Same for XXXIfExists().  I guess you're thinking that on desktop some callers
> only need the "basic" functionality and could call GetBaseForProfile().  Not
> sure if the extra complexity is worth it.

Right I noticed the vast majority of callsites just want Base functionality /
dependencies, so it seemed worth it to me. But I guess it'd be easier to do it
your way... I"ll change it.

https://codereview.chromium.org/12502017/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager_factory.h:36: // rename these (to e.g
GetExtendedForProfile) and ifdef out on chrome os.
On 2013/03/27 17:36:33, Roger Tawa wrote:
> Why change BuildServiceInstanceFor() in a second step?

I wanted to split it up to avoid a massive CL mixed with real changes and
renames. But with the ifdef suggestion I implemented I'll need to audit all the
callsites. It also looks like I'll have to refactor ProfileSyncServiceHarness,
since it's using StartSignin on all platforms.

I'll let you know when I have a working patch again.

https://codereview.chromium.org/12502017/diff/2001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/12502017/diff/2001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:1710: 'browser/signin/signin_manager.h',
On 2013/03/27 17:36:33, Roger Tawa wrote:
> don't you need to add lines below that now exlude signin_manager.cc|h from
> chromeos builds?

Yes, but I was doing this in two steps (see my comment in
signin_manager_factory).  I'll try to combine it now though.

[tim (not reviewing), 2013-04-03 17:41:07]

OK Roger, I think this is ready for another look.  

It ballooned in # of files, but the vast majority of the changes are just
s/SigninManager/SigninManagerBase.  I will TBR OWNERS after going through a
round of review with you.

I did change signin_manager.h in an interesting way -- the file exists on Chrome
OS but just short-circuits to include signin_manager_base, so no functional
difference to the previous patch set there.  On non Chrome OS platforms though,
this means any code that uses SigninManagerBase* (which is any code that is
compiled on both CrOS and desktop) can now simply include signin_manager.h
versus requiring every file to conditionally #include signin_manager.h or
signin_manager_base.h depending on the platform (so that the inheritance is
visible and the implicit upcast from derived (SigninManager) to base
(SigninManagerBase) works.

[tim (not reviewing), 2013-04-05 17:31:32]

+Pavel FYI

[Roger Tawa OOO till Jul 10th, 2013-04-05 20:53:57]

lgtm

Sorry for the delay.  A few minor nits below.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
File chrome/browser/policy/url_blacklist_manager.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
chrome/browser/policy/url_blacklist_manager.cc:69: #endif
Strange to ifdef out only this part.  Is this function needed at all in cros?

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

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.cc:384: return true;
call base class?

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager.h (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.h:8: // required by non non-Chrome OS
platforms.
"non non-Chrome OS platforms" --> "all platforms except Chrome OS" ?

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.h:10: // When a user is signed in, a
ClientLogin request is run on their behalf.
ClientLogin request is not used anymore.  Mention oauth2 instead, or point to
https://docs.google.com/a/google.com/document/d/1FDsNBK7iKydcbj5yLGSkeOHCaGTG...
?

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:296:
content::Details<const GoogleServiceSigninSuccessDetails>(&details));
this seems to be duped 3 times.  Maybe move into a helper function?

[tim (not reviewing), 2013-04-05 22:14:12]

Thanks Roger. Comments below. I'll wait for Drew to review policy/ (see comment
below) and then commit.  TBRing for the massive
s/SigninManager/SigninManagerBase rename and #ifdefing.

TBR=sky@chromium.org 
for chrome/, chrome/browser, chrome/browser/ui, and chrome/test. 

TBR=zelidrag@chromium.org 
for chrome/browser/chromeos/login

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
File chrome/browser/policy/url_blacklist_manager.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
chrome/browser/policy/url_blacklist_manager.cc:69: #endif
On 2013/04/05 20:53:57, Roger Tawa wrote:
> Strange to ifdef out only this part.  Is this function needed at all in cros?

I'm not sure.  CrOS still has a signin flow of its own, I don't know if this had
an impact, so for now I left it intact.  

Perhaps a c/b/policy OWNER can check this (atwilson?)

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

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.cc:384: return true;
On 2013/04/05 20:53:57, Roger Tawa wrote:
> call base class?

We don't want to do that.  This method replaces the base class, e.g we don't
want to return false on Windows just because the authenticated_username_ is
empty if we are in the process of signing in with client_login_.  Removing all
client_login stuff from here (once that flag is gone) will simplify this in the
future though.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager.h (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.h:8: // required by non non-Chrome OS
platforms.
On 2013/04/05 20:53:57, Roger Tawa wrote:
> "non non-Chrome OS platforms" --> "all platforms except Chrome OS" ?

Oh, that was one 'non's too many :)  All non Chrome OS platforms is what I
meant. I'll use your suggestion though.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.h:10: // When a user is signed in, a
ClientLogin request is run on their behalf.
On 2013/04/05 20:53:57, Roger Tawa wrote:
> ClientLogin request is not used anymore.  Mention oauth2 instead, or point to
>
https://docs.google.com/a/google.com/document/d/1FDsNBK7iKydcbj5yLGSkeOHCaGTG...
> ?

Actually after signin happens we still use client login to fetch Auth tokens...
but I can clean up this comment, add a TODO and reference 92948 and 226464.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:296:
content::Details<const GoogleServiceSigninSuccessDetails>(&details));
On 2013/04/05 20:53:57, Roger Tawa wrote:
> this seems to be duped 3 times.  Maybe move into a helper function?

Done.

[Joao da Silva, 2013-04-09 15:26:07]

Drive by comment on the URL blacklist.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
File chrome/browser/policy/url_blacklist_manager.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
chrome/browser/policy/url_blacklist_manager.cc:69: #endif
On 2013/04/05 22:14:12, timsteele wrote:
> On 2013/04/05 20:53:57, Roger Tawa wrote:
> > Strange to ifdef out only this part.  Is this function needed at all in
cros?
> 
> I'm not sure.  CrOS still has a signin flow of its own, I don't know if this
had
> an impact, so for now I left it intact.  
> 
> Perhaps a c/b/policy OWNER can check this (atwilson?)

This function is only used in this file (it's in the anonymous namespace) and
can be #ifdef'd away when !defined(OS_CHROMEOS). It's only called from line 390,
which will also need an #ifdef.

The blacklist only applies to the profile's request context, and ChromeOS signin
uses the system request context, so this has no impact at all on that.

[Andrew T Wilson (Slow), 2013-04-09 15:39:25]

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:78: }
Wrong indent.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
File chrome/browser/policy/url_blacklist_manager.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
chrome/browser/policy/url_blacklist_manager.cc:69: #endif
On 2013/04/05 22:14:12, timsteele wrote:
> On 2013/04/05 20:53:57, Roger Tawa wrote:
> > Strange to ifdef out only this part.  Is this function needed at all in
cros?
> 
> I'm not sure.  CrOS still has a signin flow of its own, I don't know if this
had
> an impact, so for now I left it intact.  
> 
> Perhaps a c/b/policy OWNER can check this (atwilson?)
Good question. I've asked joao to check.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager.h (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.h:12: // TokenService.
Googleand -> Google and

or Googleland! The tokeniest place on earth!

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.h:20: 
This seems weird - is this just to avoid changing a bunch of #include
signin_manager.h to #include signin_manager_base.h?

I don't think there are many places in the codebase where we *actually* want to
conditionally include SigninManager vs SigninManagerBase.

If we're  just doing this to avoid touching lots of files, can you add a TODO to
get rid of this at least?

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager_base.h (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_base.h:106: virtual void SignOut();
I'm uneasy that we are still leaving this enabled on chromeos, since my
understanding is that the user can't signout on that platform.

Is there a reason we don't move this into SigninManager proper?

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1147: bool
signout_prohibited = false;
Actually, I think signout should always be prohibited on cros (so, we should
default to true). I think the UI is already hidden on cros, but it doesn't hurt
to set the values correctly.

[tim (not reviewing), 2013-04-16 03:28:29]

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:78: }
On 2013/04/09 15:39:25, Andrew T Wilson wrote:
> Wrong indent.

Done.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
File chrome/browser/policy/url_blacklist_manager.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
chrome/browser/policy/url_blacklist_manager.cc:69: #endif
On 2013/04/09 15:39:25, Andrew T Wilson wrote:
> On 2013/04/05 22:14:12, timsteele wrote:
> > On 2013/04/05 20:53:57, Roger Tawa wrote:
> > > Strange to ifdef out only this part.  Is this function needed at all in
> cros?
> > 
> > I'm not sure.  CrOS still has a signin flow of its own, I don't know if this
> had
> > an impact, so for now I left it intact.  
> > 
> > Perhaps a c/b/policy OWNER can check this (atwilson?)
> Good question. I've asked joao to check.

Done.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
chrome/browser/policy/url_blacklist_manager.cc:69: #endif
On 2013/04/09 15:26:07, Joao da Silva wrote:
> On 2013/04/05 22:14:12, timsteele wrote:
> > On 2013/04/05 20:53:57, Roger Tawa wrote:
> > > Strange to ifdef out only this part.  Is this function needed at all in
> cros?
> > 
> > I'm not sure.  CrOS still has a signin flow of its own, I don't know if this
> had
> > an impact, so for now I left it intact.  
> > 
> > Perhaps a c/b/policy OWNER can check this (atwilson?)
> 
> This function is only used in this file (it's in the anonymous namespace) and
> can be #ifdef'd away when !defined(OS_CHROMEOS). It's only called from line
390,
> which will also need an #ifdef.
> 
> The blacklist only applies to the profile's request context, and ChromeOS
signin
> uses the system request context, so this has no impact at all on that.

Done.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager.h (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.h:20: 
On 2013/04/09 15:39:25, Andrew T Wilson wrote:
> This seems weird - is this just to avoid changing a bunch of #include
> signin_manager.h to #include signin_manager_base.h?
> 
> I don't think there are many places in the codebase where we *actually* want
to
> conditionally include SigninManager vs SigninManagerBase.
> 
> If we're  just doing this to avoid touching lots of files, can you add a TODO
to
> get rid of this at least?

This is what I was talking about in my earlier message on this code review. This
is needed because on desktop, you need to know SigninManager is a subclass of
SigninManagerBase.

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12502017/diff/111002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1147: bool
signout_prohibited = false;
On 2013/04/09 15:39:25, Andrew T Wilson wrote:
> Actually, I think signout should always be prohibited on cros (so, we should
> default to true). I think the UI is already hidden on cros, but it doesn't
hurt
> to set the values correctly.

If I understand correctly, the way it works now is "prohibit signout" is an
action only supported by the full-fledged desktop SigninManager, used by one of
the user policy services.  So there is no explicit restriction in the code w.r.t
signing out on cros.  

I'm happy to do that / look into moving SignOut out of SigninManagerBase if that
makes sense, but can I do it in a follow up?  It will require ifdefing or
splitting up more tests that try to test both platforms from each platform (e.g
ProfileSyncServiceStartupTest).

[tim (not reviewing), 2013-04-17 16:30:04]

On 2013/04/16 03:28:29, timsteele wrote:
>
https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/cl...
> File chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc
(right):
> 
>
https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/cl...
> chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:78: }
> On 2013/04/09 15:39:25, Andrew T Wilson wrote:
> > Wrong indent.
> 
> Done.
> 
>
https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
> File chrome/browser/policy/url_blacklist_manager.cc (right):
> 
>
https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
> chrome/browser/policy/url_blacklist_manager.cc:69: #endif
> On 2013/04/09 15:39:25, Andrew T Wilson wrote:
> > On 2013/04/05 22:14:12, timsteele wrote:
> > > On 2013/04/05 20:53:57, Roger Tawa wrote:
> > > > Strange to ifdef out only this part.  Is this function needed at all in
> > cros?
> > > 
> > > I'm not sure.  CrOS still has a signin flow of its own, I don't know if
this
> > had
> > > an impact, so for now I left it intact.  
> > > 
> > > Perhaps a c/b/policy OWNER can check this (atwilson?)
> > Good question. I've asked joao to check.
> 
> Done.
> 
>
https://codereview.chromium.org/12502017/diff/111002/chrome/browser/policy/ur...
> chrome/browser/policy/url_blacklist_manager.cc:69: #endif
> On 2013/04/09 15:26:07, Joao da Silva wrote:
> > On 2013/04/05 22:14:12, timsteele wrote:
> > > On 2013/04/05 20:53:57, Roger Tawa wrote:
> > > > Strange to ifdef out only this part.  Is this function needed at all in
> > cros?
> > > 
> > > I'm not sure.  CrOS still has a signin flow of its own, I don't know if
this
> > had
> > > an impact, so for now I left it intact.  
> > > 
> > > Perhaps a c/b/policy OWNER can check this (atwilson?)
> > 
> > This function is only used in this file (it's in the anonymous namespace)
and
> > can be #ifdef'd away when !defined(OS_CHROMEOS). It's only called from line
> 390,
> > which will also need an #ifdef.
> > 
> > The blacklist only applies to the profile's request context, and ChromeOS
> signin
> > uses the system request context, so this has no impact at all on that.
> 
> Done.
> 
>
https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
> File chrome/browser/signin/signin_manager.h (right):
> 
>
https://codereview.chromium.org/12502017/diff/111002/chrome/browser/signin/si...
> chrome/browser/signin/signin_manager.h:20: 
> On 2013/04/09 15:39:25, Andrew T Wilson wrote:
> > This seems weird - is this just to avoid changing a bunch of #include
> > signin_manager.h to #include signin_manager_base.h?
> > 
> > I don't think there are many places in the codebase where we *actually* want
> to
> > conditionally include SigninManager vs SigninManagerBase.
> > 
> > If we're  just doing this to avoid touching lots of files, can you add a
TODO
> to
> > get rid of this at least?
> 
> This is what I was talking about in my earlier message on this code review.
This
> is needed because on desktop, you need to know SigninManager is a subclass of
> SigninManagerBase.
> 
>
https://codereview.chromium.org/12502017/diff/111002/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/options/browser_options_handler.cc (right):
> 
>
https://codereview.chromium.org/12502017/diff/111002/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/options/browser_options_handler.cc:1147: bool
> signout_prohibited = false;
> On 2013/04/09 15:39:25, Andrew T Wilson wrote:
> > Actually, I think signout should always be prohibited on cros (so, we should
> > default to true). I think the UI is already hidden on cros, but it doesn't
> hurt
> > to set the values correctly.
> 
> If I understand correctly, the way it works now is "prohibit signout" is an
> action only supported by the full-fledged desktop SigninManager, used by one
of
> the user policy services.  So there is no explicit restriction in the code
w.r.t
> signing out on cros.  
> 
> I'm happy to do that / look into moving SignOut out of SigninManagerBase if
that
> makes sense, but can I do it in a follow up?  It will require ifdefing or
> splitting up more tests that try to test both platforms from each platform
(e.g
> ProfileSyncServiceStartupTest).

I've been trying to rebase this atop
https://chromiumcodereview.appspot.com/14029006 and a few other recent
earth-shattering changes to SigninManager but having a pretty hard time.  Drew,
if you have any pointers as to how the new cancellation stuff works that would
be great...

Assuming people think this patch is still worthwhile, ping on the review to make
sure there's nothing else holding it back (in the pre-rebase version).

[tim (not reviewing), 2013-04-17 21:44:34]

Rebased atop recent SigninManager changes; all comments addressed.  Drew, PTAL.

[Matt Perry, 2013-04-17 21:58:30]

extensions LGTM

[Andrew T Wilson (Slow), 2013-04-18 13:28:07]

So, I really think we need to fix the SignOut() situation. I'm OK with doing
this in a separate CL if you want to get this landed and stop being stuck in
merge hell, but removing SignOut() from ChromeOS was one of my core motivations
for proposing this kind of refactoring.

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

https://codereview.chromium.org/12502017/diff/148005/chrome/browser/signin/si...
chrome/browser/signin/signin_manager.cc:393: return;
Not to keep beating on the SignOut drum, but it's fragile to have a derived
class handler not call its superclass, as it assumes that the derived class has
completely knowledge of everything the superclass does and can guarantee that
it's safe not to call the superclass.

Which is fine in some situations, but I think is unnecessary in this case,
especially if we make the base class implementation empty.

https://codereview.chromium.org/12502017/diff/148005/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager_base.cc (right):

https://codereview.chromium.org/12502017/diff/148005/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_base.cc:201: DCHECK(IsInitialized());
Just want to re-interate that I'm still nervous about SignOut being defined for
ChromeOS. Can we put a TODO here with a bug to remove this (this probably means
that the code in this file that calls SignOut should probably live in the
derived class too).

In fact, I don't really even understand why we have SignOut() defined in
SigninManagerBase - SignOut() is not allowed on ChromeOS. If we need to define
it on the base class for weird dependency reasons, that's fine, but the base
class implementation should be empty and the stuff we do here should be moved to
the derived class.

This also protects us from weird cases where somebody calls
SigninManagerBase::SignOut() directly to circumvent ProhibitSignout(), or where
some bug in our code or a misconfigured policy results in chrome-on-cros
thinking it is signed out.

[tim (not reviewing), 2013-04-18 18:27:02]

https://codereview.chromium.org/12502017/diff/148005/chrome/browser/signin/si...
File chrome/browser/signin/signin_manager_base.cc (right):

https://codereview.chromium.org/12502017/diff/148005/chrome/browser/signin/si...
chrome/browser/signin/signin_manager_base.cc:201: DCHECK(IsInitialized());
On 2013/04/18 13:28:07, Andrew T Wilson wrote:
> Just want to re-interate that I'm still nervous about SignOut being defined
for
> ChromeOS. Can we put a TODO here with a bug to remove this (this probably
means
> that the code in this file that calls SignOut should probably live in the
> derived class too).
> 
> In fact, I don't really even understand why we have SignOut() defined in
> SigninManagerBase - SignOut() is not allowed on ChromeOS. If we need to define
> it on the base class for weird dependency reasons, that's fine, but the base
> class implementation should be empty and the stuff we do here should be moved
to
> the derived class.
> 
> This also protects us from weird cases where somebody calls
> SigninManagerBase::SignOut() directly to circumvent ProhibitSignout(), or
where
> some bug in our code or a misconfigured policy results in chrome-on-cros
> thinking it is signed out.

I'd like to try moving it to the derived class as well.  I added a TODO. I
already feel like I bit off more than I could chew in this patch; I was
originally just trying to fix bug 181451 :P I'd like to take baby steps :) 
Removing SignOut will require changing Initialize /
IsSigninAllowed/GoogleServicesUsernamePattern callsites or moving them to the
derived class, but it's a bit of a sweater-thread pull so if I could do it in a
separate CL I'd strongly prefer it.

[Andrew T Wilson (Slow), 2013-04-19 11:22:07]

Apologies, I should have added an explicit LGTM to my previous comment since it
was inherent in my "I'm OK with removing SignOut in a separate CL".

[commit-bot: I haz the power, 2013-04-19 17:11:10]

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

[commit-bot: I haz the power, 2013-04-19 17:11:22]

Failed to apply patch for chrome/browser/signin/signin_manager_fake.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: chrome/browser/signin/signin_manager_fake.cc
  |diff --git a/chrome/browser/signin/signin_manager_fake.cc
b/chrome/browser/signin/signin_manager_fake.cc
  |index
0e236588e7c86733cf0f194a661fe6e3a08b0a01..01d8867eabc4224ff70748a1babf6f208cea5be9
100644
  |--- a/chrome/browser/signin/signin_manager_fake.cc
  |+++ b/chrome/browser/signin/signin_manager_fake.cc
  --------------------------
  No file to patch.  Skipping patch.
  3 out of 3 hunks ignored

Patch:       chrome/browser/signin/signin_manager_fake.cc
Index: chrome/browser/signin/signin_manager_fake.cc
diff --git a/chrome/browser/signin/signin_manager_fake.cc
b/chrome/browser/signin/signin_manager_fake.cc
index
0e236588e7c86733cf0f194a661fe6e3a08b0a01..01d8867eabc4224ff70748a1babf6f208cea5be9
100644
--- a/chrome/browser/signin/signin_manager_fake.cc
+++ b/chrome/browser/signin/signin_manager_fake.cc
@@ -14,18 +14,18 @@
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_service.h"
 
-FakeSigninManager::FakeSigninManager(Profile* profile)
+FakeSigninManagerBase::FakeSigninManagerBase(Profile* profile)
     : auth_in_progress_(false) {
   profile_ = profile;
   signin_global_error_.reset(new SigninGlobalError(this, profile));
   GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(
       signin_global_error_.get());
   signin_allowed_.Init(prefs::kSigninAllowed, profile_->GetPrefs(),
-                       base::Bind(&SigninManager::OnSigninAllowedPrefChanged,
-                                  base::Unretained(this)));
+      base::Bind(&SigninManagerBase::OnSigninAllowedPrefChanged,
+      base::Unretained(this)));
 }
 
-FakeSigninManager::~FakeSigninManager() {
+FakeSigninManagerBase::~FakeSigninManagerBase() {
   if (signin_global_error_.get()) {
     GlobalErrorServiceFactory::GetForProfile(profile_)->RemoveGlobalError(
         signin_global_error_.get());
@@ -33,6 +33,35 @@ FakeSigninManager::~FakeSigninManager() {
   }
 }
 
+void FakeSigninManagerBase::SignOut() {
+  authenticated_username_.clear();
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_GOOGLE_SIGNED_OUT,
+      content::Source<Profile>(profile_),
+      content::NotificationService::NoDetails());
+}
+
+bool FakeSigninManagerBase::AuthInProgress() const {
+  return auth_in_progress_;
+}
+
+// static
+ProfileKeyedService* FakeSigninManagerBase::Build(Profile* profile) {
+  return new FakeSigninManagerBase(profile);
+}
+
+#if !defined (OS_CHROMEOS)
+
+FakeSigninManager::FakeSigninManager(Profile* profile) {
+  Initialize(profile);
+}
+
+FakeSigninManager::~FakeSigninManager() {
+}
+
+void FakeSigninManager::InitTokenService() {
+}
+
 void FakeSigninManager::StartSignIn(const std::string& username,
                                     const std::string& password,
                                     const std::string& login_token,
@@ -62,17 +91,9 @@ void FakeSigninManager::SignOut() {
       content::NotificationService::NoDetails());
 }
 
-void FakeSigninManager::ForceSignOut() {
-  // Allow signing out now.
-  prohibit_signout_ = false;
-  SignOut();
-}
-
-bool FakeSigninManager::AuthInProgress() const {
-  return auth_in_progress_;
-}
-
 // static
 ProfileKeyedService* FakeSigninManager::Build(Profile* profile) {
   return new FakeSigninManager(profile);
 }
+
+#endif  // !defined (OS_CHROMEOS)

[commit-bot: I haz the power, 2013-04-19 18:08:12]

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

[commit-bot: I haz the power, 2013-04-19 18:35:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-19 18:37:16]

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

[commit-bot: I haz the power, 2013-04-19 19:05:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-19 20:50:14]

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

[commit-bot: I haz the power, 2013-04-19 21:12:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-19 21:26:44]

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

[commit-bot: I haz the power, 2013-04-19 21:28:41]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-04-19 21:50:23]

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

[commit-bot: I haz the power, 2013-04-20 01:41:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-21 07:54:19]

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

[commit-bot: I haz the power, 2013-04-21 08:01:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[tim (not reviewing), 2013-04-21 08:07:32]

Committed patchset #14 manually as r195410 (presubmit successful).