Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2847853003:
Remove policy watching from It2MeHost. (Closed)
DescriptionRemove policy watching from It2MeHost.
This functionality is already present in It2MeNativeMessagingHost, which
owns the It2MeHost and can simply forward the policy changes to it.
This CL is part of a larger change to make the It2Me host react to
policy errors, at which point it will be more convenient to have a
single policy watcher.
BUG=433009
Review-Url: https://codereview.chromium.org/2847853003
Cr-Commit-Position: refs/heads/master@{#469834}
Committed: https://chromium.googlesource.com/chromium/src/+/cf2cc68d4e6a4a70bcadfd5491f90d119129ec38
Patch Set 1 #Patch Set 2 : Handle policies read before the host starts. #Patch Set 3 : Remove pending policies from It2MeNativeMessagingHost and add an accessor to PolicyWatcher. #
Total comments: 12
Patch Set 4 : Reviewer feedback. #Patch Set 5 : Fix unit test. #Patch Set 6 : Rebase #Patch Set 7 : Supply default policy values for tests unless overridden. #Patch Set 8 : Fix unit tests #
Total comments: 9
Patch Set 9 : Reviewer feedback. #
Total comments: 2
Patch Set 10 : Allow PolicyWatcher to be injected into It2MeNativeMessagingHost. #
Total comments: 2
Patch Set 11 : Don't read real policies. #
Total comments: 15
Patch Set 12 : Reviewer feedback. #Patch Set 13 : Reviewer feedback. #Patch Set 14 : Fix ChromeOS compile. #Patch Set 15 : Fix DEPS #Patch Set 16 : Add dep. #Messages
Total messages: 52 (19 generated)
jamiewalch@chromium.org changed reviewers: + joedow@chromium.org, rkjnsn@chromium.org
jamiewalch@chromium.org changed required reviewers: + joedow@chromium.org
Joe, PTAL Erik, FYI in case you have comments based on your previous work in this area.
Haven't looked in detail, yet, but a couple initial comments. https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:222: std::unique_ptr<base::DictionaryValue> dictionary(new base::DictionaryValue); nit: I think MakeUnique is more readable: auto dictionary = base::MakeUnique<base::DictionaryValue>(); https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:226: it2me_host_->OnPolicyUpdate(std::move(dictionary)); Note that this will bypass normalization and deprecation logic in PolicyWatcher, which means most of the tests will be broken after https://codereview.chromium.org/2682473003/ lands.
https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:226: it2me_host_->OnPolicyUpdate(std::move(dictionary)); On 2017/04/28 21:15:36, rkjnsn wrote: > Note that this will bypass normalization and deprecation logic in PolicyWatcher, > which means most of the tests will be broken after > https://codereview.chromium.org/2682473003/ lands. Using the old system (PolicyWatcher + FakeAsyncPolicyLoader) means you get our default values too so you dpon't have to init once with empty list in StartHost().
PTAL https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:222: std::unique_ptr<base::DictionaryValue> dictionary(new base::DictionaryValue); On 2017/04/28 21:15:36, rkjnsn wrote: > nit: I think MakeUnique is more readable: > auto dictionary = base::MakeUnique<base::DictionaryValue>(); Done. https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:226: it2me_host_->OnPolicyUpdate(std::move(dictionary)); On 2017/04/28 21:15:36, rkjnsn wrote: > Note that this will bypass normalization and deprecation logic in PolicyWatcher, > which means most of the tests will be broken after > https://codereview.chromium.org/2682473003/ lands. I'll hold off landing until that's in then. I think the right approach will be just to simplify these tests not to expect that normalization, since it's already tested in the policy watcher tests. https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:226: it2me_host_->OnPolicyUpdate(std::move(dictionary)); On 2017/04/28 21:46:32, joedow wrote: > On 2017/04/28 21:15:36, rkjnsn wrote: > > Note that this will bypass normalization and deprecation logic in > PolicyWatcher, > > which means most of the tests will be broken after > > https://codereview.chromium.org/2682473003/ lands. > > Using the old system (PolicyWatcher + FakeAsyncPolicyLoader) means you get our > default values too so you dpon't have to init once with empty list in > StartHost(). The tests hang without a call to SetPolicies (I think because the It2Me host needs OnPolicyUpdate to be called before it does anything).
https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:226: it2me_host_->OnPolicyUpdate(std::move(dictionary)); On 2017/04/28 22:41:09, Jamie wrote: > On 2017/04/28 21:46:32, joedow wrote: > > On 2017/04/28 21:15:36, rkjnsn wrote: > > > Note that this will bypass normalization and deprecation logic in > > PolicyWatcher, > > > which means most of the tests will be broken after > > > https://codereview.chromium.org/2682473003/ lands. > > > > Using the old system (PolicyWatcher + FakeAsyncPolicyLoader) means you get our > > default values too so you dpon't have to init once with empty list in > > StartHost(). > > The tests hang without a call to SetPolicies (I think because the It2Me host > needs OnPolicyUpdate to be called before it does anything). It2MeHost will indeed wait for OnPolicyUpdate to be called the first time. One possibility would be to keep an actual instance of PolicyWatcher in It2MeTest. That way, It2MeHost will get the default values as managed by PolicyWatcher instead of an empty list for the initial call, and deprecation and normalization will still happen (obviating the need to update the tests).
https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:226: it2me_host_->OnPolicyUpdate(std::move(dictionary)); On 2017/05/02 19:19:12, rkjnsn wrote: > On 2017/04/28 22:41:09, Jamie wrote: > > On 2017/04/28 21:46:32, joedow wrote: > > > On 2017/04/28 21:15:36, rkjnsn wrote: > > > > Note that this will bypass normalization and deprecation logic in > > > PolicyWatcher, > > > > which means most of the tests will be broken after > > > > https://codereview.chromium.org/2682473003/ lands. > > > > > > Using the old system (PolicyWatcher + FakeAsyncPolicyLoader) means you get > our > > > default values too so you dpon't have to init once with empty list in > > > StartHost(). > > > > The tests hang without a call to SetPolicies (I think because the It2Me host > > needs OnPolicyUpdate to be called before it does anything). > > It2MeHost will indeed wait for OnPolicyUpdate to be called the first time. One > possibility would be to keep an actual instance of PolicyWatcher in It2MeTest. > That way, It2MeHost will get the default values as managed by PolicyWatcher > instead of an empty list for the initial call, and deprecation and normalization > will still happen (obviating the need to update the tests). I think that's mixing up responsibilities too much. This CL allows It2MeHost to no longer care where its policies are coming from, so the test should provide them in as simple a way as possible. Adding a PolicyWatcher to the test seems overly complicated, and we shouldn't test normalization here, since it's not a feature of It2MeHost.
https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:226: it2me_host_->OnPolicyUpdate(std::move(dictionary)); On 2017/05/02 22:07:56, Jamie wrote: > On 2017/05/02 19:19:12, rkjnsn wrote: > > On 2017/04/28 22:41:09, Jamie wrote: > > > On 2017/04/28 21:46:32, joedow wrote: > > > > On 2017/04/28 21:15:36, rkjnsn wrote: > > > > > Note that this will bypass normalization and deprecation logic in > > > > PolicyWatcher, > > > > > which means most of the tests will be broken after > > > > > https://codereview.chromium.org/2682473003/ lands. > > > > > > > > Using the old system (PolicyWatcher + FakeAsyncPolicyLoader) means you get > > our > > > > default values too so you dpon't have to init once with empty list in > > > > StartHost(). > > > > > > The tests hang without a call to SetPolicies (I think because the It2Me host > > > needs OnPolicyUpdate to be called before it does anything). > > > > It2MeHost will indeed wait for OnPolicyUpdate to be called the first time. One > > possibility would be to keep an actual instance of PolicyWatcher in It2MeTest. > > That way, It2MeHost will get the default values as managed by PolicyWatcher > > instead of an empty list for the initial call, and deprecation and > normalization > > will still happen (obviating the need to update the tests). > > I think that's mixing up responsibilities too much. This CL allows It2MeHost to > no longer care where its policies are coming from, so the test should provide > them in as simple a way as possible. Adding a PolicyWatcher to the test seems > overly complicated, and we shouldn't test normalization here, since it's not a > feature of It2MeHost. I mostly agree. My main concern is here was the difference in behavior for the initial call to OnPolicyUpdate. Here we initially call it with an empty dictionary, whereas PolicyWatcher would call it with a dictionary that had all of the policies set to there default values. This could potentially lead to different behavior. E.g., with the approach here, the Update*Policy methods are never called unless the corresponding policy is set by the test, whereas with PolicyWatcher they are always called at least once. Also, the defaults might be different (e.g., It2MeHost defaults nat_traversal_enabled_ to false, but PolicyWatcher defaults the RemoteAccessHostFirewallTraversal policy to true). Now, it's probably true that none of these matter, and if they ever do, the SetPolicies({}) call below can be adjusted as needed. Thus, the current approach should be fine. It may be worth putting a comment on the SetPolicies({}) call on 244 to call out the differing behavior, though.
https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:226: it2me_host_->OnPolicyUpdate(std::move(dictionary)); On 2017/05/02 23:20:19, rkjnsn wrote: > I mostly agree. My main concern is here was the difference in behavior for the > initial call to OnPolicyUpdate. Here we initially call it with an empty > dictionary, whereas PolicyWatcher would call it with a dictionary that had all > of the policies set to there default values. This could potentially lead to > different behavior. E.g., with the approach here, the Update*Policy methods are > never called unless the corresponding policy is set by the test, whereas with > PolicyWatcher they are always called at least once. Also, the defaults might be > different (e.g., It2MeHost defaults nat_traversal_enabled_ to false, but > PolicyWatcher defaults the RemoteAccessHostFirewallTraversal policy to true). > > Now, it's probably true that none of these matter, and if they ever do, the > SetPolicies({}) call below can be adjusted as needed. Thus, the current approach > should be fine. It may be worth putting a comment on the SetPolicies({}) call on > 244 to call out the differing behavior, though. their*
https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:226: it2me_host_->OnPolicyUpdate(std::move(dictionary)); On 2017/05/02 23:27:19, rkjnsn wrote: > On 2017/05/02 23:20:19, rkjnsn wrote: > > I mostly agree. My main concern is here was the difference in behavior for the > > initial call to OnPolicyUpdate. Here we initially call it with an empty > > dictionary, whereas PolicyWatcher would call it with a dictionary that had all > > of the policies set to there default values. This could potentially lead to > > different behavior. E.g., with the approach here, the Update*Policy methods > are > > never called unless the corresponding policy is set by the test, whereas with > > PolicyWatcher they are always called at least once. Also, the defaults might > be > > different (e.g., It2MeHost defaults nat_traversal_enabled_ to false, but > > PolicyWatcher defaults the RemoteAccessHostFirewallTraversal policy to true). > > > > Now, it's probably true that none of these matter, and if they ever do, the > > SetPolicies({}) call below can be adjusted as needed. Thus, the current > approach > > should be fine. It may be worth putting a comment on the SetPolicies({}) call > on > > 244 to call out the differing behavior, though. > > their* That's a good point. I was going to argue that It2MeHost should have suitable defaults set for its member variables, but on reflection I don't think I believe that, since those will be the same defaults as are set in the policy (or derived from them) and duplication is never a good idea. Perhaps the best solution is to instantiate a PolicyWatcher, but just use its GetCurrentPolicies method to pre-set the defaults, and allow individual tests to call SetPolicies to override anything they like. That still decouples It2MeHost from the mechanics of PolicyWatcher, but does ensure that the correct default values are set. WDYT?
PTAL https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:226: it2me_host_->OnPolicyUpdate(std::move(dictionary)); On 2017/05/02 23:49:03, Jamie wrote: > On 2017/05/02 23:27:19, rkjnsn wrote: > > On 2017/05/02 23:20:19, rkjnsn wrote: > > > I mostly agree. My main concern is here was the difference in behavior for > the > > > initial call to OnPolicyUpdate. Here we initially call it with an empty > > > dictionary, whereas PolicyWatcher would call it with a dictionary that had > all > > > of the policies set to there default values. This could potentially lead to > > > different behavior. E.g., with the approach here, the Update*Policy methods > > are > > > never called unless the corresponding policy is set by the test, whereas > with > > > PolicyWatcher they are always called at least once. Also, the defaults might > > be > > > different (e.g., It2MeHost defaults nat_traversal_enabled_ to false, but > > > PolicyWatcher defaults the RemoteAccessHostFirewallTraversal policy to > true). > > > > > > Now, it's probably true that none of these matter, and if they ever do, the > > > SetPolicies({}) call below can be adjusted as needed. Thus, the current > > approach > > > should be fine. It may be worth putting a comment on the SetPolicies({}) > call > > on > > > 244 to call out the differing behavior, though. > > > > their* > > That's a good point. I was going to argue that It2MeHost should have suitable > defaults set for its member variables, but on reflection I don't think I believe > that, since those will be the same defaults as are set in the policy (or derived > from them) and duplication is never a good idea. > > Perhaps the best solution is to instantiate a PolicyWatcher, but just use its > GetCurrentPolicies method to pre-set the defaults, and allow individual tests to > call SetPolicies to override anything they like. That still decouples It2MeHost > from the mechanics of PolicyWatcher, but does ensure that the correct default > values are set. WDYT? I've taken this approach in the most recent iteration.
https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host.cc:474: // way to communicate changes to the user. Out of curiosity, in most places we try to enforce policy changes immediately, even if it requires disconnecting the sessions and restarting the host. What makes this one different? https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host_unittest.cc:399: bool nat_policy_received = false; I see this was added to fix the unit test, but I can't figure out it falls out from the other changes. https://codereview.chromium.org/2847853003/diff/140001/remoting/host/policy_w... File remoting/host/policy_watcher_unittest.cc (left): https://codereview.chromium.org/2847853003/diff/140001/remoting/host/policy_w... remoting/host/policy_watcher_unittest.cc:303: private: Is this change related? It doesn't look like you use this method in your GetCurrentPolicies test.
PTAL https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host.cc:474: // way to communicate changes to the user. On 2017/05/03 21:31:47, rkjnsn wrote: > Out of curiosity, in most places we try to enforce policy changes immediately, > even if it requires disconnecting the sessions and restarting the host. What > makes this one different? I'm not sure (Joe might be able to shed some light on it). I can always fix that up as part of the CL that implements policy error handling. https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host_unittest.cc:399: bool nat_policy_received = false; On 2017/05/03 21:31:47, rkjnsn wrote: > I see this was added to fix the unit test, but I can't figure out it falls out > from the other changes. The change to the native messaging host means that It2MeHost::OnPolicyUpdate is now being called whereas previously it wasn't, which results in additional message being sent. Does that answer your question? https://codereview.chromium.org/2847853003/diff/140001/remoting/host/policy_w... File remoting/host/policy_watcher_unittest.cc (left): https://codereview.chromium.org/2847853003/diff/140001/remoting/host/policy_w... remoting/host/policy_watcher_unittest.cc:303: private: On 2017/05/03 21:31:47, rkjnsn wrote: > Is this change related? It doesn't look like you use this method in your > GetCurrentPolicies test. You're right; I was using it in an earlier iteration. Reverted.
https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host.cc:474: // way to communicate changes to the user. On 2017/05/03 23:41:10, Jamie wrote: > On 2017/05/03 21:31:47, rkjnsn wrote: > > Out of curiosity, in most places we try to enforce policy changes immediately, > > even if it requires disconnecting the sessions and restarting the host. What > > makes this one different? > > I'm not sure (Joe might be able to shed some light on it). I can always fix that > up as part of the CL that implements policy error handling. Not all policy changes force a restart (for instance the Me2Me host will restart for auth related policies but not others (OnUsernamePolicyUpdate for example)). IIRC my thought was that this was something that would get enabled so the likely transition would be from non-UiAccess to UiAccess. Going the other way would mean the remote user has UiAccess for the rest of the session, but that isn't a big concern (in the future I would like to enable this regardless of policy).
https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host.cc:474: // way to communicate changes to the user. On 2017/05/04 17:24:22, joedow wrote: > On 2017/05/03 23:41:10, Jamie wrote: > > On 2017/05/03 21:31:47, rkjnsn wrote: > > > Out of curiosity, in most places we try to enforce policy changes > immediately, > > > even if it requires disconnecting the sessions and restarting the host. What > > > makes this one different? > > > > I'm not sure (Joe might be able to shed some light on it). I can always fix > that > > up as part of the CL that implements policy error handling. > > Not all policy changes force a restart (for instance the Me2Me host will restart > for auth related policies but not others (OnUsernamePolicyUpdate for example)). > > IIRC my thought was that this was something that would get enabled so the likely > transition would be from non-UiAccess to UiAccess. Going the other way would > mean the remote user has UiAccess for the rest of the session, but that isn't a > big concern (in the future I would like to enable this regardless of policy). > Isn't the native messaging host potentially long-running, though? That is, won't the old policy potentially be used for multiple it2me sessions after having been changed? Either way, I'm not concerned about it for this CL, since the behavior is unchanged. https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/140001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host_unittest.cc:399: bool nat_policy_received = false; On 2017/05/03 23:41:10, Jamie wrote: > On 2017/05/03 21:31:47, rkjnsn wrote: > > I see this was added to fix the unit test, but I can't figure out it falls out > > from the other changes. > > The change to the native messaging host means that It2MeHost::OnPolicyUpdate is > now being called whereas previously it wasn't, which results in additional > message being sent. Does that answer your question? Ah, I see. Since MockIt2MeHost overrides Connect, which before this change would have normally kicked off reading the policy, the policy was never read and OnNatPolicyChanged was never getting called. Now that the code is calling OnPolicyUpdate explicitly, which isn't overridden by the mock, OnNatPolicyChanged is getting called as a result. https://codereview.chromium.org/2847853003/diff/160001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/2847853003/diff/160001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host.cc:323: it2me_host_->OnPolicyUpdate(policy_watcher_->GetCurrentPolicies()); Looking at the code, I realized that It2MeNativeMessagingHost creates a PolicyWatcher using the standard Create method, even when running tests. That means that with this change, it2me_host will be passed the real policies from the local machine when running the It2MeNativeMessagingHost tests. I don't *think* that can cause a test failure, since we override Connect and Disconnect for testing, but it might make it easier for a future change to introduce test dependence on the host system.
https://codereview.chromium.org/2847853003/diff/160001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/2847853003/diff/160001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host.cc:323: it2me_host_->OnPolicyUpdate(policy_watcher_->GetCurrentPolicies()); On 2017/05/04 20:45:45, rkjnsn wrote: > Looking at the code, I realized that It2MeNativeMessagingHost creates a > PolicyWatcher using the standard Create method, even when running tests. That > means that with this change, it2me_host will be passed the real policies from > the local machine when running the It2MeNativeMessagingHost tests. I don't > *think* that can cause a test failure, since we override Connect and Disconnect > for testing, but it might make it easier for a future change to introduce test > dependence on the host system. Given that It2MeNativeMessagingHost is currently constructed from a PolicyService instance, which is only used to construct the PolicyWatcher, I think it might as well just be constructed with a PolicyWatcher directly, which would allow tests to control it. I think that belongs in this CL.
PTAL
Mostly looks good, with one comment. https://codereview.chromium.org/2847853003/diff/180001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/180001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host_unittest.cc:536: fake_policy_service_.get(), context->file_task_runner()); This will still create a PolicyWatcher that reads the real policy from the current system. To isolate the tests, you'll need to use PolicyWatcher::CreateFromPolicyLoaderForTesting along with policy::FakeAsyncPolicyLoader.
PTAL https://codereview.chromium.org/2847853003/diff/180001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/180001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host_unittest.cc:536: fake_policy_service_.get(), context->file_task_runner()); On 2017/05/04 22:52:40, rkjnsn wrote: > This will still create a PolicyWatcher that reads the real policy from the > current system. To isolate the tests, you'll need to use > PolicyWatcher::CreateFromPolicyLoaderForTesting along with > policy::FakeAsyncPolicyLoader. Done (it let me delete FakePolicyService entirely, which was a nice bonus).
lgtm
Joe, can I get an LGTM for committers?
lgtm https://codereview.chromium.org/2847853003/diff/200001/remoting/host/it2me/it... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/2847853003/diff/200001/remoting/host/it2me/it... remoting/host/it2me/it2me_host.h:96: // Called when initial policies are read, and when they change. nit: comma isn't needed here. https://codereview.chromium.org/2847853003/diff/200001/remoting/host/it2me/it... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2847853003/diff/200001/remoting/host/it2me/it... remoting/host/it2me/it2me_host_unittest.cc:204: it2me_host_->OnPolicyUpdate(PolicyWatcher::GetDefaultPolicies()); This is a nice change. https://codereview.chromium.org/2847853003/diff/200001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/2847853003/diff/200001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host.cc:491: if (!pending_connect_.is_null()) { nit: the style for this has changed, the preferred syntax is to treat the var as a bool: if (pending_connect_) { https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... File remoting/host/policy_watcher.cc (right): https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... remoting/host/policy_watcher.cc:162: // If old_policies is empty, then the PolicyService has not yet provided s/old_policies/|old_policies_| https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... remoting/host/policy_watcher.cc:163: // policies, so just return the default values. Otherwise, old_policies same here https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... remoting/host/policy_watcher.cc:164: // already contains all the supported polcies, defaults and overrides. s/polcies/policies
FYI https://codereview.chromium.org/2847853003/diff/200001/remoting/host/it2me/it... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/2847853003/diff/200001/remoting/host/it2me/it... remoting/host/it2me/it2me_host.h:96: // Called when initial policies are read, and when they change. On 2017/05/05 14:42:14, joedow wrote: > nit: comma isn't needed here. Done. https://codereview.chromium.org/2847853003/diff/200001/remoting/host/it2me/it... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/2847853003/diff/200001/remoting/host/it2me/it... remoting/host/it2me/it2me_native_messaging_host.cc:491: if (!pending_connect_.is_null()) { On 2017/05/05 14:42:15, joedow wrote: > nit: the style for this has changed, the preferred syntax is to treat the var as > a bool: > if (pending_connect_) { Done, here and above. https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... File remoting/host/policy_watcher.cc (right): https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... remoting/host/policy_watcher.cc:162: // If old_policies is empty, then the PolicyService has not yet provided On 2017/05/05 14:42:15, joedow wrote: > s/old_policies/|old_policies_| Done. https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... remoting/host/policy_watcher.cc:163: // policies, so just return the default values. Otherwise, old_policies On 2017/05/05 14:42:15, joedow wrote: > same here Done. https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... remoting/host/policy_watcher.cc:164: // already contains all the supported polcies, defaults and overrides. On 2017/05/05 14:42:15, joedow wrote: > s/polcies/policies Done.
The CQ bit was checked by jamiewalch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkjnsn@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2847853003/#ps220001 (title: "Reviewer feedback.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... File remoting/host/policy_watcher.cc (right): https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... remoting/host/policy_watcher.cc:162: // If old_policies is empty, then the PolicyService has not yet provided On 2017/05/05 16:49:06, Jamie wrote: > On 2017/05/05 14:42:15, joedow wrote: > > s/old_policies/|old_policies_| > > Done. This needs a trailing underscore: old_policies_ https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... remoting/host/policy_watcher.cc:163: // policies, so just return the default values. Otherwise, old_policies On 2017/05/05 16:49:06, Jamie wrote: > On 2017/05/05 14:42:15, joedow wrote: > > same here > > Done. Same here
The CQ bit was unchecked by jamiewalch@chromium.org
The CQ bit was checked by jamiewalch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkjnsn@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2847853003/#ps240001 (title: "Reviewer feedback.")
https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... File remoting/host/policy_watcher.cc (right): https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... remoting/host/policy_watcher.cc:162: // If old_policies is empty, then the PolicyService has not yet provided On 2017/05/05 16:53:04, rkjnsn wrote: > On 2017/05/05 16:49:06, Jamie wrote: > > On 2017/05/05 14:42:15, joedow wrote: > > > s/old_policies/|old_policies_| > > > > Done. > > This needs a trailing underscore: old_policies_ Done. https://codereview.chromium.org/2847853003/diff/200001/remoting/host/policy_w... remoting/host/policy_watcher.cc:163: // policies, so just return the default values. Otherwise, old_policies On 2017/05/05 16:53:04, rkjnsn wrote: > On 2017/05/05 16:49:06, Jamie wrote: > > On 2017/05/05 14:42:15, joedow wrote: > > > same here > > > > Done. > > Same here Done.
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Description was changed from ========== Remove policy watching from It2MeHost. This functionality is already present in It2MeNativeMessagingHost, which owns the It2MeHost and can simply forward the policy changes to it. This CL is part of a larger change to make the It2Me host react to policy errors, at which point it will be more convenient to have a single policy watcher. BUG=433009 ========== to ========== Remove policy watching from It2MeHost. This functionality is already present in It2MeNativeMessagingHost, which owns the It2MeHost and can simply forward the policy changes to it. This CL is part of a larger change to make the It2Me host react to policy errors, at which point it will be more convenient to have a single policy watcher. BUG=433009 ==========
jamiewalch@chromium.org changed reviewers: + asargent@chromium.org
jamiewalch@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - asargent@chromium.org
Devlin, ptal for chrome/browser/extensions/api/messaging/ OWNERS
extensions lgtm
The CQ bit was checked by jamiewalch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkjnsn@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2847853003/#ps280001 (title: "Fix DEPS")
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jamiewalch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkjnsn@chromium.org, rdevlin.cronin@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2847853003/#ps300001 (title: "Add dep.")
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": 300001, "attempt_start_ts": 1494019519515050,
"parent_rev": "02ea6568c0b73b6c9873ea2f3aef44d77dc81a25", "commit_rev":
"cf2cc68d4e6a4a70bcadfd5491f90d119129ec38"}
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1494019519515050,
"parent_rev": "02ea6568c0b73b6c9873ea2f3aef44d77dc81a25", "commit_rev":
"cf2cc68d4e6a4a70bcadfd5491f90d119129ec38"}
Message was sent while issue was closed.
Description was changed from ========== Remove policy watching from It2MeHost. This functionality is already present in It2MeNativeMessagingHost, which owns the It2MeHost and can simply forward the policy changes to it. This CL is part of a larger change to make the It2Me host react to policy errors, at which point it will be more convenient to have a single policy watcher. BUG=433009 ========== to ========== Remove policy watching from It2MeHost. This functionality is already present in It2MeNativeMessagingHost, which owns the It2MeHost and can simply forward the policy changes to it. This CL is part of a larger change to make the It2Me host react to policy errors, at which point it will be more convenient to have a single policy watcher. BUG=433009 Review-Url: https://codereview.chromium.org/2847853003 Cr-Commit-Position: refs/heads/master@{#469834} Committed: https://chromium.googlesource.com/chromium/src/+/cf2cc68d4e6a4a70bcadfd5491f9... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/cf2cc68d4e6a4a70bcadfd5491f9... |
