Initial RemoteCommandsInvalidator

chrome/browser/policy/cloud/remote_commands_invalidator_base.h

+// Copyright 2015 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.
+
+#ifndef CHROME_BROWSER_POLICY_CLOUD_REMOTE_COMMANDS_INVALIDATOR_BASE_H_
+#define CHROME_BROWSER_POLICY_CLOUD_REMOTE_COMMANDS_INVALIDATOR_BASE_H_
+
+#include <string>

[bartfab (slow), 2015/05/15 13:28:08]

Nit: Not used (only used in overriden method declaration and for that, you
should not include it).

[binjin, 2015/05/15 14:50:39]

Done.

+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
+#include "components/invalidation/invalidation.h"

[bartfab (slow), 2015/05/15 13:28:08]

Nit: Not used.

[binjin, 2015/05/15 14:50:39]

Done.

+#include "components/invalidation/invalidation_handler.h"
+#include "policy/proto/device_management_backend.pb.h"
+
+namespace base {
+class SequencedTaskRunner;
+}  // namespace base
+
+namespace invalidation {
+class InvalidationService;
+}  // namespace invalidation
+
+namespace policy {
+
+class CloudPolicyCore;

[bartfab (slow), 2015/05/15 13:28:08]

Nit: Not used.

[binjin, 2015/05/15 14:50:39]

Done.

+
+// XXX

[bartfab (slow), 2015/05/15 13:28:08]

I presume you will add comments before this lands :).

[binjin, 2015/05/15 14:50:39]

Done.

+class RemoteCommandsInvalidatorBase : public syncer::InvalidationHandler {
+ public:
+  // XXX
+  RemoteCommandsInvalidatorBase(

[bartfab (slow), 2015/05/15 13:28:08]

Nit: explicit.

[binjin, 2015/05/15 14:50:39]

Done.

+      const scoped_refptr<base::SequencedTaskRunner>& task_runner);

[bartfab (slow), 2015/05/15 13:28:08]

What is this |task_runner| used for? If it is used for running background tasks
on another thread, rename it to |background_task_runner| instead. If it is meant
for posting asynchronous tasks on the current thread, why not just grab it from
ThreadTaskRunnerHandle::Get() instead of passing it in?

[binjin, 2015/05/15 14:50:39]

It's used to post |DoRemoteCommandsFetch|, and subclasses might want to use a
different
task runner. For example, the MockRemoteCommandsInvalidator actually had to post
tasks to
MessageLoopProxy, due to limitation of FakeInvalidationService.

[bartfab (slow), 2015/05/15 15:37:12]

The MessageLoopProxy you are using in that class is also accessible via
ThreadTaskRunnerHandle::Get(). By definition, the current thread's task runner
will *always* be accessible via ThreadTaskRunnerHandle::Get(). I see no reason
to pass it as a dependency.

+  ~RemoteCommandsInvalidatorBase() override;
+
+  // XXX
+  void Initialize(invalidation::InvalidationService* invalidation_service);
+
+  // XXX
+  void Shutdown();
+
+  // XXX
+  void Start();
+
+  // XXX
+  void Stop();
+
+  // Helpful accessors.
+  invalidation::InvalidationService* invalidation_service() {
+    return invalidation_service_;
+  }
+  bool invalidations_enabled() { return invalidations_enabled_; }
+
+  // syncer::InvalidationHandler:
+  void OnInvalidatorStateChange(syncer::InvalidatorState state) override;
+  void OnIncomingInvalidation(
+      const syncer::ObjectIdInvalidationMap& invalidation_map) override;
+  std::string GetOwnerName() const override;
+
+ protected:
+  virtual void OnInitialize() {}

[bartfab (slow), 2015/05/15 13:28:08]

Nit 1: The style guide forbids inlining virtual methods.
Nit 2: Why do you provide empty default implementations for these four methods
if every sub-class actually overrides them?

[binjin, 2015/05/15 14:50:39]

Done.

+  virtual void OnShutdown() {}
+  virtual void OnStart() {}
+  virtual void OnStop() {}
+  virtual void DoRemoteCommandsFetch() = 0;
+
+  // XXX
+  void ReloadPolicyData(const enterprise_management::PolicyData* policy);
+
+ private:
+  // Registers the given object with the invalidation service.
+  void Register(const invalidation::ObjectId& object_id);

[bartfab (slow), 2015/05/15 13:28:08]

Nit: #include "google/cacheinvalidation/include/types.h"

[binjin, 2015/05/15 14:50:39]

Done.

+
+  // Unregisters the current object with the invalidation service.
+  void Unregister();
+
+  // Updates invalidations_enabled_ and calls the invalidation handler if the

[bartfab (slow), 2015/05/15 13:28:08]

Which handler are you talking about here? I cannot see any handlers other than
|this|.

[binjin, 2015/05/15 14:50:39]

Done.

+  // value changed.
+  void UpdateInvalidationsEnabled();
+
+  // The state of the object.
+  enum State {
+    SHUT_DOWN,
+    STOPPED,
+    STARTED,
+  };
+  State state_;
+
+  // Schedules delayed tasks.
+  const scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
+  // The invalidation service.
+  invalidation::InvalidationService* invalidation_service_ = nullptr;
+
+  // Whether the invalidator currently has the ability to receive invalidations.
+  // This is true if the invalidation service is enabled and the invalidator
+  // has registered for a remote commands object.
+  bool invalidations_enabled_ = false;
+
+  // Whether the invalidation service is currently enabled.
+  bool invalidation_service_enabled_ = false;
+
+  // Whether this object has registered for remote commands invalidations.
+  bool is_registered_ = false;
+
+  // The object id representing the remote commands in the invalidation service.
+  invalidation::ObjectId object_id_;
+
+  // A thread checker to make sure that callbacks are invoked on the correct
+  // thread.
+  base::ThreadChecker thread_checker_;
+
+  // WeakPtrFactory used to create callbacks to this object.
+  base::WeakPtrFactory<RemoteCommandsInvalidatorBase> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(RemoteCommandsInvalidatorBase);
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_POLICY_CLOUD_REMOTE_COMMANDS_INVALIDATOR_BASE_H_