Chromium Code Reviews
Help | Chromium Project | Sign in
(13)

Issue 2682833003: Skip ARC initial screen when everything is set up by policy (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 2 weeks ago by emaxx
Modified:
1 month, 1 week 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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+364 lines, -49 lines) Patch
M chrome/browser/chromeos/arc/arc_session_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +25 lines, -7 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_session_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +73 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/extensions/fake_arc_support.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc View 1 2 3 4 5 6 7 8 9 10 11 9 chunks +61 lines, -21 lines 0 comments Download
M chrome/browser/chromeos/arc/intent_helper/arc_settings_service_browsertest.cc View 1 2 3 4 5 chunks +98 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +14 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc View 1 2 3 4 5 6 chunks +64 lines, -7 lines 0 comments Download
M chrome/browser/policy/policy_browsertest.cc View 1 2 3 4 5 6 4 chunks +18 lines, -6 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 97 (74 generated)
emaxx
Yusuke, could you PTAL?
1 month, 2 weeks ago (2017-02-08 20:02:07 UTC) #12
Yusuke Sato
+hidehiko who would be the best reviewer for the files.
1 month, 2 weeks ago (2017-02-08 20:37:18 UTC) #14
hidehiko
Posted a question on the bug. https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos/arc/arc_session_manager.cc File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos/arc/arc_session_manager.cc#newcode530 chrome/browser/chromeos/arc/arc_session_manager.cc:530: if (!g_disable_ui_for_testing && ...
1 month, 2 weeks ago (2017-02-09 15:28:04 UTC) #20
emaxx
On 2017/02/09 15:28:04, hidehiko wrote: > Posted a question on the bug. So I implemented ...
1 month, 2 weeks ago (2017-02-10 15:40:16 UTC) #25
emaxx
Please hold on reviewing. After some offline discussion, an idea for how to deal with ...
1 month, 2 weeks ago (2017-02-10 17:22:18 UTC) #26
emaxx
I've updated the CL to support the "managed pref"=>"unmanaged pref" transition. PTAL. +khmel: Yury, as ...
1 month, 1 week ago (2017-02-13 17:18:55 UTC) #34
hidehiko
https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc#newcode448 chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:448: TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) { On 2017/02/10 15:40:16, emaxx wrote: > ...
1 month, 1 week ago (2017-02-14 11:51:36 UTC) #39
emaxx
https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc#newcode448 chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:448: TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) { On 2017/02/14 11:51:36, hidehiko wrote: > ...
1 month, 1 week ago (2017-02-15 03:40:59 UTC) #49
hidehiko
https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc (right): https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc#newcode84 chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84: // Tick the checkbox by default in the unmanaged ...
1 month, 1 week ago (2017-02-15 04:59:16 UTC) #53
emaxx
https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc (right): https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc#newcode84 chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84: // Tick the checkbox by default in the unmanaged ...
1 month, 1 week ago (2017-02-15 05:12:31 UTC) #54
hidehiko
https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc (right): https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc#newcode84 chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84: // Tick the checkbox by default in the unmanaged ...
1 month, 1 week ago (2017-02-15 14:38:43 UTC) #55
emaxx
Hidehiko, I see now. But wouldn't this be a bit fragile in the sense that ...
1 month, 1 week ago (2017-02-15 15:15:26 UTC) #56
khmel
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)
1 month, 1 week ago (2017-02-15 16:06:06 UTC) #57
emaxx
As per offline discussion with Hidehiko, I've updated the CL to use the simpler approach ...
1 month, 1 week ago (2017-02-16 16:57:45 UTC) #66
emaxx
1 month, 1 week ago (2017-02-17 00:00:41 UTC) #79
hidehiko
lgtm https://codereview.chromium.org/2682833003/diff/260001/chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h File chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h (right): https://codereview.chromium.org/2682833003/diff/260001/chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h#newcode24 chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h:24: // Note that the preferences and metrics mode ...
1 month, 1 week ago (2017-02-17 09:03:01 UTC) #80
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2682833003/260001
1 month, 1 week ago (2017-02-17 16:26:17 UTC) #82
commit-bot: I haz the power
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: ...
1 month, 1 week ago (2017-02-17 16:36:39 UTC) #84
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2682833003/280001
1 month, 1 week ago (2017-02-17 16:50:01 UTC) #87
commit-bot: I haz the power
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8031fc13c8dad
1 month, 1 week ago (2017-02-17 18:17:05 UTC) #90
alph
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2702893002/ by alph@chromium.org. ...
1 month, 1 week ago (2017-02-18 02:08:23 UTC) #91
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2682833003/280001
1 month, 1 week ago (2017-02-18 04:33:27 UTC) #94
commit-bot: I haz the power
1 month, 1 week ago (2017-02-18 07:05:01 UTC) #97
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/f392d837d227d79a9b8dec1d298e...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62