Initial RemoteCommandsInvalidator

chrome/browser/policy/cloud/remote_commands_invalidator_base.cc

+// 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.
+
+#include "chrome/browser/policy/cloud/remote_commands_invalidator_base.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/sequenced_task_runner.h"
+#include "components/invalidation/invalidation_service.h"
+#include "components/invalidation/invalidation_util.h"
+#include "components/invalidation/object_id_invalidation_map.h"
+#include "components/invalidation/single_object_invalidation_set.h"
+#include "components/policy/core/common/remote_commands/remote_commands_service.h"

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

Nit: Not used.

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

Done.

+
+namespace policy {
+
+RemoteCommandsInvalidatorBase::RemoteCommandsInvalidatorBase(
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner)
+    : state_(SHUT_DOWN), task_runner_(task_runner), weak_factory_(this) {

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

Nit: Why not initialize |state_| on declaration, like you do with all the other
POD members?

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

Done.

+  DCHECK(task_runner.get());

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

Nit: s/.get()//

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

Done.

+}
+
+RemoteCommandsInvalidatorBase::~RemoteCommandsInvalidatorBase() {
+  DCHECK_EQ(SHUT_DOWN, state_);
+}
+
+void RemoteCommandsInvalidatorBase::Initialize(
+    invalidation::InvalidationService* invalidation_service) {
+  DCHECK_EQ(SHUT_DOWN, state_);
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  DCHECK(invalidation_service);
+  invalidation_service_ = invalidation_service;
+
+  state_ = STOPPED;
+  OnInitialize();
+}
+
+void RemoteCommandsInvalidatorBase::Shutdown() {
+  DCHECK_NE(SHUT_DOWN, state_);
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (state_ == STARTED)
+    Stop();

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

Since Stop() handles |state_ != STOPPED| gracefully, you could just
unconditionally call it here.

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

Done.

+
+  state_ = SHUT_DOWN;
+  weak_factory_.InvalidateWeakPtrs();
+  OnShutdown();
+}
+
+void RemoteCommandsInvalidatorBase::Start() {
+  DCHECK_EQ(STOPPED, state_);
+  DCHECK(thread_checker_.CalledOnValidThread());
+  state_ = STARTED;
+
+  OnStart();
+}
+
+void RemoteCommandsInvalidatorBase::Stop() {
+  DCHECK_NE(SHUT_DOWN, state_);
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (state_ == STARTED) {
+    Unregister();
+    state_ = STOPPED;
+
+    OnStop();
+  }
+}
+
+void RemoteCommandsInvalidatorBase::OnInvalidatorStateChange(
+    syncer::InvalidatorState state) {
+  DCHECK_EQ(STARTED, state_);
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  invalidation_service_enabled_ = state == syncer::INVALIDATIONS_ENABLED;

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

Nit: #include "components/invalidation/invalidator_state.h"

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

Done.

+  UpdateInvalidationsEnabled();
+}
+
+void RemoteCommandsInvalidatorBase::OnIncomingInvalidation(
+    const syncer::ObjectIdInvalidationMap& invalidation_map) {
+  DCHECK_EQ(STARTED, state_);
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (!invalidation_service_enabled_)
+    LOG(WARNING) << "Unexpected invalidation received.";
+
+  const syncer::SingleObjectInvalidationSet& list =
+      invalidation_map.ForObject(object_id_);
+  if (list.IsEmpty()) {
+    NOTREACHED();
+    return;
+  }
+
+  // Acknowledge all invalidations.
+  for (const auto& it : list)
+    it.Acknowledge();

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

Nit: #include "components/invalidation/invalidation.h"

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

Done.

+
+  task_runner_->PostTask(

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

Why can we not run this synchronously?

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

It's mainly due to the messageloopproxy used in unit tests. Also maybe there are
need to use PostDelayedTask later.

+      FROM_HERE,
+      base::Bind(&RemoteCommandsInvalidatorBase::DoRemoteCommandsFetch,
+                 weak_factory_.GetWeakPtr()));
+}
+
+std::string RemoteCommandsInvalidatorBase::GetOwnerName() const {
+  return "RemoteCommands";

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

Nit: #include <string>

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

Done.

+}
+
+void RemoteCommandsInvalidatorBase::ReloadPolicyData(
+    const enterprise_management::PolicyData* policy) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (state_ != STARTED)
+    return;
+
+  // Create the ObjectId based on the policy data.
+  // If the policy does not specify an the ObjectId, then unregister.
+  if (!policy || !policy->has_command_invalidation_source() ||
+      !policy->has_command_invalidation_name()) {
+    Unregister();
+    return;
+  }
+  invalidation::ObjectId object_id(policy->command_invalidation_source(),

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

Nit: const.

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

Done.

+                                   policy->command_invalidation_name());
+
+  // If the policy object id in the policy data is different from the currently
+  // registered object id, update the object registration.
+  if (!is_registered_ || !(object_id == object_id_))

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

Nit: Is there no operator != for object IDs?

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

Yes, it's not implemented.

+    Register(object_id);
+}
+
+void RemoteCommandsInvalidatorBase::Register(
+    const invalidation::ObjectId& object_id) {
+  // Register this handler with the invalidation service if needed.
+  if (!is_registered_) {
+    OnInvalidatorStateChange(invalidation_service_->GetInvalidatorState());
+    invalidation_service_->RegisterInvalidationHandler(this);
+  }
+
+  is_registered_ = true;

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

Nit: Move this inside the conditional above.

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

Done.

+  object_id_ = object_id;
+  UpdateInvalidationsEnabled();
+
+  // Update registration with the invalidation service.
+  syncer::ObjectIdSet ids;
+  ids.insert(object_id);
+  invalidation_service_->UpdateRegisteredInvalidationIds(this, ids);
+}
+
+void RemoteCommandsInvalidatorBase::Unregister() {
+  if (is_registered_) {
+    invalidation_service_->UpdateRegisteredInvalidationIds(
+        this, syncer::ObjectIdSet());
+    invalidation_service_->UnregisterInvalidationHandler(this);
+    is_registered_ = false;
+    UpdateInvalidationsEnabled();
+  }
+}
+
+void RemoteCommandsInvalidatorBase::UpdateInvalidationsEnabled() {
+  invalidations_enabled_ = invalidation_service_enabled_ && is_registered_;
+}
+
+}  // namespace policy