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

Issue 2954293002: Chromad: Prevent session from starting without policy (Closed)

Created:
3 years, 5 months ago by Thiemo Nagel
Modified:
3 years, 5 months ago
Reviewers:
stevenjb, emaxx
CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Interface and implementation are modeled after UserCloudPolicyManagerChromeOS. Also added some slight modifications/explanations to the latter. BUG=732801 TEST=added unit tests Review-Url: https://codereview.chromium.org/2954293002 Cr-Commit-Position: refs/heads/master@{#487791} Committed: https://chromium.googlesource.com/chromium/src/+/cab862ac8252d68584eaf94f9b19e639f03200d7

Patch Set 1 : Integrate session exit into tests #

Patch Set 2 : Comment fixes #

Total comments: 14

Patch Set 3 : Polish #

Patch Set 4 : Fix initialization complete for component policy #

Total comments: 10

Patch Set 5 : Address nits #

Total comments: 10

Patch Set 6 : Drop |wait_for_policy_fetch| and minor improvements #

Patch Set 7 : Move MockAuthPolicyClient into unittest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+686 lines, -96 lines) Patch
M chrome/browser/chromeos/BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chromeos/policy/active_directory_policy_manager.h View 1 2 3 4 5 5 chunks +64 lines, -7 lines 0 comments Download
M chrome/browser/chromeos/policy/active_directory_policy_manager.cc View 1 2 3 4 5 7 chunks +104 lines, -9 lines 0 comments Download
A chrome/browser/chromeos/policy/active_directory_policy_manager_unittest.cc View 1 2 3 4 5 6 1 chunk +432 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h View 1 2 3 4 5 4 chunks +19 lines, -20 lines 0 comments Download
M chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc View 1 2 3 4 5 12 chunks +33 lines, -33 lines 0 comments Download
M chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc View 1 2 3 4 5 14 chunks +19 lines, -22 lines 0 comments Download
M chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc View 1 2 3 4 5 3 chunks +7 lines, -5 lines 0 comments Download
M chromeos/dbus/dbus_thread_manager.h View 1 chunk +1 line, -0 lines 0 comments Download
M chromeos/dbus/dbus_thread_manager.cc View 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 72 (54 generated)
Thiemo Nagel
Hey Maksim, may I ask you to take a look? This should be equivalent to ...
3 years, 5 months ago (2017-07-11 14:19:39 UTC) #26
emaxx
On 2017/07/11 14:19:39, Thiemo Nagel wrote: > Hey Maksim, > > may I ask you ...
3 years, 5 months ago (2017-07-11 14:40:38 UTC) #27
Thiemo Nagel
> Will try to take a look by tomorrow's EOD, sorry. Thank you, np!
3 years, 5 months ago (2017-07-11 16:10:55 UTC) #30
emaxx
Haven't read the tests yet, but I'm a bit concerned by the complexity around state ...
3 years, 5 months ago (2017-07-12 20:18:20 UTC) #32
Thiemo Nagel
I've addressed your comments and added/improved docs. Could you please take another look? https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeos/policy/active_directory_policy_manager.cc File ...
3 years, 5 months ago (2017-07-14 12:18:56 UTC) #35
Thiemo Nagel
https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeos/policy/active_directory_policy_manager.cc File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeos/policy/active_directory_policy_manager.cc#newcode86 chrome/browser/chromeos/policy/active_directory_policy_manager.cc:86: return false; > Since we don't support component policy ...
3 years, 5 months ago (2017-07-14 12:21:29 UTC) #38
emaxx
LGTM % nits https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeos/policy/active_directory_policy_manager.cc File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeos/policy/active_directory_policy_manager.cc#newcode98 chrome/browser/chromeos/policy/active_directory_policy_manager.cc:98: CancelWaitForInitialPolicy(fetch_ever_succeeded_ /* success */); nit: What ...
3 years, 5 months ago (2017-07-14 13:56:41 UTC) #39
Thiemo Nagel
Many thanks! I've addressed the nits. Would you like to take another look? https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeos/policy/active_directory_policy_manager.cc File ...
3 years, 5 months ago (2017-07-17 09:29:35 UTC) #44
Thiemo Nagel
Hey Steven, may I ask you to take a look at this tiny change in ...
3 years, 5 months ago (2017-07-17 09:32:50 UTC) #46
emaxx
On 2017/07/17 09:29:35, Thiemo Nagel wrote: > Many thanks! I've addressed the nits. Would you ...
3 years, 5 months ago (2017-07-17 09:38:08 UTC) #47
stevenjb
https://codereview.chromium.org/2954293002/diff/200001/chrome/browser/chromeos/policy/active_directory_policy_manager.cc File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/200001/chrome/browser/chromeos/policy/active_directory_policy_manager.cc#newcode137 chrome/browser/chromeos/policy/active_directory_policy_manager.cc:137: DCHECK_NE(wait_for_policy_fetch, initial_policy_fetch_timeout.is_zero()); nit: Do we need |wait_for_policy_fetch| as a ...
3 years, 5 months ago (2017-07-17 16:07:38 UTC) #50
Thiemo Nagel
Many thanks Steven! Could you please take another look? https://codereview.chromium.org/2954293002/diff/200001/chrome/browser/chromeos/policy/active_directory_policy_manager.cc File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/200001/chrome/browser/chromeos/policy/active_directory_policy_manager.cc#newcode137 chrome/browser/chromeos/policy/active_directory_policy_manager.cc:137: ...
3 years, 5 months ago (2017-07-18 13:02:18 UTC) #56
stevenjb
lgtm https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_auth_policy_client.h File chromeos/dbus/mock_auth_policy_client.h (right): https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_auth_policy_client.h#newcode33 chromeos/dbus/mock_auth_policy_client.h:33: } On 2017/07/18 13:02:18, Thiemo Nagel wrote: > ...
3 years, 5 months ago (2017-07-18 15:37:44 UTC) #59
Thiemo Nagel
Thank you! Would like to take another look? (I plan to land tomorrow unless there ...
3 years, 5 months ago (2017-07-18 16:03:32 UTC) #62
stevenjb
I do like this better, thanks. lgtm++
3 years, 5 months ago (2017-07-18 16:06:05 UTC) #63
Thiemo Nagel
On 2017/07/18 16:06:05, stevenjb wrote: > I do like this better, thanks. lgtm++ Thanks again!
3 years, 5 months ago (2017-07-19 08:14:55 UTC) #66
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/2954293002/260001
3 years, 5 months ago (2017-07-19 08:15:15 UTC) #69
commit-bot: I haz the power
3 years, 5 months ago (2017-07-19 08:20:16 UTC) #72
Message was sent while issue was closed.
Committed patchset #7 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/cab862ac8252d68584eaf94f9b19...

Powered by Google App Engine
This is Rietveld 408576698