Add a SigninAllowed policy.

Add a signin allowed policy which is used to determine if a user is allowed to sign in to Chrome. This will be used for example for one preference for managed user settings.

BUG=171370
TEST=unit_tests, browser_tests, manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185359

[Adrian Kuegel, 2013-01-29 13:41:53]

I added code for the "no signin" preference. Can you please take a look if this
is ok?

[Pam (message me for reviews), 2013-01-30 10:03:40]

Unfortunately, it's a little more complicated than this. Signing in controls
more than just sync, and as long as we're putting in a way to prevent signin,
I'd say we should make it a new admin policy, rather than something only
available to managed users.

Drew just recently did some work to make the no-sync policy *not* also mean
no-signin. See http://crbug.com/166148 . He directly, or that patch, would be
good places to look for info.

https://codereview.chromium.org/12088040/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/12088040/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_service.cc:117:
prefs->RegisterBooleanPref(prefs::kManagedUserNoSignin,
There are a lot of old counterexamples, but we've decided more recently to
express prefs and policies as "allow" rather than "disable", to avoid
double-negatives ("disable is true").

[Andrew T Wilson (Slow), 2013-01-30 10:30:33]

One additional point - do you need to make sure that users get signed out if
they are already signed in? Perhaps modifying SigninManager to call SignOut() in
its Initialize() method if this pref is set?

https://codereview.chromium.org/12088040/diff/2001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/12088040/diff/2001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.cc:134: return false;
I have a CL in flight that changes how this works
(https://codereview.chromium.org/12077030/) so changing IsSyncAccessible() will
stop sync from starting up but won't prohibit signin.

My suggestion is to add a new API ala "IsSigninAllowed()" and call that API from
every place in the UI that IsSyncAccessible() is currently called, like so:

if (IsSyncAccessible() && IsSigninAllowed()) {
  ...do signin ui stuff...
}

Then when we merge with my CL, we can just change that code to:

if (IsSigninAllowed()) {
   ...do signin ui stuff...
}

[Adrian Kuegel, 2013-01-30 10:34:21]

On 2013/01/30 10:30:33, Andrew T Wilson wrote:
> One additional point - do you need to make sure that users get signed out if
> they are already signed in? Perhaps modifying SigninManager to call SignOut()
in
> its Initialize() method if this pref is set?

I am not sure about this, but it seems like the right thing to do.
@Pam: what do you think?

>
https://codereview.chromium.org/12088040/diff/2001/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile.cc (right):
> 
>
https://codereview.chromium.org/12088040/diff/2001/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile.cc:134: return false;
> I have a CL in flight that changes how this works
> (https://codereview.chromium.org/12077030/) so changing IsSyncAccessible()
will
> stop sync from starting up but won't prohibit signin.
> 
> My suggestion is to add a new API ala "IsSigninAllowed()" and call that API
from
> every place in the UI that IsSyncAccessible() is currently called, like so:
> 
> if (IsSyncAccessible() && IsSigninAllowed()) {
>   ...do signin ui stuff...
> }

Ok, thanks for confirming this. This answers my question in the email I just
wrote to you :-)

> Then when we merge with my CL, we can just change that code to:
> 
> if (IsSigninAllowed()) {
>    ...do signin ui stuff...
> }

Ok, so I will do the change as you suggested.

[Pam (message me for reviews), 2013-01-30 10:36:15]

On 2013/01/30 10:34:21, Adrian Kuegel wrote:
> On 2013/01/30 10:30:33, Andrew T Wilson wrote:
> > One additional point - do you need to make sure that users get signed out if
> > they are already signed in? Perhaps modifying SigninManager to call
SignOut()
> in
> > its Initialize() method if this pref is set?
> 
> I am not sure about this, but it seems like the right thing to do.
> @Pam: what do you think?

Sounds right to me too.

Do you think we should put this in as a user pref, or is there enough value in
adding a policy?

[Andrew T Wilson (Slow), 2013-01-30 10:51:45]

On 2013/01/30 10:36:15, Pam wrote:
> Do you think we should put this in as a user pref, or is there enough value in
> adding a policy?
I personally think it should be a policy, and I've been pushing for making it a
policy for a while now but haven't gotten much interest from the PMs. I think
that when enterprise customers find out that SyncDisabled doesn't prevent signin
any more, some of them will freak out.

So if you can make it a policy, that'd be awesome.

[Adrian Kuegel, 2013-02-04 15:03:57]

I uploaded a new patch. Automatic tests are still TODO, but I tested manually
and it seems to work. Can you please take another look at it?

[Andrew T Wilson (Slow), 2013-02-04 15:41:48]

Overall looks pretty good. I had a few concerns here and there.

The biggest question I had is whether we actually need that SigninPrefObserver
or if objects like BrowserCommandController can just use regular pref
observers/registrars to listen for changes to that pref.

Is there a good reason to define a new observer class?

https://codereview.chromium.org/12088040/diff/17001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:8225: +        Allow signing into Chrome
Don't put the product name in generated_resources.grd - it should be in
chromium_strings.grd and google_chrome_strings.grd.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.cc:776: if (!IsSigninAllowed())
Is this called during startup? You might want to add code to Initialize() to
sign out the user if signin is not allowed (see the call to IsAllowedUsername()
in that routine).

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:146: profile_->IsSyncAccessible() &&
profile_->IsSigninAllowed() ?
No need to check IsSigninAllowed() here - leave it as IsSyncAccessible(), since
we're really just checking to see if there's a ProfileSyncService.

If you want to add a check to IsSigninAllowed(), then do it above:

if (signin->GetAuthenticatedUsername().empty() || !signin->IsSigninAllowed()) {
   ... failure
}

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_ui_util.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
chrome/browser/signin/signin_ui_util.cc:41: if (profile->IsSyncAccessible() &&
profile->IsSigninAllowed()) {
Don't check IsSigninAllowed() here.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
chrome/browser/signin/signin_ui_util.cc:58: profile->IsSyncAccessible() &&
profile->IsSigninAllowed() ?
Don't check IsSigninAllowed() here.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.cc:220:
signin->AddSigninPrefObserver(this);
I don't think we want to define a new SigninPrefObserver. I think it's probably
better to just have BrowserCommandController listen to changes to the pref
directly. That's what we do elsewhere in this file for things like
kIncognitoModeAvailability.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/wrench_menu_model.cc:539: // No "Sign in to
Chromium..." menu item on ChromeOS.
Move this comment one line up so it's clear it refers to the #ifdef.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:254:
profile->GetOriginalProfile()->IsSigninAllowed();
Probably don't need to call GetOriginalProfile() here since we know that the
profile is not off the record, but it's fine as-is.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1081: !!service &&
(!signin || signin->IsSigninAllowed()));
I don't think that signin can be null here so no need to check for it. If you
want to add a DCHECK(signin) that's fine.

[Andrew T Wilson (Slow), 2013-02-05 08:58:53]

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1081: !!service &&
(!signin || signin->IsSigninAllowed()));
On 2013/02/04 15:41:48, Andrew T Wilson wrote:
> I don't think that signin can be null here so no need to check for it. If you
> want to add a DCHECK(signin) that's fine.
Also, I'm changing the JS code to not hide the signin UI when sync is disabled.
So you'll want to pass a separate signinDisabled flag to browser_options.js and
use that to hide the signin UI rather than trying to piggy-back on
syncSystemEnabled.

[Adrian Kuegel, 2013-02-05 10:55:15]

I uploaded a new patch which hopefully adresses the review comments.

https://codereview.chromium.org/12088040/diff/17001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:8225: +        Allow signing into Chrome
Done.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.cc:776: if (!IsSigninAllowed())
True, I missed that. Thanks for pointing it out. I added it to that condition in
the Initialize() method.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:146: profile_->IsSyncAccessible() &&
profile_->IsSigninAllowed() ?
On 2013/02/04 15:41:48, Andrew T Wilson wrote:
> No need to check IsSigninAllowed() here - leave it as IsSyncAccessible(),
since
> we're really just checking to see if there's a ProfileSyncService.
> 
> If you want to add a check to IsSigninAllowed(), then do it above:
> 
> if (signin->GetAuthenticatedUsername().empty() || !signin->IsSigninAllowed())
{
>    ... failure
> }

Done.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_ui_util.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
chrome/browser/signin/signin_ui_util.cc:41: if (profile->IsSyncAccessible() &&
profile->IsSigninAllowed()) {
On 2013/02/04 15:41:48, Andrew T Wilson wrote:
> Don't check IsSigninAllowed() here.

Done.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/signin/sig...
chrome/browser/signin/signin_ui_util.cc:58: profile->IsSyncAccessible() &&
profile->IsSigninAllowed() ?
On 2013/02/04 15:41:48, Andrew T Wilson wrote:
> Don't check IsSigninAllowed() here.

Done.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.cc:220:
signin->AddSigninPrefObserver(this);
I removed the SigninPrefObserver now. You are right, we don't need it, I just
used the ProfileSyncServiceObserver as an example and I thought I should do it
similarly.
Anyway, I think here I cannot use the profile_pref_registrar, because it would
not work in incognito mode. I need an observer of the preferences of the
original profile.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/wrench_menu_model.cc:539: // No "Sign in to
Chromium..." menu item on ChromeOS.
On 2013/02/04 15:41:48, Andrew T Wilson wrote:
> Move this comment one line up so it's clear it refers to the #ifdef.

Done.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:254:
profile->GetOriginalProfile()->IsSigninAllowed();
True, this is not necessary. I removed it, now it fits in one line.

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12088040/diff/17001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1081: !!service &&
(!signin || signin->IsSigninAllowed()));
On 2013/02/04 15:41:48, Andrew T Wilson wrote:
> I don't think that signin can be null here so no need to check for it. If you
> want to add a DCHECK(signin) that's fine.

Done.

[Adrian Kuegel, 2013-02-06 16:07:33]

I added some tests and fixed a few more issues. Can you please take another look
at it?

[Andrew T Wilson (Slow), 2013-02-07 10:08:11]

Very close after addressing comments below - can you sync with ToT and
re-upload? I'd like to see what BrowserOptionsHandler looks like post-merge.

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler.cc (right):

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler.cc:570: if (value &&
value->GetAsBoolean(&signin_allowed))
This is fine with me, but I wonder if this boilerplate is really necessary. You
might ask one of the policy folks like Joao if there's a way to do this with
less boilerplate.

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/signin/sig...
File chrome/browser/signin/signin_ui_util.cc (right):

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/signin/sig...
chrome/browser/signin/signin_ui_util.cc:58: profile->IsSyncAccessible() ?
This formatting change is fine, but you might want to revert this file to avoid
churn in the revision log.

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller_unittest.cc:193:
profile2->set_incognito(true);
nit: This is the wrong way to create an incognito profile, because you end up
with services created that can't easily shutdown because they can no longer
access their dependencies because those dependencies don't exist on incognito
profiles.

Use TestingProfile.Builder to create an incognito profile instead.

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1077: DCHECK(signin);
I believe signin can be NULL for chromeos guest profiles. If you sync with ToT
you'll see a fix I landed for this case.

[Adrian Kuegel, 2013-02-07 11:09:20]

I rebased to ToT, and fixed the issues you mentioned. Patch number 8 can be
ignored, somehow the upload script crashed, so I had to redo the upload which
lead to Patch number 9.

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler.cc (right):

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler.cc:570: if (value &&
value->GetAsBoolean(&signin_allowed))
Yes, I will do that. I did this part just in the same way as I have seen for the
syncManaged policy, so I didn't look into it if there is a better way.

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/signin/sig...
File chrome/browser/signin/signin_ui_util.cc (right):

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/signin/sig...
chrome/browser/signin/signin_ui_util.cc:58: profile->IsSyncAccessible() ?
I should have checked that I reformat it the same way it was before. This was
not an intended change, and I reverted the file.

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller_unittest.cc:193:
profile2->set_incognito(true);
On 2013/02/07 10:08:11, Andrew T Wilson wrote:
> nit: This is the wrong way to create an incognito profile, because you end up
> with services created that can't easily shutdown because they can no longer
> access their dependencies because those dependencies don't exist on incognito
> profiles.
> 
> Use TestingProfile.Builder to create an incognito profile instead.

Done.

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12088040/diff/32004/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1077: DCHECK(signin);
I merged it to ToT. Hopefully correctly :-)

