Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206883002:
Do not sync ARC opt-in preference (Closed)
|
Created:
4 years, 4 months ago by dspaid Modified:
4 years, 4 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not sync the ARC++ opt-in preference.
This is a re-implementation of 1c7841e8e0322b8c3599710e017ff0fb38ab056f,
which made ARC opt-in preference not synced. That change also stopped
sending UMA stats due to not calling the OnSyncedPrefChanged method.
BUG=633017
TEST=browser_tests --gtest_filter=ArcAuthServiceTest.*
Additionally manually tested that the normal opt-in/opt-out process
worked as expected.
Committed: https://crrev.com/176f6733ab5bdf6de91f08e840b814cf76b910bb
Cr-Commit-Position: refs/heads/master@{#410994}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Do not sync the ARC++ opt-in preference. #Patch Set 3 : Consider all opt-in changes to be synced if ARC is a managed preference #
Total comments: 4
Patch Set 4 : Add TODOs #Messages
Total messages: 18 (5 generated)
dspaid@chromium.org changed reviewers: + hidehiko@chromium.org, khmel@chromium.org
On 2016/08/03 07:23:13, dspaid wrote: Based on discussion on b/ we have a plan to restore synchronized pref. From this point of view this code is not 'dead'. Making this pref local is mostly hack in order to fix not-related issue with removing data on local device. I would recommend to implement it properly by adding local pref (I recommend to add arc.opted_out. In this way you keep arc behavior close to legacy). I also don't see code that handle arc notification shown (you just cut it without proper handling). Even if you force to use this feature local please leave existing code, make call to it temporarily, add todo and bug number that would implement it properly.
https://codereview.chromium.org/2206883002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2206883002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:476: UpdateOptInActionUMA(arc_enabled ? OptInActionType::OPTED_IN Now this statistics is incorrect for managed pref. This UMA for user's action. When admin manages this feature we don't update it. That means arc.enabled can be synced even if it is declared local. We have to check all cases, including when managed state is changed during Arc start.
Description was changed from ========== Correctly report UMA statistics for ARC. This is a followup/fix for 1c7841e8e0322b8c3599710e017ff0fb38ab056f, which made ARC opt-in preference not synced. This also gets rid of some associated dead code. BUG=633017 TEST=browser_tests --gtest_filter=ArcAuthServiceTest.* Additionally manually tested that the normal opt-in/opt-out process worked as expected. ========== to ========== Do not sync the ARC++ opt-in preference. This is a re-implementation of 1c7841e8e0322b8c3599710e017ff0fb38ab056f, which made ARC opt-in preference not synced. That change also stopped sending UMA stats due to not calling the OnSyncedPrefChanged method. BUG=633017 TEST=browser_tests --gtest_filter=ArcAuthServiceTest.* Additionally manually tested that the normal opt-in/opt-out process worked as expected. ==========
On 2016/08/03 14:28:32, khmel wrote: > https://codereview.chromium.org/2206883002/diff/1/chrome/browser/chromeos/arc... > File chrome/browser/chromeos/arc/arc_auth_service.cc (right): > > https://codereview.chromium.org/2206883002/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/arc_auth_service.cc:476: > UpdateOptInActionUMA(arc_enabled ? OptInActionType::OPTED_IN > Now this statistics is incorrect for managed pref. This UMA for user's action. > When admin manages this feature we don't update it. That means arc.enabled can > be synced even if it is declared local. We have to check all cases, including > when managed state is changed during Arc start. In the case of a managed non-synced preference, how do you distinguish between an admin-initiated change of a preference or a user local one?
On 2016/08/08 00:07:58, dspaid wrote: > On 2016/08/03 14:28:32, khmel wrote: > > > https://codereview.chromium.org/2206883002/diff/1/chrome/browser/chromeos/arc... > > File chrome/browser/chromeos/arc/arc_auth_service.cc (right): > > > > > https://codereview.chromium.org/2206883002/diff/1/chrome/browser/chromeos/arc... > > chrome/browser/chromeos/arc/arc_auth_service.cc:476: > > UpdateOptInActionUMA(arc_enabled ? OptInActionType::OPTED_IN > > Now this statistics is incorrect for managed pref. This UMA for user's action. > > When admin manages this feature we don't update it. That means arc.enabled can > > be synced even if it is declared local. We have to check all cases, including > > when managed state is changed during Arc start. > > In the case of a managed non-synced preference, how do you distinguish between > an admin-initiated change of a preference or a user local one? They should come as synced pref.
On 2016/08/08 14:32:19, khmel wrote: > On 2016/08/08 00:07:58, dspaid wrote: > > On 2016/08/03 14:28:32, khmel wrote: > > > > > > https://codereview.chromium.org/2206883002/diff/1/chrome/browser/chromeos/arc... > > > File chrome/browser/chromeos/arc/arc_auth_service.cc (right): > > > > > > > > > https://codereview.chromium.org/2206883002/diff/1/chrome/browser/chromeos/arc... > > > chrome/browser/chromeos/arc/arc_auth_service.cc:476: > > > UpdateOptInActionUMA(arc_enabled ? OptInActionType::OPTED_IN > > > Now this statistics is incorrect for managed pref. This UMA for user's > action. > > > When admin manages this feature we don't update it. That means arc.enabled > can > > > be synced even if it is declared local. We have to check all cases, > including > > > when managed state is changed during Arc start. > > > > In the case of a managed non-synced preference, how do you distinguish between > > an admin-initiated change of a preference or a user local one? > > They should come as synced pref. Ok, comments should be resolved. I'm explicitly not adding a separate parallel synced preference here because it makes the patch much larger and not suitable for cherry picking to M53. There will be a follow up patch for M54 that does add this after more design work.
https://codereview.chromium.org/2206883002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2206883002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:156: registry->RegisterBooleanPref(prefs::kArcEnabled, false); Please add // TODO(): ... https://codereview.chromium.org/2206883002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:520: OnSyncedPrefChanged(prefs::kArcEnabled, IsArcManaged()); Please add // TODO(): ...
https://codereview.chromium.org/2206883002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2206883002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:156: registry->RegisterBooleanPref(prefs::kArcEnabled, false); On 2016/08/09 15:25:48, khmel wrote: > Please add // TODO(): ... Done. https://codereview.chromium.org/2206883002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:520: OnSyncedPrefChanged(prefs::kArcEnabled, IsArcManaged()); On 2016/08/09 15:25:48, khmel wrote: > Please add // TODO(): ... Done.
based on our meeting lgtm
lgtm
The CQ bit was checked by dspaid@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Do not sync the ARC++ opt-in preference. This is a re-implementation of 1c7841e8e0322b8c3599710e017ff0fb38ab056f, which made ARC opt-in preference not synced. That change also stopped sending UMA stats due to not calling the OnSyncedPrefChanged method. BUG=633017 TEST=browser_tests --gtest_filter=ArcAuthServiceTest.* Additionally manually tested that the normal opt-in/opt-out process worked as expected. ========== to ========== Do not sync the ARC++ opt-in preference. This is a re-implementation of 1c7841e8e0322b8c3599710e017ff0fb38ab056f, which made ARC opt-in preference not synced. That change also stopped sending UMA stats due to not calling the OnSyncedPrefChanged method. BUG=633017 TEST=browser_tests --gtest_filter=ArcAuthServiceTest.* Additionally manually tested that the normal opt-in/opt-out process worked as expected. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Do not sync the ARC++ opt-in preference. This is a re-implementation of 1c7841e8e0322b8c3599710e017ff0fb38ab056f, which made ARC opt-in preference not synced. That change also stopped sending UMA stats due to not calling the OnSyncedPrefChanged method. BUG=633017 TEST=browser_tests --gtest_filter=ArcAuthServiceTest.* Additionally manually tested that the normal opt-in/opt-out process worked as expected. ========== to ========== Do not sync the ARC++ opt-in preference. This is a re-implementation of 1c7841e8e0322b8c3599710e017ff0fb38ab056f, which made ARC opt-in preference not synced. That change also stopped sending UMA stats due to not calling the OnSyncedPrefChanged method. BUG=633017 TEST=browser_tests --gtest_filter=ArcAuthServiceTest.* Additionally manually tested that the normal opt-in/opt-out process worked as expected. Committed: https://crrev.com/176f6733ab5bdf6de91f08e840b814cf76b910bb Cr-Commit-Position: refs/heads/master@{#410994} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/176f6733ab5bdf6de91f08e840b814cf76b910bb Cr-Commit-Position: refs/heads/master@{#410994} |
