Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(231)

Issue 14655009: Client changes for disabled dasher account (Closed)

Created:
7 years, 7 months ago by pavely
Modified:
7 years, 6 months ago
CC:
chromium-reviews, dbeam+watch-options_chromium.org, akalin, Raghu Simha, sail+watch_chromium.org, arv+watch_chromium.org, haitaol1, tim (not reviewing)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Client changes for disabled dasher account When sync receives DISABLED_BY_ADMIN from server PSS stops SyncBackendHost and disables sync by settings DisabledByAdmin pref. This prevents sync from connecting to server in the future. The way to reenable sync is to sign out and sign in again. In future milestone we will implement retry logic that will periodically (once a day) try to connect to sync server to see if sync is still disabled. BUG=178417 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202956

Patch Set 1 #

Total comments: 22

Patch Set 2 : Fix small things based on feedback #

Total comments: 2

Patch Set 3 : Variant with in-memory flag and no pref setting #

Patch Set 4 : Cleanup #

Total comments: 11

Patch Set 5 : Make PSS reset "disabled" flag after sign out. #

Patch Set 6 : Apply feedback #

Patch Set 7 : Add comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+63 lines, -25 lines) Patch
M chrome/browser/profiles/profile.cc View 1 2 3 4 5 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.h View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.cc View 1 2 3 4 5 6 5 chunks +11 lines, -3 lines 0 comments Download
M chrome/browser/sync/sync_prefs.h View 1 2 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/sync/sync_prefs.cc View 1 2 3 4 5 6 1 chunk +0 lines, -7 lines 0 comments Download
M chrome/browser/ui/sync/one_click_signin_sync_starter.h View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/ui/sync/one_click_signin_sync_starter.cc View 1 2 3 4 5 5 chunks +11 lines, -7 lines 0 comments Download
M chrome/browser/ui/webui/sync_setup_handler.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M sync/engine/sync_scheduler_impl.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M sync/engine/syncer_proto_util.cc View 1 2 3 chunks +7 lines, -1 line 0 comments Download
M sync/internal_api/public/util/syncer_error.h View 1 chunk +1 line, -0 lines 0 comments Download
M sync/internal_api/public/util/syncer_error.cc View 1 chunk +1 line, -0 lines 0 comments Download
M sync/protocol/proto_enum_conversions.cc View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M sync/protocol/sync_enums.proto View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M sync/protocol/sync_protocol_error.h View 1 2 chunks +7 lines, -0 lines 0 comments Download
M sync/protocol/sync_protocol_error.cc View 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
pavely
7 years, 7 months ago (2013-05-06 23:43:55 UTC) #1
tim (not reviewing)
https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc#newcode1801 chrome/common/pref_names.cc:1801: const char kSyncDisabledByAdmin[] = "sync.disabled_by_admin"; Can eventually replace sync.managed ...
7 years, 7 months ago (2013-05-13 04:18:57 UTC) #2
Andrew T Wilson (Slow)
My main concern is that I don't think we want to keep the disabled status ...
7 years, 7 months ago (2013-05-13 11:51:13 UTC) #3
pavely
https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/profile_sync_service.cc File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/14655009/diff/1/chrome/browser/sync/profile_sync_service.cc#newcode678 chrome/browser/sync/profile_sync_service.cc:678: start_up_time_ = base::Time(); On 2013/05/13 11:51:13, Andrew T Wilson ...
7 years, 7 months ago (2013-05-13 22:37:08 UTC) #4
Andrew T Wilson (Slow)
https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc#newcode1801 chrome/common/pref_names.cc:1801: const char kSyncDisabledByAdmin[] = "sync.disabled_by_admin"; On 2013/05/13 22:37:09, pavely ...
7 years, 7 months ago (2013-05-14 12:34:00 UTC) #5
tim (not reviewing)
https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/14655009/diff/1/chrome/common/pref_names.cc#newcode1801 chrome/common/pref_names.cc:1801: const char kSyncDisabledByAdmin[] = "sync.disabled_by_admin"; On 2013/05/14 12:34:00, Andrew ...
7 years, 7 months ago (2013-05-16 18:32:48 UTC) #6
pavely
> I don't think I can approve this change with the disabled status stored in ...
7 years, 7 months ago (2013-05-21 01:21:57 UTC) #7
Andrew T Wilson (Slow)
On 2013/05/16 18:32:48, timsteele wrote: > > Are you proposing we leave all the sync ...
7 years, 7 months ago (2013-05-21 09:40:50 UTC) #8
pavely
Andrew, could you take a look? This is implementation with "sync disabled" flag kept in ...
7 years, 7 months ago (2013-05-23 01:12:26 UTC) #9
Andrew T Wilson (Slow)
LGTM if we can make the change I suggest to IsSyncAccessible() https://codereview.chromium.org/14655009/diff/26001/chrome/browser/signin/signin_tracker.cc File chrome/browser/signin/signin_tracker.cc (right): ...
7 years, 7 months ago (2013-05-24 15:13:10 UTC) #10
pavely
Andrew, your advice didn't work out as easily. Could you take a look at my ...
7 years, 7 months ago (2013-05-24 22:17:10 UTC) #11
Andrew T Wilson (Slow)
Still LGTM. On second thought, I'm OK with not moving IsManaged() into IsSyncAccessible(). Can you ...
7 years, 7 months ago (2013-05-27 15:03:33 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/14655009/43002
7 years, 6 months ago (2013-05-29 00:40:10 UTC) #13
pavely
I've updated CL. Thanks.
7 years, 6 months ago (2013-05-29 00:41:13 UTC) #14
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=5465
7 years, 6 months ago (2013-05-29 01:00:39 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/14655009/43002
7 years, 6 months ago (2013-05-29 07:00:29 UTC) #16
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=5548
7 years, 6 months ago (2013-05-29 07:09:21 UTC) #17
pavely
+rlp & erg for profile.cc Rachel, Elliot, could one of you take a look at ...
7 years, 6 months ago (2013-05-29 17:13:09 UTC) #18
Elliot Glaysher
lgtm
7 years, 6 months ago (2013-05-29 18:05:31 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/14655009/43002
7 years, 6 months ago (2013-05-29 18:14:01 UTC) #20
commit-bot: I haz the power
7 years, 6 months ago (2013-05-29 20:45:00 UTC) #21
Message was sent while issue was closed.
Change committed as 202956

Powered by Google App Engine
This is Rietveld 408576698