[Adrian Kuegel, 2013-02-07 15:01:35]

@Joao: can you please take a look at the files of this changelist in
chrome/browser/policy , chrome/app/policy and chrome/test/data/policy ?
Is this the right thing to do to add a new policy?

[Andrew T Wilson (Slow), 2013-02-07 15:05:30]

LGTM with one nit.

https://codereview.chromium.org/12088040/diff/43002/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12088040/diff/43002/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:667:
base::Bind(&BrowserOptionsHandler::SetupProxySettingsSection,
Indent is off here now.

[Joao da Silva, 2013-02-07 15:36:36]

https://codereview.chromium.org/12088040/diff/43002/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler.h (right):

https://codereview.chromium.org/12088040/diff/43002/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler.h:225: class
SigninPolicyHandler: public TypeCheckingPolicyHandler {
This class isn't ever instantiated, so the policy won't work.

The class is also not necessary, since the policy maps directly to a boolean
pref; adding an entry for this at kSimplePolicyMap (in
configuration_policy_handler_list.cc) is enough.

[Andrew T Wilson (Slow), 2013-02-07 15:51:02]

Joao - so why do we have a special class for the SyncDisabled policy? Shouldn't
that just be a simple boolean pref mapping too?

[Joao da Silva, 2013-02-07 16:03:43]

On 2013/02/07 15:51:02, Andrew T Wilson wrote:
> Joao - so why do we have a special class for the SyncDisabled policy?
Shouldn't
> that just be a simple boolean pref mapping too?

I think the only reason left for that is sync_section.html.

It contains the button to setup sync that appears in the settings, and if the
pref backing the button is managed then the button becomes disabled. But if the
policy is set to false then we don't want to disable the button; so this hack
was introduced to prevent that.

I'm not sure this is still needed anymore, will have a look.

[Adrian Kuegel, 2013-02-07 16:08:59]

I just uploaded a new patch which addresses your comments. I also found that I
needed to initialize the signin_allowed pref member variable in the
one_click_signin_helper_unittest, this change is included too. I will now try to
run some trybot jobs.

https://codereview.chromium.org/12088040/diff/43002/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler.h (right):

https://codereview.chromium.org/12088040/diff/43002/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler.h:225: class
SigninPolicyHandler: public TypeCheckingPolicyHandler {
Thanks, I understand now that this special handling is only necessary if there
is no direct mapping to a pref, right?
And in case there is no direct mapping, I would also have to instantiate the
handler from the configuration_policy_handler_list file.
I changed it to an entry in the kSimplePolicyMap.

https://codereview.chromium.org/12088040/diff/43002/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12088040/diff/43002/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:667:
base::Bind(&BrowserOptionsHandler::SetupProxySettingsSection,
On 2013/02/07 15:05:30, Andrew T Wilson wrote:
> Indent is off here now.

Done.

[Joao da Silva, 2013-02-07 16:13:05]

https://codereview.chromium.org/12088040/diff/43002/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler.h (right):

https://codereview.chromium.org/12088040/diff/43002/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler.h:225: class
SigninPolicyHandler: public TypeCheckingPolicyHandler {
On 2013/02/07 16:08:59, Adrian Kuegel wrote:
> Thanks, I understand now that this special handling is only necessary if there
> is no direct mapping to a pref, right?
> And in case there is no direct mapping, I would also have to instantiate the
> handler from the configuration_policy_handler_list file.
> I changed it to an entry in the kSimplePolicyMap.

Sounds good. Did you forget to upload that patch?

[Adrian Kuegel, 2013-02-07 16:16:28]

There was an error when I uploaded the patch. It seems I should not do a reload
of the page during the upload of the patch, otherwise the upload script crashes.
I deleted the half-completed patch and uploaded again.

[Joao da Silva, 2013-02-07 16:25:53]

policy/ lgtm

I'd ask for a new policy_browsertest, but
new_tab_page_sync_handler_browsertest.cc already covers that.

[Adrian Kuegel, 2013-02-08 10:23:37]

@Scott: can you please look at chrome/browser/app_controller_mac.mm and
chrome/chrome_tests.gypi and the files in chrome/test/base, chrome/common,
chrome/browser/ui and chrome/browser/ui/toolbar ?

@Dan: can you please take a look at the files in  chrome/browser/ui/webui/ntp
and chrome/browser/ui/webui/sync_promo ?

@Sailesh: can you please take a look at the files in chrome/browser/profiles and
chrome/browser/ui/cocoa ?

@Chris: can you please take a look at the files in
chrome/browser/ui/webui/options ?

[Adrian Kuegel, 2013-02-08 10:24:19]

@Scott: can you please look at chrome/browser/app_controller_mac.mm and
chrome/chrome_tests.gypi and the files in chrome/test/base, chrome/common,
chrome/browser/ui and chrome/browser/ui/toolbar ?

@Dan: can you please take a look at the files in  chrome/browser/ui/webui/ntp
and chrome/browser/ui/webui/sync_promo ?

@Sailesh: can you please take a look at the files in chrome/browser/profiles and
chrome/browser/ui/cocoa ?

@Chris: can you please take a look at the files in
chrome/browser/ui/webui/options ?

[csilv, 2013-02-08 18:31:10]

chrome/browser/ui/webui/options lgtm

[sail, 2013-02-08 20:18:57]

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/g...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/g...
chrome/browser/profiles/gaia_info_update_service.cc:68:
!profile->GetOriginalProfile()->IsSigninAllowed())
Why this change?

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/p...
chrome/browser/profiles/profile.h:356: bool IsSigninAllowed();
Could we avoid adding this here? chrome/browser/signin is probably a better
place.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:447: mutable BooleanPrefMember
signin_allowed_;
Why do you need to store this?

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/signin/sig...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:138: !signin->IsSigninAllowed()) {
This code doesn't make any sense. If sign in is now allowed then why would you
want to trigger a signin failure?

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:150: ProfileSyncService* service =
Why this change?

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.cc:759:
command_updater_.UpdateCommandEnabled(IDC_SHOW_SYNC_SETUP,
Move the code to update this state into a single function.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/chrome_...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/chrome_...
chrome/browser/ui/chrome_pages.cc:189: if (!manager->IsSigninAllowed())
Why would this code be reached if signin is not allowed.
If signin in was not allowed and the user did something to trigger this code
then what would the user see now?

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:1093: enable &=
original_profile->IsSigninAllowed();
Don't do this, if chrome::IsCommandEnabled() is not returning the correct value
then fix that function.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.h (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.h:49: std::string msg,
std::string linktext);
indentation is incorrect

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc
(right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:23: class
NewTabPageSyncHandlerBrowserTest : public InProcessBrowserTest {
If this class doesn't do anything then just do:
  typedef InProcessBrowserTest  NewTabPageSyncHandlerBrowserTest

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:28: class
NewTabPageSyncHandlerTest : public NewTabPageSyncHandler {
I think names ending in Test are mostly reserved for tests. Rename to something
like MockNewTabPageSyncHandler.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:41:
ChangeSigninAllowedToFalse) {
move beneath the "N" for NewTab

[Andrew T Wilson (Slow), 2013-02-08 20:33:11]

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/signin/sig...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:138: !signin->IsSigninAllowed()) {
On 2013/02/08 20:18:57, sail wrote:
> This code doesn't make any sense. If sign in is now allowed then why would you
> want to trigger a signin failure?
Because signin isn't allowed, so signin should fail?

In any case, we could probably skip this check since SigninManager will
SignOut() if SigninAllowed transitions to true during the signin process.

[sail, 2013-02-08 20:34:59]

> Because signin isn't allowed, so signin should fail?

This kind of logic is super scary (and a lot of this CL too).

[Andrew T Wilson (Slow), 2013-02-08 20:51:47]

On 2013/02/08 20:34:59, sail wrote:
> > Because signin isn't allowed, so signin should fail?
> 
> This kind of logic is super scary (and a lot of this CL too).

Imagine the case where a user is signing in, policy is downloaded, which then
disallows signin. Why would we not want to check this in the signin tracker? 

I mean, I can understand you saying that it's redundant based on logic in
SigninManager, and I think it's reasonable to remove it, but I'm not sure it's
"super scary" :)

[Andrew T Wilson (Slow), 2013-02-08 20:53:40]

That said, I think it's reasonable to change some of the purely-defensive checks
for IsSIgninAllowed() (like in chrome_pages.cc) with DCHECKs.

[sail, 2013-02-08 21:00:47]

On 2013/02/08 20:53:40, Andrew T Wilson wrote:
> That said, I think it's reasonable to change some of the purely-defensive
checks
> for IsSIgninAllowed() (like in chrome_pages.cc) with DCHECKs.

What you're describing seems very hard to implement. It means that in the middle
of any operation something could have happened that now disallows that
operation.

[Andrew T Wilson (Slow), 2013-02-08 21:07:22]

On 2013/02/08 21:00:47, sail wrote:
> On 2013/02/08 20:53:40, Andrew T Wilson wrote:
> > That said, I think it's reasonable to change some of the purely-defensive
> checks
> > for IsSIgninAllowed() (like in chrome_pages.cc) with DCHECKs.
> 
> What you're describing seems very hard to implement. It means that in the
middle
> of any operation something could have happened that now disallows that
> operation.
I'm going to take a step back and let Adrian defend his own CL :) But I'll just
point out that policies can (and will!) change dynamically (especially during
signin, because that's when we download cloud policy).

Any time your code path lets the MessageLoop run (which it does during signin
because it makes multiple async network requests) then you need to deal with
policies potentially changing in mid-operation. But, of course, if you check
IsSIgninAllowed(), then the value is guaranteed not to change for any code you
directly call on that thread before going back to the MessageLoop.

[Adrian Kuegel, 2013-02-08 23:17:55]

@sail: Thanks for the review. Sorry that I did not respond earlier, it is
already quite late here and I will go to bed soon. I will address your comments
on Monday.

[Adrian Kuegel, 2013-02-11 16:47:30]

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/g...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/g...
chrome/browser/profiles/gaia_info_update_service.cc:68:
!profile->GetOriginalProfile()->IsSigninAllowed())
I thought that "No Signin" should imply "No Sync" as well. But it seems to be
already covered since with a policy change that disallows signin we also sign
out automatically, so I removed the change.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/p...
chrome/browser/profiles/profile.h:356: bool IsSigninAllowed();
Yes, it can be avoided, it was just making the code at other places a bit
easier, because I didn't have to get the SigninManager first. But I removed it
here now, and changed the code at other places accordingly.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:447: mutable BooleanPrefMember
signin_allowed_;
This is needed in the one_click_signin_helper.cc file when calling the
IsSigninAllowedOnIOThread function. I just implemented it the same way as for
example with the sync_disabled pref.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/signin/sig...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:138: !signin->IsSigninAllowed()) {
IsSigninAllowed returns true if signin is allowed. So here I check if signin is
not allowed, and if it is not allowed, a signin failure is triggered. This
should cover the case that the policy currently doesn't allow signin, but
somehow a user still was able to sign in. I am not sure if we could ever end up
in this path, so I removed it now.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/signin/sig...
chrome/browser/signin/signin_tracker.cc:150: ProfileSyncService* service =
Sorry, that was a relict from an earlier version of this changelist, and I
forgot to change it back.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.cc:759:
command_updater_.UpdateCommandEnabled(IDC_SHOW_SYNC_SETUP,
Done. Note that I had to pass show_main_ui as a parameter to the function, as
this value is determined differently at the three locations where the
IDC_SHOW_SYNC_SETUP command is updated.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/chrome_...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/chrome_...
chrome/browser/ui/chrome_pages.cc:189: if (!manager->IsSigninAllowed())
Yes, I agree this code should never be reached if everything else works properly
(updating the corresponding command in BrowserCommandController for example).
If for example the command in the menu is not updated properly, and the user
clicks on it, nothing would happen.
As suggested by Andrew, I changed the code here into a DCHECK. But I can also
remove it if you want.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:1093: enable &=
original_profile->IsSigninAllowed();
IsCommandEnabled should return the correct value, I am testing that with two
tests I added to the BrowserCommandController. So I will remove the code here.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.h (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.h:49: std::string msg,
std::string linktext);
Yes, sorry about that, I forgot to fix it when I added the virtual to the
function.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc
(right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:23: class
NewTabPageSyncHandlerBrowserTest : public InProcessBrowserTest {
On 2013/02/08 20:18:57, sail wrote:
> If this class doesn't do anything then just do:
>   typedef InProcessBrowserTest  NewTabPageSyncHandlerBrowserTest

Done.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:28: class
NewTabPageSyncHandlerTest : public NewTabPageSyncHandler {
On 2013/02/08 20:18:57, sail wrote:
> I think names ending in Test are mostly reserved for tests. Rename to
something
> like MockNewTabPageSyncHandler.

Done.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:41:
ChangeSigninAllowedToFalse) {
On 2013/02/08 20:18:57, sail wrote:
> move beneath the "N" for NewTab

Done.

[Adrian Kuegel, 2013-02-15 15:53:52]

Could I please get another review, so that I know what I still need to change?

[Adrian Kuegel, 2013-02-20 22:16:17]

Another ping.

[sky, 2013-02-20 22:52:43]

When you have multiple reviewers and you ping please specify who you
are waiting on. Otherwise we are all likely to ignore thinking you're
waiting on someone else.

On Wed, Feb 20, 2013 at 2:16 PM,  <akuegel@chromium.org> wrote:
> Another ping.
>
> https://codereview.chromium.org/12088040/

[sail, 2013-02-20 23:08:21]

Not sure how I missed the previous ping. Sorry about that.
This CL looks good.
Could you get one of the ProflieIOData folks to review the profile_io_data.*
changes? (willchan or mmenke or ajwong).

Otherwise LGTM

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:447: mutable BooleanPrefMember
signin_allowed_;
On 2013/02/11 16:47:30, Adrian Kuegel wrote:
> This is needed in the one_click_signin_helper.cc file when calling the
> IsSigninAllowedOnIOThread function. I just implemented it the same way as for
> example with the sync_disabled pref.

Ok, makes sense.

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/chrome_...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/12088040/diff/38008/chrome/browser/ui/chrome_...
chrome/browser/ui/chrome_pages.cc:189: if (!manager->IsSigninAllowed())
On 2013/02/11 16:47:30, Adrian Kuegel wrote:
> Yes, I agree this code should never be reached if everything else works
properly
> (updating the corresponding command in BrowserCommandController for example).
> If for example the command in the menu is not updated properly, and the user
> clicks on it, nothing would happen.
> As suggested by Andrew, I changed the code here into a DCHECK. But I can also
> remove it if you want.

DCHECK is fine.

https://codereview.chromium.org/12088040/diff/53005/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc
(right):

https://codereview.chromium.org/12088040/diff/53005/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:28:
std::string linktext));
new line after this

https://codereview.chromium.org/12088040/diff/53005/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:31: }
new line after this

https://codereview.chromium.org/12088040/diff/53005/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:32:
MessageType GetHideType() {
Not sure what this is. Can you remove this and hard code the value in the test?

[Adrian Kuegel, 2013-02-21 15:48:59]

@Sail: thanks for the review, and I will ask willchan to review the profileio
files.

https://codereview.chromium.org/12088040/diff/53005/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc
(right):

https://codereview.chromium.org/12088040/diff/53005/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:28:
std::string linktext));
On 2013/02/20 23:08:21, sail wrote:
> new line after this

Done.

https://codereview.chromium.org/12088040/diff/53005/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:31: }
On 2013/02/20 23:08:21, sail wrote:
> new line after this

Done.

https://codereview.chromium.org/12088040/diff/53005/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler_browsertest.cc:32:
MessageType GetHideType() {
I just added the test class as friend, so I could get rid of this method.

[Adrian Kuegel, 2013-02-21 15:54:47]

@willchan: could you please review the profile_io files?
@Scott: can you please review chrome/browser/app_controller_mac.mm and
chrome/chrome_tests.gypi and the files in chrome/test/base, chrome/common,
chrome/browser/ui and chrome/browser/ui/toolbar ?

@Dan: can you please take a look at the files in  chrome/browser/ui/webui/ntp
and chrome/browser/ui/webui/sync_promo ?

[willchan no longer on Chromium, 2013-02-21 16:02:47]

lgtm

[sky, 2013-02-21 20:18:44]

LGTM

[Adrian Kuegel, 2013-02-25 16:28:07]

@Dan: can you please take a look at the files in  chrome/browser/ui/webui/ntp
and chrome/browser/ui/webui/sync_promo ?
Seems like I forgot to include you in some of the previous emails, sorry about
that.

[Dan Beam, 2013-02-25 17:35:07]

lgtm but please address nits

https://codereview.chromium.org/12088040/diff/64001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc (right):

https://codereview.chromium.org/12088040/diff/64001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc:49:
profile_pref_registrar_.RemoveAll();
probably isn't necessary.
https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/public/...

https://codereview.chromium.org/12088040/diff/64001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12088040/diff/64001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:154:
profile_pref_registrar_.RemoveAll();
also may not be necessary.

[commit-bot: I haz the power, 2013-02-28 16:11:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/12088040/71001

[commit-bot: I haz the power, 2013-02-28 17:36:01]

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

[commit-bot: I haz the power, 2013-02-28 21:12:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/12088040/71001

[commit-bot: I haz the power, 2013-02-28 23:14:21]

Change committed as 185359