Remote assistance on Chrome OS Part IV - It2MeHost

remoting/host/policy_hack/policy_watcher_chromeos.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/policy_hack/policy_watcher.h"
+
+#include "components/policy/core/common/policy_service.h"
+#include "remoting/base/auto_thread_task_runner.h"
+
+using namespace policy;
+
+namespace remoting {
+namespace policy_hack {
+
+namespace {
+
+class PolicyWatcherChromeOS : public PolicyWatcher,
+                              public PolicyService::Observer {
+ public:
+  PolicyWatcherChromeOS(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                        PolicyService* policy_service);
+
+  virtual ~PolicyWatcherChromeOS() {}

[Wez, 2014/10/17 17:58:01]

Don't inline virtuals, even destructors (except in pure interface classes :)

[kelvinp, 2014/10/20 00:21:18]

Done.

+
+  // PolicyService::Observer implementation.

[Wez, 2014/10/17 17:58:01]

s/implementation/interface

[kelvinp, 2014/10/20 00:21:18]

Seems like sergey prefers implementation and you prefer interface :)

+  virtual void OnPolicyUpdated(const PolicyNamespace& ns,
+                               const PolicyMap& previous,
+                               const PolicyMap& current) OVERRIDE;
+
+ protected:
+  // PolicyWatcher overrides.

[Wez, 2014/10/17 17:58:01]

s/overrides/interface

[kelvinp, 2014/10/20 00:21:18]

Done.

+  virtual void Reload() OVERRIDE;
+  virtual void StartWatchingInternal() OVERRIDE;
+  virtual void StopWatchingInternal() OVERRIDE;
+
+ private:
+  PolicyService* policy_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(PolicyWatcherChromeOS);
+};
+
+PolicyWatcherChromeOS::PolicyWatcherChromeOS(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    PolicyService* policy_service)

[Wez, 2014/10/17 17:58:01]

What's the lifetime guarantee/requirement on policy service?

[kelvinp, 2014/10/20 00:21:18]

Good point.  Comments added.

+    : PolicyWatcher(task_runner), policy_service_(policy_service) {
+  DCHECK(policy_service_);
+}
+
+void PolicyWatcherChromeOS::OnPolicyUpdated(const PolicyNamespace& ns,
+                                            const PolicyMap& previous,
+                                            const PolicyMap& current) {
+  Reload();
+}
+
+void PolicyWatcherChromeOS::Reload(){

[Wez, 2014/10/17 17:58:01]

nit: Space between () and {

[kelvinp, 2014/10/20 00:21:18]

Done.

+  const PolicyMap& map = policy_service_->GetPolicies(
+      PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()));

[Wez, 2014/10/17 17:58:01]

This is redundant in the OnPolicyUpdated case, since it passes you the namespace
and the previous & current maps; you only need this Reload() logic at
start-of-day so can you move this logic into OnPolicyUpdated and then have
Reload() call that with these paremeters in place?

[kelvinp, 2014/10/20 00:21:18]

Done.

+
+  scoped_ptr<base::DictionaryValue> policy_dict(new base::DictionaryValue());
+
+  for (PolicyMap::const_iterator it = map.begin(); it != map.end(); it++) {
+    policy_dict->Set(it->first, it->second.value->DeepCopy());
+  }
+
+  UpdatePolicies(policy_dict.get());
+};
+
+void PolicyWatcherChromeOS::StartWatchingInternal() {
+  policy_service_->AddObserver(POLICY_DOMAIN_CHROME, this);
+  Reload();
+};
+
+void PolicyWatcherChromeOS::StopWatchingInternal(){
+  policy_service_->RemoveObserver(POLICY_DOMAIN_CHROME, this);
+};
+
+}  // namespace
+
+PolicyWatcher* PolicyWatcher::Create(
+    ChromotingHostContext* context,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+  // The PolicyWatcher on ChromeOS accesses the PolicyService object, which must
+  // be called on the UI thread of the browser process.
+  return new PolicyWatcherChromeOS(context->ui_task_runner(),
+                                   context->policy_service());
+}
+
+}  // namespace policy_hack
+}  // namespace remoting