Global Resource Coordinator: Basic service internals

services/resource_coordinator/coordination_unit/coordination_unit_impl.cc

+// Copyright 2017 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 "services/resource_coordinator/coordination_unit/coordination_unit_impl.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/logging.h"
+#include "base/process/process_handle.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/unguessable_token.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "services/resource_coordinator/public/cpp/coordination_unit_id.h"
+
+namespace resource_coordinator {
+
+namespace {
+
+using CUIDMap = std::unordered_map<CoordinationUnitID, CoordinationUnitImpl*>;
+
+CUIDMap& g_cu_map() {
+  static CUIDMap* instance = new CUIDMap();
+  return *instance;
+}
+
+}  // namespace
+
+CoordinationUnitImpl::CoordinationUnitImpl(
+    const CoordinationUnitID& id,
+    std::unique_ptr<service_manager::ServiceContextRef> service_ref) {
+  if (!id.id) {
+    id_ = CoordinationUnitID(id.type,
+                             base::UnguessableToken().Create().ToString());
+  } else {
+    id_ = id;
+  }
+
+  auto it = g_cu_map().insert(std::make_pair(id_, this));
+  DCHECK(it.second);  // Inserted successfully
+
+  service_ref_ = std::move(service_ref);
+}
+
+CoordinationUnitImpl::~CoordinationUnitImpl() {
+  g_cu_map().erase(id_);
+
+  for (CoordinationUnitImpl* child : children_) {
+    child->RemoveParent(this);
+  }
+
+  for (CoordinationUnitImpl* parent : parents_) {
+    parent->RemoveChild(this);
+  }
+}
+
+bool CoordinationUnitImpl::SelfOrParentHasFlagSet(StateFlags state) {
+  const base::Optional<bool>& state_flag = state_flags_[state];
+  if (state_flag && *state_flag)
+    return true;
+
+  for (CoordinationUnitImpl* parent : parents_) {
+    if (parent->SelfOrParentHasFlagSet(state))
+      return true;
+  }
+
+  return false;
+}
+
+void CoordinationUnitImpl::RecalcPolicy() {
+  for (CoordinationUnitImpl* child : children_) {
+    child->RecalcPolicy();
+  }
+
+  if (!policy_callback_) {
+    return;
+  }
+
+  bool background_priority = !SelfOrParentHasFlagSet(kTabVisible) &&
+                             !SelfOrParentHasFlagSet(kAudioPlaying);
+
+  // Send the priority to the client if it's new or changed.
+  if (!current_policy_)

[nasko, 2017/04/24 22:10:50]

Be consistent about omitting {} for one line if statements. The policy_callback_
check above has them, which will be more readable without them and they are
missing here, where there is an else if clause. I think here they are needed,
regardless of the decision on the check above.

[oystein (OOO til 10th of July), 2017/04/25 17:19:17]

Done.

+    current_policy_ = mojom::Policy::New();

[nasko, 2017/04/24 22:10:51]

Maybe add a bit more scope to the name of the Policy object, as this now reads
as a very generic Mojo policy object, which it isn't.

[oystein (OOO til 10th of July), 2017/04/25 17:19:16]

Done; renamed to CoordinationPolicy.

+  else if ((current_policy_->use_background_priority == background_priority) &&
+           !SelfOrParentHasFlagSet(StateFlags::kTestState)) {
+    return;
+  }
+
+  mojom::PolicyPtr policy = mojom::Policy::New();
+  policy->use_background_priority = background_priority;
+  current_policy_->use_background_priority = background_priority;
+
+  policy_callback_->SetPolicy(std::move(policy));

[nasko, 2017/04/24 22:10:51]

Is policy here supposed to be a copy of the current_policy_ that we just supply
to the callback? Might be worthwhile a comment to clarify.

[oystein (OOO til 10th of July), 2017/04/25 17:19:16]

Done.

+}
+
+void CoordinationUnitImpl::SendEvent(mojom::EventPtr event) {
+  switch (event->type) {
+    case mojom::EventType::kOnWebContentsShown:
+      state_flags_[kTabVisible] = true;
+      break;
+    case mojom::EventType::kOnWebContentsHidden:
+      state_flags_[kTabVisible] = false;
+      break;
+    case mojom::EventType::kOnProcessAudioStarted:
+      state_flags_[kAudioPlaying] = true;
+      break;
+    case mojom::EventType::kOnProcessAudioStopped:
+      state_flags_[kAudioPlaying] = false;
+      break;
+    case mojom::EventType::kTestEvent:
+      state_flags_[kTestState] = true;
+      break;
+    default:
+      return;
+  }
+
+  RecalcPolicy();
+}
+
+void CoordinationUnitImpl::GetID(const GetIDCallback& callback) {
+  callback.Run(id_);
+}
+
+void CoordinationUnitImpl::Duplicate(mojom::CoordinationUnitRequest request) {
+  bindings_.AddBinding(this, std::move(request));
+}
+
+void CoordinationUnitImpl::AddChild(const CoordinationUnitID& child_id) {

[nasko, 2017/04/24 22:10:51]

Some comment on what the expected behavior of this method is will be useful. It
looks like if the child_id supplied has an existing CoordinationUnit for it, but
it isn't in the immediate tree reachable from this unit, then it is added.
However, it could still be part of another tree, is this intended?

[oystein (OOO til 10th of July), 2017/04/25 17:19:16]

Added a comment to the .mojom here; the child_id can't be an ascendant or
descendant to avoid infinite recursion when recalculating policies etc. In
theory it could be also inserted in another part of the DAG; I'm honestly not
sure if this is something we want/need to prevent, or if there's use-cases where
that's useful. Happy to make any changes here that makes us more future-safe.

+  if (child_id == id_)
+    return;
+
+  auto child_iter = g_cu_map().find(child_id);
+  if (child_iter != g_cu_map().end()) {
+    CoordinationUnitImpl* child = child_iter->second;
+    if (HasParent(child) || HasChild(child))
+      return;
+
+    DCHECK(child->id_ == child_id);
+    DCHECK(child != this);
+
+    if (AddChild(child)) {
+      child->AddParent(this);
+    }
+  }
+}
+
+bool CoordinationUnitImpl::AddChild(CoordinationUnitImpl* child) {
+  return children_.count(child) ? false : children_.insert(child).second;
+}
+
+void CoordinationUnitImpl::RemoveChild(CoordinationUnitImpl* child) {
+  size_t children_removed = children_.erase(child);
+  DCHECK_EQ(1u, children_removed);

[nasko, 2017/04/24 22:10:50]

Shouldn't the policy be recalculated on child removal?

[oystein (OOO til 10th of July), 2017/04/25 17:19:16]

The current design is that policies only bubble down, to be able to have clear
rules around priorities (like, visibility=true on a higher level could be
overridden by a visibility=false flag on a lower level; like a tab might be
visible but a frame is occluded, etc). If we start looking downwards in the tree
this gets more confusing (may mean that specific events will have to bubble up).
Added a comment about this.

[nasko, 2017/04/25 21:40:56]

Awesome, thanks!

+}
+
+void CoordinationUnitImpl::AddParent(CoordinationUnitImpl* parent) {
+  DCHECK_EQ(0u, parents_.count(parent));
+  parents_.insert(parent);
+
+  RecalcPolicy();
+}
+
+void CoordinationUnitImpl::RemoveParent(CoordinationUnitImpl* parent) {
+  size_t parents_removed = parents_.erase(parent);
+  DCHECK_EQ(1u, parents_removed);
+
+  RecalcPolicy();
+}
+
+bool CoordinationUnitImpl::HasParent(CoordinationUnitImpl* unit) {
+  for (CoordinationUnitImpl* parent : parents_) {
+    if (parent == unit || parent->HasParent(unit))
+      return true;
+  }
+
+  return false;
+}
+
+bool CoordinationUnitImpl::HasChild(CoordinationUnitImpl* unit) {
+  for (CoordinationUnitImpl* child : children_) {
+    if (child == unit || child->HasChild(unit))
+      return true;
+  }
+
+  return false;
+}
+
+void CoordinationUnitImpl::SetPolicyCallback(
+    mojom::PolicyCallbackPtr callback) {
+  callback.set_connection_error_handler(base::Bind(
+      &CoordinationUnitImpl::UnregisterPolicyCallback, base::Unretained(this)));
+
+  policy_callback_ = std::move(callback);
+
+  RecalcPolicy();
+}
+
+void CoordinationUnitImpl::UnregisterPolicyCallback() {
+  policy_callback_.reset();
+  current_policy_.reset();
+}
+
+}  // namespace resource_coordinator