Allow signin to continue even if sync is disabled by policy.

Allow signin to continue even if sync is disabled by policy.

Change signin logic to allow signin if SyncDisabled policy is set.

Also broke out logic in SyncUIUtil into separate class so it can be used even
if sync is not enabled.

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

[Andrew T Wilson (Slow), 2013-01-29 17:43:15]

https://codereview.chromium.org/12077030/diff/13004/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_manager_unittest.cc (left):

https://codereview.chromium.org/12077030/diff/13004/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_manager_unittest.cc:3083:
profile()->set_incognito(true);
It's not OK to initialize a profile then set it to be incognito, because now any
services that rely on SigninManager can't shutdown since SigninManager doesn't
exist on incognito profiles. Moved this code to a separate test on a profile
that starts off as incognito.

[Andrew T Wilson (Slow), 2013-01-29 17:45:10]

Roger, PTAL at the browser/signin and browser/sync stuff.
James, PTAL at browser/ui/webui

[Andrew T Wilson (Slow), 2013-01-29 17:46:24]

On 2013/01/29 17:45:10, Andrew T Wilson wrote:
Also, James, take a peek at the test change in browser/autofill if you don't
mind.

[sail, 2013-01-29 19:17:16]

Please add me as a reviewer to all sync promo changes.

https://codereview.chromium.org/12077030/diff/5007/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12077030/diff/5007/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:131: profile =
profile->GetOriginalProfile();
Changing passed in parameters is really confusing. Use a new variable instead.
Also, update the code below that calls GetOriginalProfile() to use the new temp
variable.

https://codereview.chromium.org/12077030/diff/5007/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:133: // Sync is disabled by
policy - display the promo if the user is not signed
I don't understand this. This implies that if sync is accessible we may show the
promo if the user is signed in. Why would we do that?

I guess what I'm trying to say is, how is this different from the
HasSyncSetupCompleted() check below?

[Andrew T Wilson (Slow), 2013-01-30 07:43:41]

https://codereview.chromium.org/12077030/diff/5007/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12077030/diff/5007/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:133: // Sync is disabled by
policy - display the promo if the user is not signed
On 2013/01/29 19:17:16, sail wrote:
> I don't understand this. This implies that if sync is accessible we may show
the
> promo if the user is signed in. Why would we do that?
> 
> I guess what I'm trying to say is, how is this different from the
> HasSyncSetupCompleted() check below?
If sync is accessible (meaning not disabled by policy/command line flags) we
just fall through to the old logic below (no logic change).

If sync is not accessible, then we return true if the user is not yet signed in.

So previously, we would not show the promo if sync was disabled by policy. But
now, in that case, we still show the promo if the user isn't signed in yet
because we want to prompt the user to signin (the promo becomes more of a
"signin promo" and not a "sync promo").

Let me know if you think this is inappropriate somehow.

[Andrew T Wilson (Slow), 2013-01-30 13:55:49]

Roger, please also look at browser/ui/sync

jhawkins: also look at browser/resources

Sky, please look at files in browser/ui top-level dir

Sail, thanks for looking at this and I'll add you as a reviewer for future promo
changes. Can you also look at browser/ui/cocoa since you are an OWNER there?

jcivelli: Can you look at my change to chrome/tests?

dbeam: please review changes to webui/ntp

isherman: PTAL at my change to the browser/autofill test.

Whew.

https://codereview.chromium.org/12077030/diff/5007/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12077030/diff/5007/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:131: profile =
profile->GetOriginalProfile();
On 2013/01/29 19:17:16, sail wrote:
> Changing passed in parameters is really confusing. Use a new variable instead.
> Also, update the code below that calls GetOriginalProfile() to use the new
temp
> variable.

Done.

https://codereview.chromium.org/12077030/diff/5007/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:133: // Sync is disabled by
policy - display the promo if the user is not signed
On 2013/01/30 07:43:41, Andrew T Wilson wrote:
> On 2013/01/29 19:17:16, sail wrote:
> > I don't understand this. This implies that if sync is accessible we may show
> the
> > promo if the user is signed in. Why would we do that?
> > 
> > I guess what I'm trying to say is, how is this different from the
> > HasSyncSetupCompleted() check below?

Actually, after re-reading your comment, I've changed this to show the promo if
the user is not logged in, regardless of whether sync is enabled or disabled by
policy. The logic is much simpler.

