Merged PolicyServiceWatcher into PolicyWatcher.

remoting/host/policy_hack/policy_watcher.cc

Index: remoting/host/policy_hack/policy_watcher.cc
diff --git a/remoting/host/policy_hack/policy_watcher.cc b/remoting/host/policy_hack/policy_watcher.cc
index 6d93ff9d2439d7c30329a1f32cea1d702d982a65..844e1a09bcef94b350e49ffd0c9367aecb709aff 100644
--- a/remoting/host/policy_hack/policy_watcher.cc
+++ b/remoting/host/policy_hack/policy_watcher.cc
@@ -9,10 +9,16 @@
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
+#include "base/files/file_path.h"
 #include "base/location.h"
-#include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/values.h"
+#include "components/policy/core/common/async_policy_loader.h"
+#include "components/policy/core/common/async_policy_provider.h"
+#include "components/policy/core/common/policy_namespace.h"
+#include "components/policy/core/common/policy_service_impl.h"
+#include "components/policy/core/common/schema.h"
+#include "components/policy/core/common/schema_registry.h"
 #include "policy/policy_constants.h"
 #include "remoting/host/dns_blackhole_checker.h"
 
@@ -20,6 +26,19 @@
 #include "base/json/json_reader.h"
 #endif
 
+#if defined(OS_CHROMEOS)
+#include "content/public/browser/browser_thread.h"
+#elif defined(OS_WIN)
+#include "components/policy/core/common/policy_loader_win.h"
+#elif defined(OS_MACOSX)
+#include "components/policy/core/common/policy_loader_mac.h"
+#include "components/policy/core/common/preferences_mac.h"
+#elif defined(OS_POSIX) && !defined(OS_ANDROID)
+#include "components/policy/core/common/config_dir_policy_loader.h"
+#endif
+
+using namespace policy;

[Sergey Ulanov, 2015/01/26 18:20:01]

style guide prohibits "using namespace":
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces

But you can replace it with "using policy::key;". 

[Łukasz Anforowicz, 2015/01/26 19:24:20]

Oops.  Fixed.  FWIW, I was just retaining it from policy_watcher_chromeos.cc.

[Sergey Ulanov, 2015/01/28 07:07:53]

If I remember correctly the style guide did allow it in the past.  Don't know
when that section was changed.

