|
|
Created:
3 years, 10 months ago by emaxx Modified:
3 years, 10 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSkip ARC initial screen when everything is set up by policy
Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.
Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.
If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).
BUG=690121
BUG=b/34285262
TEST=New unit and browser tests;
Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
and ArcLocationServiceEnabled policies to some values, start
a fresh profile, check that ARC eventually gets provisioned
without showing any dialogs.
Review-Url: https://codereview.chromium.org/2682833003
Cr-Original-Commit-Position: refs/heads/master@{#451340}
Committed: https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8031fc13c8dad
Review-Url: https://codereview.chromium.org/2682833003
Cr-Commit-Position: refs/heads/master@{#451437}
Committed: https://chromium.googlesource.com/chromium/src/+/f392d837d227d79a9b8dec1d298e9c562929c7cf
Patch Set 1 #Patch Set 2 : Add missing includes #
Total comments: 10
Patch Set 3 : Make the prefs off after policy is unset #Patch Set 4 : Rebase #Patch Set 5 : Sync during managed->unmanaged transition #
Total comments: 8
Patch Set 6 : Refactor as per review feedback #Patch Set 7 : Rebase #Patch Set 8 : Make tests parametric #
Total comments: 4
Patch Set 9 : Move the OptIn logic into ArcOptInPreferenceHandler #Patch Set 10 : Rebase #Patch Set 11 : Fix bad rebase #Patch Set 12 : Fix typo #Patch Set 13 : Fix typo #Patch Set 14 : Rebase #
Total comments: 2
Patch Set 15 : Rebase #Messages
Total messages: 97 (74 generated)
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Skip ARC Terms of Service when policies are set up BUG= ========== to ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. BUG=690121 TEST=new unit test; manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs ==========
Description was changed from ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. BUG=690121 TEST=new unit test; manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs ========== to ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. BUG=690121 BUG=b/34285262 TEST=new unit test; manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs ==========
Description was changed from ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. BUG=690121 BUG=b/34285262 TEST=new unit test; manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs ========== to ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. BUG=690121 BUG=b/34285262 TEST=new unit test; manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs ==========
Description was changed from ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. BUG=690121 BUG=b/34285262 TEST=new unit test; manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs ========== to ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. BUG=690121 BUG=b/34285262 TEST=new unit test; manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
emaxx@chromium.org changed reviewers: + yusukes@chromium.org
Yusuke, could you PTAL?
yusukes@chromium.org changed reviewers: + hidehiko@chromium.org
+hidehiko who would be the best reviewer for the files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. BUG=690121 BUG=b/34285262 TEST=new unit test; manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs ========== to ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. BUG=690121 BUG=b/34285262 TEST=new unit test; manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs ==========
Description was changed from ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. BUG=690121 BUG=b/34285262 TEST=new unit test; manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs ========== to ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. BUG=690121 BUG=b/34285262 TEST=New unit test; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. ==========
Description was changed from ========== Skip ARC initial screen when policies are set up Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. BUG=690121 BUG=b/34285262 TEST=New unit test; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. ========== to ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. BUG=690121 BUG=b/34285262 TEST=New unit test; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. ==========
Posted a question on the bug. https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:530: if (!g_disable_ui_for_testing && prefs->HasPrefPath(prefs::kArcEnabled)) { Style: Could you elide the brace for one-line if-body case? https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:566: prefs->SetBoolean(prefs::kArcTermsAccepted, true); How about: // Skip to show UI asking users to enable/disable their preference for backup-restore and location-service, if both are managed by the admin policy. // Note that, in managed case, ToS agreement is already taken in ${where it is}, so it is ok to mark arc.terms.accepted. BTW, is there any case that arc.enabled pref is changed from managed to unmanaged at some point while ARC is running? If yes, what's the expected behavior in the case? https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:610: if (support_host_ && Good catch! Thank you for fix. https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:448: TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) { Could you use parameterized test so that what failed is easily identified.
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/09 15:28:04, hidehiko wrote: > Posted a question on the bug. So I implemented a different approach that allows to return to the "disabled" state once the managed values go away and there was no explicit user decision yet. However, this doesn't work fully now, due to the way how the settings synchronization with ARC is implemented (the ArcSettingsServiceImpl class; see also b/35043055). I'm investigating now whether that issue can be fixed separately, so that the implementation from this CL may function correctly... https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:530: if (!g_disable_ui_for_testing && prefs->HasPrefPath(prefs::kArcEnabled)) { On 2017/02/09 15:28:04, hidehiko wrote: > Style: Could you elide the brace for one-line if-body case? Done. https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:566: prefs->SetBoolean(prefs::kArcTermsAccepted, true); On 2017/02/09 15:28:04, hidehiko wrote: > How about: > > // Skip to show UI asking users to enable/disable their preference for > backup-restore and location-service, if both are managed by the admin policy. > // Note that, in managed case, ToS agreement is already taken in ${where it is}, > so it is ok to mark arc.terms.accepted. > > BTW, is there any case that arc.enabled pref is changed from managed to > unmanaged at some point while ARC is running? If yes, what's the expected > behavior in the case? Done, with a slight modification of the second sentence, because the actual ToS text is already hidden in the managed ARC case regardless of this CL. PTAL. https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:610: if (support_host_ && On 2017/02/09 15:28:04, hidehiko wrote: > Good catch! Thank you for fix. Acknowledged. https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:448: TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) { On 2017/02/09 15:28:04, hidehiko wrote: > Could you use parameterized test so that what failed is easily identified. Quick question: do you think this will be better, given that the debug information is currently printed by the test code too? (Note the last EXPECT_EQ in this function - all other EXPECT* statements are actually assertions which should hold regardless of the tested pref values.) I'm just concerned a little bit about bloating the test code.
Please hold on reviewing. After some offline discussion, an idea for how to deal with the corner cases emerged.
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
emaxx@chromium.org changed reviewers: + khmel@chromium.org
I've updated the CL to support the "managed pref"=>"unmanaged pref" transition. PTAL. +khmel: Yury, as per our offline discussion, the CL now contains the pref manipulation that allows to support the "managed pref"=>"unmanaged" transition. As for another idea - assign the prefs to "true" when the ToS screen is shown - then I decided against it, as if the user closes the ToS screen without pressing "Agree", then the prefs would contain wrong values.
Description was changed from ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. BUG=690121 BUG=b/34285262 TEST=New unit test; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. ========== to ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled} policies become unset later, then the corresponding preferences will change to the "false" state (which means that the user would have to enable these features manually if they want to). BUG=690121 BUG=b/34285262 TEST=New unit test; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. ==========
Description was changed from ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled} policies become unset later, then the corresponding preferences will change to the "false" state (which means that the user would have to enable these features manually if they want to). BUG=690121 BUG=b/34285262 TEST=New unit test; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. ========== to ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled} policies become unset later, then the corresponding preferences will change to the "false" state (which means that the user would have to enable these features manually if they want to). BUG=690121 BUG=b/34285262 TEST=New unit and browser tests; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:448: TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) { On 2017/02/10 15:40:16, emaxx wrote: > On 2017/02/09 15:28:04, hidehiko wrote: > > Could you use parameterized test so that what failed is easily identified. > > Quick question: do you think this will be better, given that the debug > information is currently printed by the test code too? (Note the last EXPECT_EQ > in this function - all other EXPECT* statements are actually assertions which > should hold regardless of the tested pref values.) > I'm just concerned a little bit about bloating the test code. I do not think using WithParamInterface bloats the code. It'd be something like; class ArcSessionManagerPolicyTest : public ArcSessionManagerTest, public testing::WithParamInterface<std::tuple<base::Value, base::Value>> { const base::Value& GetBackupRestorePref() { return std::get<0>(GetParam()); } const base::Value& GetLocationServicePref() { return std::get<1>(GetParam()); } }; INSTANTIATE_TEST_CASE_P( ArcSessionManagerPolicyTest, ArcSessionManagerPolicyTest, testing::Combine( testing::Values(base::Value(), base::Value(false), base::Value(true)), testing::Values(base::Value(), base::Value(false), base::Value(true)))); So that you can flatten all the contents in the nested for stmt. Additional benefit to the logs is, all inflight test state is reset for each case, so even if some are failed, others won't be affected. WDYT? Note: you may want/need to rebase on top of https://codereview.chromium.org/2691213002/, to avoid name conflicting. https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:334: prefs->ClearPref(pref); In general, side effect in a function to check the condition (e.g. IsSomething, HasSomething, CanSomething, ShouldSomething etc. in Chrome) is practically confusing. Actually, it is also marked as "const". Could you avoid them? Maybe renaming somehow? https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc (right): https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84: const bool checkbox_checked = enabled || !managed; For unmanaged users, even if backup-and-restore is set to false, ArcSupport will receive "true", which sounds unexpected behavior. Could you kindly explain what you'd like to do here? Ditto for below. https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc (right): https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc:29: protected: nit: could you keep public? https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc:128: base::MakeUnique<base::Value>(false).release()); nit: new base::Value(false) BTW, according to the comments of SetManagedPref, it takes the ownership of the Value. Can it be changed to std::unique_ptr<base::Value> (maybe in a separate CL?).
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:448: TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) { On 2017/02/14 11:51:36, hidehiko wrote: > On 2017/02/10 15:40:16, emaxx wrote: > > On 2017/02/09 15:28:04, hidehiko wrote: > > > Could you use parameterized test so that what failed is easily identified. > > > > Quick question: do you think this will be better, given that the debug > > information is currently printed by the test code too? (Note the last > EXPECT_EQ > > in this function - all other EXPECT* statements are actually assertions which > > should hold regardless of the tested pref values.) > > I'm just concerned a little bit about bloating the test code. > > I do not think using WithParamInterface bloats the code. It'd be something like; > > class ArcSessionManagerPolicyTest : public ArcSessionManagerTest, public > testing::WithParamInterface<std::tuple<base::Value, base::Value>> { > > const base::Value& GetBackupRestorePref() { > return std::get<0>(GetParam()); > } > const base::Value& GetLocationServicePref() { > return std::get<1>(GetParam()); > } > }; > > INSTANTIATE_TEST_CASE_P( > ArcSessionManagerPolicyTest, > ArcSessionManagerPolicyTest, > testing::Combine( > testing::Values(base::Value(), base::Value(false), base::Value(true)), > testing::Values(base::Value(), base::Value(false), base::Value(true)))); > > So that you can flatten all the contents in the nested for stmt. > Additional benefit to the logs is, all inflight test state is reset for each > case, so even if some are failed, others won't be affected. > WDYT? > > Note: you may want/need to rebase on top of > https://codereview.chromium.org/2691213002/, to avoid name conflicting. Done. Thanks for the detailed comment! https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:334: prefs->ClearPref(pref); On 2017/02/14 11:51:36, hidehiko wrote: > In general, side effect in a function to check the condition (e.g. IsSomething, > HasSomething, CanSomething, ShouldSomething etc. in Chrome) is practically > confusing. Actually, it is also marked as "const". > > Could you avoid them? > Maybe renaming somehow? Done. I left only non-modifying operations in the Should* methods, and moved modifications into the Sync* methods. (This required to move away from sharing this bit of code, but the duplication probably made the code even clearer.) https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc (right): https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84: const bool checkbox_checked = enabled || !managed; On 2017/02/14 11:51:36, hidehiko wrote: > For unmanaged users, even if backup-and-restore is set to false, ArcSupport will > receive "true", which sounds unexpected behavior. > > Could you kindly explain what you'd like to do here? > > Ditto for below. That's exactly what I was trying to achieve: the pref has its default ("false") value until the ToS screen completes, while the checkbox displayed to the user in the unmanaged case should be "on" by default. Added a comment on that. https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc (right): https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc:29: protected: On 2017/02/14 11:51:36, hidehiko wrote: > nit: could you keep public? Done. (Although I thought it's more common in the Chrome code base to use "protected" in such cases.) https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc:128: base::MakeUnique<base::Value>(false).release()); On 2017/02/14 11:51:36, hidehiko wrote: > nit: new base::Value(false) > > BTW, according to the comments of SetManagedPref, it takes the ownership of the > Value. Can it be changed to std::unique_ptr<base::Value> (maybe in a separate > CL?). Done. Created crbug.com/692121 to track that refactoring. https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:120: void SyncInitialSettings() const; Moved this from the group of other Sync* methods, as I think this method is conceptually separate from them.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc (right): https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84: // Tick the checkbox by default in the unmanaged case, regardless of the pref Ok, then, could you move them to arc_optin_preference_handler.cc instead? Here, callbacks are called on initialization *or update*. So, anytime even the preference is explicitly set to false, the checkbox is set to true (= checked), if it is for unmanaged user. You can see if the value is explicitly set or not by HasPref() in ArcOptinPreferenceHandler.
https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc (right): https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84: // Tick the checkbox by default in the unmanaged case, regardless of the pref On 2017/02/15 04:59:16, hidehiko wrote: > Ok, then, could you move them to arc_optin_preference_handler.cc instead? > Here, callbacks are called on initialization *or update*. > So, anytime even the preference is explicitly set to false, the checkbox is > set to true (= checked), if it is for unmanaged user. > > You can see if the value is explicitly set or not by HasPref() in > ArcOptinPreferenceHandler. > Hmm, but would it be more appropriate in ArcOptinPreferenceHandler? The interface of this class seems to talk about actual prefs, and so is ArcOptInPreferenceHandlerObserver. And it's probably not clean to "patch" the passed pref value for the sole goal of having the desired UI effect (having the checkbox checked by default). Another option is to modify the ArcOptInPreferenceHandlerObserver interface to include the HasPref bit of information, so that the observers may choose how to react...
https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc (right): https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84: // Tick the checkbox by default in the unmanaged case, regardless of the pref On 2017/02/15 05:12:31, emaxx wrote: > On 2017/02/15 04:59:16, hidehiko wrote: > > Ok, then, could you move them to arc_optin_preference_handler.cc instead? > > Here, callbacks are called on initialization *or update*. > > So, anytime even the preference is explicitly set to false, the checkbox is > > set to true (= checked), if it is for unmanaged user. > > > > You can see if the value is explicitly set or not by HasPref() in > > ArcOptinPreferenceHandler. > > > > Hmm, but would it be more appropriate in ArcOptinPreferenceHandler? The > interface of this class seems to talk about actual prefs, I think it is ok to modify ArcOptinPreferenceHandler directly. Quote from the comment: "This helper encapsulates access to preferences"... I think what you're trying to do is changing default value of the preference based on managed state, which is not supported by Preference framework by default. > and so is > ArcOptInPreferenceHandlerObserver. And it's probably not clean to "patch" the > passed pref value for the sole goal of having the desired UI effect (having the > checkbox checked by default). So, in the above sense, it is not only for changing checkbox default value, but also changing default value for the preference (assuming all access to the preference should pass through the class). What I'm imaging is something like; https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/optin/arc_op... bool is_managed = pref_service->IsManagedPreference(...); // If the preference is not set, switch the default value based on whether // it is managed preference or not: "true" if unmanaged, otherwise "false". bool value = pref_service_->HasPrefPath(kArcBackupRestoreEnabled) ? pref_service_->GetBoolean(...) : !is_managed; observer_->OnBackupAndRestoreModeChanged(value, is_managed); WDYT? > > Another option is to modify the ArcOptInPreferenceHandlerObserver interface to > include the HasPref bit of information, so that the observers may choose how to > react...
Hidehiko, I see now. But wouldn't this be a bit fragile in the sense that some consumers that read the pref value directly would get the contradicting information comparing to the consumers of ArcOptinPreferenceHandler? There is already one such consumer - ArcSettingsServiceImpl; it's fine as it works only after optin, but there may be more added in the future.
High level comment. We should handle OOBE case too. Please see chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.cc chrome/browser/chromeos/login/wizard_controller.cc (ShowArcTermsOfServiceScreen)
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
As per offline discussion with Hidehiko, I've updated the CL to use the simpler approach with putting the logic around default values into ArcOptInPreferenceHandler. This allows to reuse the same logic for both UIs: in-session (ArcTermsOfServiceDefaultNegotiator) and OOBE (ArcTermsOfServiceScreenHandler). Also placed a comment to clarify that ArcOptInPreferenceHandler should only be used for OptIn flow. On 2017/02/15 16:06:06, khmel wrote: > High level comment. > > We should handle OOBE case too. Thanks, this is fixed now. The checkboxes will receive the correct values (true by default, and the managed value if there's one). Note that the ARC OOBE screen won't be skipped even when all policies are set - this should be tackled in a separate task.
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2682833003/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h (right): https://codereview.chromium.org/2682833003/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h:24: // Note that the preferences and metrics mode passed by this class should only Nice comment! https://codereview.chromium.org/2682833003/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h (right): https://codereview.chromium.org/2682833003/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h:13: // Notifies that the metrics mode has been changed. Thank you for clean up!
The CQ bit was checked by emaxx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h: While running git apply --index -p1; error: patch failed: chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h:10 error: chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h: patch does not apply Patch: chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h Index: chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h diff --git a/chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h b/chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h index c1518c2b6e67799b943736fc6b14c3425e714d14..f41f8726c47b195fd5ada78135bc02ab48892688 100644 --- a/chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h +++ b/chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h @@ -10,11 +10,11 @@ namespace arc { // Notifies about changes in Arc related preferences and metrics mode. class ArcOptInPreferenceHandlerObserver { public: - // Notifies metrics mode has been changed. + // Notifies that the metrics mode has been changed. virtual void OnMetricsModeChanged(bool enabled, bool managed) = 0; - // Notifies use backup and restore preference has been changed. + // Notifies that the backup and restore mode has been changed. virtual void OnBackupAndRestoreModeChanged(bool enabled, bool managed) = 0; - // Notifies location service consent preference has been changed. + // Notifies that the location service mode has been changed. virtual void OnLocationServicesModeChanged(bool enabled, bool managed) = 0; virtual ~ArcOptInPreferenceHandlerObserver() = default;
The CQ bit was checked by emaxx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2682833003/#ps280001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 280001, "attempt_start_ts": 1487350171215170, "parent_rev": "54e8d335c0e4bebe49fbb2cacc5e49c06f29dc6e", "commit_rev": "1f0e3460633a614baa787c46a2c8031fc13c8dad"}
Message was sent while issue was closed.
Description was changed from ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled} policies become unset later, then the corresponding preferences will change to the "false" state (which means that the user would have to enable these features manually if they want to). BUG=690121 BUG=b/34285262 TEST=New unit and browser tests; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. ========== to ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled} policies become unset later, then the corresponding preferences will change to the "false" state (which means that the user would have to enable these features manually if they want to). BUG=690121 BUG=b/34285262 TEST=New unit and browser tests; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. Review-Url: https://codereview.chromium.org/2682833003 Cr-Commit-Position: refs/heads/master@{#451340} Committed: https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8...
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2702893002/ by alph@chromium.org. The reason for reverting is: Tentative revert for breaking RestoreOnStartupTestChromeOS.PRE_LogInAndVerify https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....
Message was sent while issue was closed.
Description was changed from ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled} policies become unset later, then the corresponding preferences will change to the "false" state (which means that the user would have to enable these features manually if they want to). BUG=690121 BUG=b/34285262 TEST=New unit and browser tests; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. Review-Url: https://codereview.chromium.org/2682833003 Cr-Commit-Position: refs/heads/master@{#451340} Committed: https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8... ========== to ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled} policies become unset later, then the corresponding preferences will change to the "false" state (which means that the user would have to enable these features manually if they want to). BUG=690121 BUG=b/34285262 TEST=New unit and browser tests; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. Review-Url: https://codereview.chromium.org/2682833003 Cr-Commit-Position: refs/heads/master@{#451340} Committed: https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8... ==========
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 280001, "attempt_start_ts": 1487392382829600, "parent_rev": "7c0ecf945d1514a474eb184587112cd7f50b7e9d", "commit_rev": "f392d837d227d79a9b8dec1d298e9c562929c7cf"}
Message was sent while issue was closed.
Description was changed from ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled} policies become unset later, then the corresponding preferences will change to the "false" state (which means that the user would have to enable these features manually if they want to). BUG=690121 BUG=b/34285262 TEST=New unit and browser tests; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. Review-Url: https://codereview.chromium.org/2682833003 Cr-Commit-Position: refs/heads/master@{#451340} Committed: https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8... ========== to ========== Skip ARC initial screen when everything is set up by policy Skip the ARC initial screen during provision when there is nothing to be requested from the user on that screen, i.e. when the following conditions are all true: 1. ArcEnabled policy is set to true; 2. ArcBackupRestoreEnabled policy is set to some value; 3. ArcLocationServiceEnabled policy is set to some value. Note that the subsequent screen with the progress spinner will also be hidden. This is the intended behavior - the UI feedback for the user, if we decide to add it, will have to be implemented in a different, less noisy for the user way. If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled} policies become unset later, then the corresponding preferences will change to the "false" state (which means that the user would have to enable these features manually if they want to). BUG=690121 BUG=b/34285262 TEST=New unit and browser tests; Manual test: enable ARC by policy, set ArcBackupRestoreEnabled and ArcLocationServiceEnabled policies to some values, start a fresh profile, check that ARC eventually gets provisioned without showing any dialogs. Review-Url: https://codereview.chromium.org/2682833003 Cr-Original-Commit-Position: refs/heads/master@{#451340} Committed: https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8... Review-Url: https://codereview.chromium.org/2682833003 Cr-Commit-Position: refs/heads/master@{#451437} Committed: https://chromium.googlesource.com/chromium/src/+/f392d837d227d79a9b8dec1d298e... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/f392d837d227d79a9b8dec1d298e... |