Implement support for disabling sync through configuration management.

Implement support for disabling sync through configuration management.

BUG=45316
TEST=Configure SyncDisabled policy and check the UI.

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

[Mattias Nissler (ping if slow), 2010-07-08 15:32:46]

Hi guys, can you review this? I thought danno for the policy related stuff,
fbarchard representing the sync people and pkasting looking at the various UI
bits and pieces. Would that be OK? Do I need to bring in more/different people?

[Peter Kasting, 2010-07-08 20:38:39]

LGTM

http://codereview.chromium.org/2905003/diff/1/11
File chrome/browser/dom_ui/new_tab_page_sync_handler.cc (right):

http://codereview.chromium.org/2905003/diff/1/11#newcode1
chrome/browser/dom_ui/new_tab_page_sync_handler.cc:1: // Copyright (c) 2006-2008
The Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2905003/diff/1/12
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/2905003/diff/1/12#newcode1
chrome/browser/dom_ui/new_tab_ui.cc:1: // Copyright (c) 2006-2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2905003/diff/1/12#newcode468
chrome/browser/dom_ui/new_tab_ui.cc:468: if (GetProfile()->IsSyncAccessible()) {
Nit: Remove {} while here

http://codereview.chromium.org/2905003/diff/1/21
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/2905003/diff/1/21#newcode1
chrome/browser/sync/sync_ui_util.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

[ncarter (slow), 2010-07-08 21:01:44]

-fbarchard, +skrul to review the sync parts.

[skrul, 2010-07-08 21:36:32]

Sync parts LGTM, but can you add a test to
profile_sync_service_startup_unittest.cc that tests the patch where disabling
management will start the sync service?

[danno, 2010-07-08 22:07:24]

http://codereview.chromium.org/2905003/diff/1/7
File chrome/browser/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/2905003/diff/1/7#newcode29
chrome/browser/configuration_policy_pref_store.cc:29: { Value::TYPE_BOOLEAN,
kPolicySyncDisabled, prefs::kSyncManaged }
It looks like there is a duplication here. You handle kPolicySyncDisabled in
ApplySyncPolicy, but it gets handled in ApplyPolicyFromMap, too.

http://codereview.chromium.org/2905003/diff/1/10
File chrome/browser/configuration_policy_store.h (right):

http://codereview.chromium.org/2905003/diff/1/10#newcode46
chrome/browser/configuration_policy_store.h:46: #endif  //
CHROME_BROWSER_CONFIGURATION_POLICY_STORE_H_
minor nit: re-add the trailing newline.

[danno, 2010-07-08 22:08:47]

LGTM if you fix the two items in my previous comment.

[Mattias Nissler (ping if slow), 2010-07-12 15:10:56]

Thanks for your comments! I didn't get around to writing the unit test today,
but I'll do it tomorrow.

http://codereview.chromium.org/2905003/diff/1/7
File chrome/browser/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/2905003/diff/1/7#newcode29
chrome/browser/configuration_policy_pref_store.cc:29: { Value::TYPE_BOOLEAN,
kPolicySyncDisabled, prefs::kSyncManaged }
On 2010/07/08 22:07:25, danno wrote:
> It looks like there is a duplication here. You handle kPolicySyncDisabled in
> ApplySyncPolicy, but it gets handled in ApplyPolicyFromMap, too.

Removed. Thanks for pointing out!
Done.

http://codereview.chromium.org/2905003/diff/1/10
File chrome/browser/configuration_policy_store.h (right):

http://codereview.chromium.org/2905003/diff/1/10#newcode46
chrome/browser/configuration_policy_store.h:46: #endif  //
CHROME_BROWSER_CONFIGURATION_POLICY_STORE_H_
On 2010/07/08 22:07:25, danno wrote:
> minor nit: re-add the trailing newline.

Why? The coding style says to avoid vertical whitespace
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Vertical_White...

And btw. I didn't even do it myself, but the Google vi configuration that strips
trailing newlines automatically on saving :-)

http://codereview.chromium.org/2905003/diff/1/11
File chrome/browser/dom_ui/new_tab_page_sync_handler.cc (right):

http://codereview.chromium.org/2905003/diff/1/11#newcode1
chrome/browser/dom_ui/new_tab_page_sync_handler.cc:1: // Copyright (c) 2006-2008
The Chromium Authors. All rights reserved.
On 2010/07/08 20:38:40, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2905003/diff/1/12
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/2905003/diff/1/12#newcode1
chrome/browser/dom_ui/new_tab_ui.cc:1: // Copyright (c) 2006-2009 The Chromium
Authors. All rights reserved.
On 2010/07/08 20:38:40, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2905003/diff/1/12#newcode468
chrome/browser/dom_ui/new_tab_ui.cc:468: if (GetProfile()->IsSyncAccessible()) {
On 2010/07/08 20:38:40, Peter Kasting wrote:
> Nit: Remove {} while here

Done.

http://codereview.chromium.org/2905003/diff/1/21
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/2905003/diff/1/21#newcode1
chrome/browser/sync/sync_ui_util.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2010/07/08 20:38:40, Peter Kasting wrote:
> Nit: 2010

Done.

[Peter Kasting, 2010-07-12 17:41:16]

http://codereview.chromium.org/2905003/diff/1/10
File chrome/browser/configuration_policy_store.h (right):

http://codereview.chromium.org/2905003/diff/1/10#newcode46
chrome/browser/configuration_policy_store.h:46: #endif  //
CHROME_BROWSER_CONFIGURATION_POLICY_STORE_H_
Files should end with a newline.  However, they don't need to end with two
newlines.  If we see a blank line at an end of a file, that means there are at
least two newlines, and one can be deleted.  Therefore, this change should
stand, not be reverted.

[Mattias Nissler (ping if slow), 2010-07-12 19:39:59]

skrul: Now with unit test for sync startup.

[skrul, 2010-07-12 20:28:09]

Thank you :) LGTM