tracing: the agent registry implementation

services/resource_coordinator/tracing/agent_registry.h

Index: services/resource_coordinator/tracing/agent_registry.h
diff --git a/services/resource_coordinator/tracing/agent_registry.h b/services/resource_coordinator/tracing/agent_registry.h
new file mode 100644
index 0000000000000000000000000000000000000000..64249ca15abd79fbb3a50a5101ec0042acb46b38
--- /dev/null
+++ b/services/resource_coordinator/tracing/agent_registry.h
@@ -0,0 +1,104 @@
+// 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.
+
+#ifndef SERVICES_RESOURCE_COORDINATOR_TRACING_AGENT_REGISTRY_H_
+#define SERVICES_RESOURCE_COORDINATOR_TRACING_AGENT_REGISTRY_H_
+
+#include "base/callback_forward.h"
+#include "base/threading/thread_checker.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
+#include "services/resource_coordinator/public/interfaces/tracing/tracing.mojom.h"
+#include "services/service_manager/public/cpp/bind_source_info.h"
+
+namespace tracing {
+
+class AgentRegistry : public mojom::AgentRegistry {

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

s/AgentRegistry/AgentRegistryImpl/ as in the other CL seems to be the
convention.

[chiniforooshan, 2017/05/26 15:46:31]

As Ken mentioned in the other CL, looks like AgentRegistry is preferred :)

+ public:
+  class Entry {

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

Any more verbose names that can be used for this? AgentEntry?

[chiniforooshan, 2017/05/26 15:46:32]

Done.

+   public:
+    Entry(size_t id,
+          AgentRegistry* agent_registry,
+          mojom::AgentPtr agent,
+          const std::string& label,
+          mojom::TraceDataType type,
+          bool supports_explicit_clock_sync);
+    ~Entry();
+
+    // Currently, at most one callback when the tracing agent is disconnected is
+    // enough. We can generalize this later if several parts of the service need
+    // to get notified when an agent disconnects.
+    void SetDisconnectClosure(base::OnceClosure closure);
+    bool RemoveDisconnectClosure();
+
+    mojom::Agent* agent() const { return agent_.get(); }
+    const std::string& label() const { return label_; }
+    mojom::TraceDataType type() const { return type_; }
+    bool supports_explicit_clock_sync() const {
+      return supports_explicit_clock_sync_;
+    }
+
+   private:
+    void OnConnectionError();
+
+    const size_t id_;
+    AgentRegistry* agent_registry_;
+    mojom::AgentPtr agent_;
+    const std::string label_;
+    const mojom::TraceDataType type_;
+    const bool supports_explicit_clock_sync_;
+    base::OnceClosure closure_;
+
+    DISALLOW_COPY_AND_ASSIGN(Entry);
+  };
+
+  // A function to be run for every agent that registers itself.
+  typedef base::RepeatingCallback<void(Entry*)> AgentInitializationCallback;

[Primiano Tucci (use gerrit), 2017/05/25 13:24:28]

using AgentInitializationCallback = base....

[chiniforooshan, 2017/05/26 15:46:32]

Done (I promise to remember this next time :p).

+
+  // The getter of the unique instance.

[Primiano Tucci (use gerrit), 2017/05/25 13:24:28]

I guess this is a quite common pattern in chrome so this comment might be
unnecessary 

[chiniforooshan, 2017/05/26 15:46:32]

Done.

+  static AgentRegistry* GetInstance();
+
+  AgentRegistry();
+
+  void BindAgentRegistryRequest(
+      const service_manager::BindSourceInfo& source_info,
+      mojom::AgentRegistryRequest request);
+  void SetAgentInitializationCallback(
+      const AgentInitializationCallback& callback);
+  void RemoveAgentInitializationCallback();
+
+  template <typename FunctionType>
+  void ForAllAgents(FunctionType function) {
+    DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

[Primiano Tucci (use gerrit), 2017/05/25 13:24:28]

is this really needed? It feels that without this you could just write below:

for(const auto& agent : agents_)
  agent_initialization_callback_.Run(agent); 

so not sure this extra complexity is really helping. 

[chiniforooshan, 2017/05/26 15:46:31]

This is the public method that gives access to the set
of registered agents. |agents_| is private. The coordinator,
that will come in the next CL, will use this for sending
messages to agents (you can see the usage in the prototype,
too).

+    for (const auto& key_value : agents_) {
+      function(key_value.second.get());
+    }
+  }
+
+ private:
+  friend std::default_delete<AgentRegistry>;
+  friend class AgentRegistryTest;  // For testing.
+
+  ~AgentRegistry() override;
+
+  // mojom::AgentRegistry

[Primiano Tucci (use gerrit), 2017/05/25 13:24:28]

similar comment to the other CL. not sure if there is a suggested pattern here,
but I find slightly more readable having these in the public: section as this is
more the real API of the class

[chiniforooshan, 2017/05/26 15:46:32]

My preference is to keep this private so that only
mojo internals call it from the right thread, if it's
OK with you?

+  void RegisterAgent(mojom::AgentPtr agent,
+                     const std::string& label,
+                     mojom::TraceDataType type,
+                     bool supports_explicit_clock_sync) override;
+
+  void UnregisterAgent(size_t agent_id);
+
+  mojo::BindingSet<mojom::AgentRegistry> bindings_;
+  size_t next_agent_id_ = 0;
+  std::unordered_map<size_t, std::unique_ptr<Entry>> agents_;

[Primiano Tucci (use gerrit), 2017/05/25 13:24:28]

see brett's PSA on chromium-dev about unordered_map.
TL;DR  (IIRC) use either map<> or flat_map<>

[chiniforooshan, 2017/05/26 15:46:32]

Done.

+  AgentInitializationCallback agent_initialization_callback_;
+
+  THREAD_CHECKER(thread_checker_);
+
+  DISALLOW_COPY_AND_ASSIGN(AgentRegistry);

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

nit: "base/macros.h"

[chiniforooshan, 2017/05/26 15:46:32]

Done.

+};
+
+}  // namespace tracing
+
+#endif  // SERVICES_RESOURCE_COORDINATOR_TRACING_AGENT_REGISTRY_H_