As before, let me know if you think this is inappropriate.

[sky, 2013-01-30 15:02:13]

I only looked at browser/ui, none of the children. That LGTM.

[Roger Tawa OOO till Jul 10th, 2013-01-30 16:46:45]

lgtm

https://codereview.chromium.org/12077030/diff/6008/chrome/browser/signin/fake...
File chrome/browser/signin/fake_auth_status_provider.cc (right):

https://codereview.chromium.org/12077030/diff/6008/chrome/browser/signin/fake...
chrome/browser/signin/fake_auth_status_provider.cc:21: void
FakeAuthStatusProvider::set_auth_error(
SetAuthError naming?

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

https://codereview.chromium.org/12077030/diff/6008/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/wrench_menu_model.cc:537: if
(browser_defaults::kShowSigninMenuItem) {
maybe better to now replace with an #ifdef for chromeos?

[James Hawkins, 2013-01-30 16:55:09]

webui LGTM with nit.

https://codereview.chromium.org/12077030/diff/6008/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/12077030/diff/6008/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:551: ASSERT_LE(1U,
web_ui_.call_data().size());
This really should be an EXPECT, since the test can continue if this line fails.
 ASSERTS should be used to check that a size is >= a certain number so array
access doesn't crash the test.

[sail, 2013-01-30 17:51:01]

cocoa/* profiles/* and sync_promo/* LGTM

https://codereview.chromium.org/12077030/diff/5007/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12077030/diff/5007/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:133: // Sync is disabled by
policy - display the promo if the user is not signed
On 2013/01/30 13:55:50, Andrew T Wilson wrote:
> On 2013/01/30 07:43:41, Andrew T Wilson wrote:
> > On 2013/01/29 19:17:16, sail wrote:
> > > I don't understand this. This implies that if sync is accessible we may
show
> > the
> > > promo if the user is signed in. Why would we do that?
> > > 
> > > I guess what I'm trying to say is, how is this different from the
> > > HasSyncSetupCompleted() check below?
> 
> Actually, after re-reading your comment, I've changed this to show the promo
if
> the user is not logged in, regardless of whether sync is enabled or disabled
by
> policy. The logic is much simpler.
> 
> As before, let me know if you think this is inappropriate.

Awesome!

[Jay Civelli, 2013-01-30 18:20:30]

chrome/tests LGTM

[Ilya Sherman, 2013-01-30 21:24:33]

lgtm

[Andrew T Wilson (Slow), 2013-01-31 10:38:05]

Thanks everyone for the speedy reviews!

https://codereview.chromium.org/12077030/diff/6008/chrome/browser/signin/fake...
File chrome/browser/signin/fake_auth_status_provider.cc (right):

https://codereview.chromium.org/12077030/diff/6008/chrome/browser/signin/fake...
chrome/browser/signin/fake_auth_status_provider.cc:21: void
FakeAuthStatusProvider::set_auth_error(
On 2013/01/30 16:46:46, Roger Tawa wrote:
> SetAuthError naming?

Done.

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

https://codereview.chromium.org/12077030/diff/6008/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/wrench_menu_model.cc:537: if
(browser_defaults::kShowSigninMenuItem) {
On 2013/01/30 16:46:46, Roger Tawa wrote:
> maybe better to now replace with an #ifdef for chromeos?

Done.

https://codereview.chromium.org/12077030/diff/6008/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/12077030/diff/6008/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:551: ASSERT_LE(1U,
web_ui_.call_data().size());
On 2013/01/30 16:55:09, James Hawkins wrote:
> This really should be an EXPECT, since the test can continue if this line
fails.
>  ASSERTS should be used to check that a size is >= a certain number so array
> access doesn't crash the test.

Done.

[commit-bot: I haz the power, 2013-01-31 14:39:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/12077030/9006

[Dan Beam, 2013-01-31 19:18:53]

c/b/ui/webui/ntp lgtm

[commit-bot: I haz the power, 2013-01-31 20:22:04]

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

[commit-bot: I haz the power, 2013-02-01 16:14:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/12077030/26001

[commit-bot: I haz the power, 2013-02-01 17:46:37]

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=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-01 23:32:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/12077030/26001

[commit-bot: I haz the power, 2013-02-02 03:17:32]

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

[commit-bot: I haz the power, 2013-02-02 08:39:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/12077030/26001

[commit-bot: I haz the power, 2013-02-02 17:53:37]

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