+
 namespace remoting {
 namespace policy_hack {
 
@@ -54,7 +73,7 @@ scoped_ptr<base::DictionaryValue> CopyGoodValuesAndAddDefaults(
 #if !defined(NDEBUG)
   // Replace values with those specified in DebugOverridePolicies, if present.
   std::string policy_overrides;
-  if (from->GetString(policy::key::kRemoteAccessHostDebugOverridePolicies,
+  if (from->GetString(key::kRemoteAccessHostDebugOverridePolicies,
                       &policy_overrides)) {
     scoped_ptr<base::Value> value(base::JSONReader::Read(policy_overrides));
     const base::DictionaryValue* override_values;
@@ -67,68 +86,57 @@ scoped_ptr<base::DictionaryValue> CopyGoodValuesAndAddDefaults(
   return to.Pass();
 }
 
+PolicyNamespace GetPolicyNamespace() {
+  return PolicyNamespace(POLICY_DOMAIN_CHROME, std::string());
+}
+

[Łukasz Anforowicz, 2015/01/26 17:45:11]

GetPolicyNamespace is copy&pasted from policy_service_watcher.cc

 }  // namespace
 
-PolicyWatcher::PolicyWatcher(
-    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner)
-    : task_runner_(task_runner),
+PolicyWatcher::PolicyWatcher(const scoped_refptr<base::SingleThreadTaskRunner>&

[Sergey Ulanov, 2015/01/26 18:20:01]

nit: Move this constructructor next to the other constructors below?

[Łukasz Anforowicz, 2015/01/26 19:24:20]

Done.

+                                 policy_service_task_runner)
+    : policy_service_task_runner_(policy_service_task_runner),
       transient_policy_error_retry_counter_(0),
       old_policies_(new base::DictionaryValue()),
-      default_values_(new base::DictionaryValue()),
-      weak_factory_(this) {
+      default_values_(new base::DictionaryValue()) {
   // Initialize the default values for each policy.
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostFirewallTraversal,
-                              true);
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostRequireTwoFactor,
-                              false);
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostRequireCurtain,
-                              false);
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostMatchUsername,
-                              false);
-  default_values_->SetString(policy::key::kRemoteAccessHostDomain,
-                             std::string());
-  default_values_->SetString(policy::key::kRemoteAccessHostTalkGadgetPrefix,
+  default_values_->SetBoolean(key::kRemoteAccessHostFirewallTraversal, true);
+  default_values_->SetBoolean(key::kRemoteAccessHostRequireTwoFactor, false);
+  default_values_->SetBoolean(key::kRemoteAccessHostRequireCurtain, false);
+  default_values_->SetBoolean(key::kRemoteAccessHostMatchUsername, false);
+  default_values_->SetString(key::kRemoteAccessHostDomain, std::string());
+  default_values_->SetString(key::kRemoteAccessHostTalkGadgetPrefix,
                              kDefaultHostTalkGadgetPrefix);
-  default_values_->SetString(policy::key::kRemoteAccessHostTokenUrl,
-                             std::string());
-  default_values_->SetString(policy::key::kRemoteAccessHostTokenValidationUrl,
+  default_values_->SetString(key::kRemoteAccessHostTokenUrl, std::string());
+  default_values_->SetString(key::kRemoteAccessHostTokenValidationUrl,
                              std::string());
   default_values_->SetString(
-      policy::key::kRemoteAccessHostTokenValidationCertificateIssuer,
-      std::string());
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostAllowClientPairing,
+      key::kRemoteAccessHostTokenValidationCertificateIssuer, std::string());
+  default_values_->SetBoolean(key::kRemoteAccessHostAllowClientPairing, true);
+  default_values_->SetBoolean(key::kRemoteAccessHostAllowGnubbyAuth, true);
+  default_values_->SetBoolean(key::kRemoteAccessHostAllowRelayedConnection,
                               true);
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostAllowGnubbyAuth,
-                              true);
-  default_values_->SetBoolean(
-      policy::key::kRemoteAccessHostAllowRelayedConnection, true);
-  default_values_->SetString(policy::key::kRemoteAccessHostUdpPortRange, "");
+  default_values_->SetString(key::kRemoteAccessHostUdpPortRange, "");
 #if !defined(NDEBUG)
-  default_values_->SetString(
-      policy::key::kRemoteAccessHostDebugOverridePolicies, std::string());
+  default_values_->SetString(key::kRemoteAccessHostDebugOverridePolicies,
+                             std::string());
 #endif
 
   // Initialize the fall-back values to use for unreadable policies.
   // For most policies these match the defaults.
   bad_type_values_.reset(default_values_->DeepCopy());
-  bad_type_values_->SetBoolean(policy::key::kRemoteAccessHostFirewallTraversal,
+  bad_type_values_->SetBoolean(key::kRemoteAccessHostFirewallTraversal, false);
+  bad_type_values_->SetBoolean(key::kRemoteAccessHostAllowRelayedConnection,
                                false);
-  bad_type_values_->SetBoolean(
-      policy::key::kRemoteAccessHostAllowRelayedConnection, false);
-}
-
-PolicyWatcher::~PolicyWatcher() {
 }
 
 void PolicyWatcher::StartWatching(
     const PolicyUpdatedCallback& policy_updated_callback,
     const PolicyErrorCallback& policy_error_callback) {
   if (!OnPolicyWatcherThread()) {
-    task_runner_->PostTask(FROM_HERE,
-                           base::Bind(&PolicyWatcher::StartWatching,
-                                      base::Unretained(this),
-                                      policy_updated_callback,
-                                      policy_error_callback));
+    policy_service_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&PolicyWatcher::StartWatching, base::Unretained(this),
+                   policy_updated_callback, policy_error_callback));
     return;
   }
 
@@ -138,7 +146,7 @@ void PolicyWatcher::StartWatching(
 }
 
 void PolicyWatcher::StopWatching(const base::Closure& stopped_callback) {
-  task_runner_->PostTaskAndReply(
+  policy_service_task_runner_->PostTaskAndReply(
       FROM_HERE, base::Bind(&PolicyWatcher::StopWatchingOnPolicyWatcherThread,
                             base::Unretained(this)),
       stopped_callback);
@@ -146,17 +154,12 @@ void PolicyWatcher::StopWatching(const base::Closure& stopped_callback) {
 
 void PolicyWatcher::StopWatchingOnPolicyWatcherThread() {
   StopWatchingInternal();
-  weak_factory_.InvalidateWeakPtrs();
   policy_updated_callback_.Reset();
   policy_error_callback_.Reset();
 }
 
-const base::DictionaryValue& PolicyWatcher::Defaults() const {
-  return *default_values_;
-}
-
 bool PolicyWatcher::OnPolicyWatcherThread() const {
-  return task_runner_->BelongsToCurrentThread();
+  return policy_service_task_runner_->BelongsToCurrentThread();
 }
 
 void PolicyWatcher::UpdatePolicies(
@@ -205,5 +208,127 @@ void PolicyWatcher::SignalTransientPolicyError() {
   }
 }
 
+PolicyWatcher::PolicyWatcher(const scoped_refptr<base::SingleThreadTaskRunner>&
+                                 policy_service_task_runner,
+                             PolicyService* policy_service)
+    : PolicyWatcher(policy_service_task_runner) {
+  policy_service_ = policy_service;
+}
+
+PolicyWatcher::PolicyWatcher(

[Sergey Ulanov, 2015/01/26 18:20:01]

This class has 3 private constructors. Consider replacing them with a single
constructor that takes all the arguments and pass nullptr for some of them. Even
if you prefer to keep multiple constructors you don't need all 3. This
constructor can call the one above, so you can merge the first two constructors.

[Łukasz Anforowicz, 2015/01/26 19:24:20]

I merged into a single constructor.

+    const scoped_refptr<base::SingleThreadTaskRunner>&
+        policy_service_task_runner,
+    scoped_ptr<PolicyService> owned_policy_service,
+    scoped_ptr<ConfigurationPolicyProvider> owned_policy_provider,
+    scoped_ptr<SchemaRegistry> owned_schema_registry)
+    : PolicyWatcher(policy_service_task_runner) {
+  owned_schema_registry_ = owned_schema_registry.Pass();
+  owned_policy_provider_ = owned_policy_provider.Pass();
+  owned_policy_service_ = owned_policy_service.Pass();
+  policy_service_ = owned_policy_service_.get();
+}

[Łukasz Anforowicz, 2015/01/26 17:45:11]

Everything below is copy&pasted from policy_service_watcher.cc except:
1. s/PolicyServiceWatcher/PolicyWatcher/g
2. minor changes to the form used by constructors (in order to reuse a "base"
constructor, I had to move field initialization of owner_policy_provider [etc]
into field assignment in the body of the constructor)

+
+PolicyWatcher::~PolicyWatcher() {
+  if (owned_policy_provider_) {
+    owned_policy_provider_->Shutdown();
+  }
+}
+
+void PolicyWatcher::OnPolicyUpdated(const PolicyNamespace& ns,
+                                    const PolicyMap& previous,
+                                    const PolicyMap& current) {
+  scoped_ptr<base::DictionaryValue> policy_dict(new base::DictionaryValue());
+  for (PolicyMap::const_iterator it = current.begin(); it != current.end();
+       it++) {

[Sergey Ulanov, 2015/01/26 18:20:01]

s/it++/++it/
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preincrement_...

[Łukasz Anforowicz, 2015/01/26 19:24:20]

Done.

I also looked into whether I can use "auto" here, but there is no way to
explicitly say that I want a "const" iterator, so I decided not to do it. 
("auto" would still give a "const" iterator because "current" is "const", but
this seems like too much magic the reader of the code has to be aware of... :-/
).

[Sergey Ulanov, 2015/01/28 07:07:53]

I don't think it's essential that the iterator is const here, so if auto works
here I think it would make this code more readable. Up to you.

+    // TODO(lukasza): Use policy::Schema::Normalize() for schema verification.
+    policy_dict->Set(it->first, it->second.value->DeepCopy());
+  }
+  UpdatePolicies(policy_dict.get());
+}
+
+void PolicyWatcher::OnPolicyServiceInitialized(PolicyDomain domain) {
+  PolicyNamespace ns = GetPolicyNamespace();
+  const PolicyMap& current = policy_service_->GetPolicies(ns);
+  OnPolicyUpdated(ns, current, current);
+}
+
+void PolicyWatcher::StartWatchingInternal() {
+  // Listen for future policy changes.
+  policy_service_->AddObserver(POLICY_DOMAIN_CHROME, this);
+
+  // Process current policy state.
+  if (policy_service_->IsInitializationComplete(POLICY_DOMAIN_CHROME)) {
+    OnPolicyServiceInitialized(POLICY_DOMAIN_CHROME);
+  }
+}
+
+void PolicyWatcher::StopWatchingInternal() {
+  policy_service_->RemoveObserver(POLICY_DOMAIN_CHROME, this);
+}
+
+scoped_ptr<PolicyWatcher> PolicyWatcher::CreateFromPolicyLoader(
+    const scoped_refptr<base::SingleThreadTaskRunner>&
+        policy_service_task_runner,
+    scoped_ptr<AsyncPolicyLoader> async_policy_loader) {
+  // TODO(lukasza): Schema below should ideally only cover Chromoting-specific
+  // policies (expecting perf and maintanability improvement, but no functional
+  // impact).
+  Schema schema = Schema::Wrap(GetChromeSchemaData());
+
+  scoped_ptr<SchemaRegistry> schema_registry(new SchemaRegistry());
+  schema_registry->RegisterComponent(GetPolicyNamespace(), schema);
+
+  scoped_ptr<AsyncPolicyProvider> policy_provider(new AsyncPolicyProvider(
+      schema_registry.get(), async_policy_loader.Pass()));
+  policy_provider->Init(schema_registry.get());
+
+  PolicyServiceImpl::Providers providers;
+  providers.push_back(policy_provider.get());
+  scoped_ptr<PolicyService> policy_service(new PolicyServiceImpl(providers));
+
+  return make_scoped_ptr(
+      new PolicyWatcher(policy_service_task_runner, policy_service.Pass(),
+                        policy_provider.Pass(), schema_registry.Pass()));
+}
+
+scoped_ptr<PolicyWatcher> PolicyWatcher::Create(
+    policy::PolicyService* policy_service,
+    const scoped_refptr<base::SingleThreadTaskRunner>& network_task_runner) {
+#if defined(OS_CHROMEOS)
+  DCHECK(policy_service);
+  return make_scoped_ptr(
+      new PolicyWatcher(content::BrowserThread::GetMessageLoopProxyForThread(
+                            content::BrowserThread::UI),
+                        policy_service));
+#elif defined(OS_WIN)
+  DCHECK(!policy_service);
+  static const wchar_t kRegistryKey[] = L"SOFTWARE\\Policies\\Google\\Chrome";
+  return PolicyWatcher::CreateFromPolicyLoader(
+      network_task_runner,
+      PolicyLoaderWin::Create(network_task_runner, kRegistryKey));
+#elif defined(OS_MACOSX)
+  CFStringRef bundle_id = CFSTR("com.google.Chrome");
+  DCHECK(!policy_service);
+  return PolicyWatcher::CreateFromPolicyLoader(
+      network_task_runner,
+      make_scoped_ptr(new PolicyLoaderMac(
+          network_task_runner,
+          policy::PolicyLoaderMac::GetManagedPolicyPath(bundle_id),
+          new MacPreferences(), bundle_id)));
+#elif defined(OS_POSIX) && !defined(OS_ANDROID)
+  DCHECK(!policy_service);
+  // Always read the Chrome policies (even on Chromium) so that policy

[Sergey Ulanov, 2015/01/26 18:20:01]

This comment applies to Win and OSX too. Move it out of the ifdef?

[Łukasz Anforowicz, 2015/01/26 19:24:20]

I moved it slightly higher + added a note that the comment applies to all 3
cases.  Note that this behavior is different from behavior of the browser and
I'll probably have to follow-up on this later on (i.e. Mattias was surprised by
this behavior here).

+  // enforcement can't be bypassed by running Chromium.
+  static const base::FilePath::CharType kPolicyDir[] =
+      FILE_PATH_LITERAL("/etc/opt/chrome/policies");
+  return PolicyWatcher::CreateFromPolicyLoader(
+      network_task_runner, make_scoped_ptr(new ConfigDirPolicyLoader(
+                               network_task_runner, base::FilePath(kPolicyDir),
+                               POLICY_SCOPE_MACHINE)));
+#else
+#error OS that is not yet supported by PolicyWatcher code.
+#endif
+}
+
 }  // namespace policy_hack
 }  // namespace remoting