tracing: the recorder implementation

services/resource_coordinator/tracing/recorder.h

Index: services/resource_coordinator/tracing/recorder.h
diff --git a/services/resource_coordinator/tracing/recorder.h b/services/resource_coordinator/tracing/recorder.h
new file mode 100644
index 0000000000000000000000000000000000000000..3e606a80ea3fc2c312526a06428dfefa469a23ac
--- /dev/null
+++ b/services/resource_coordinator/tracing/recorder.h
@@ -0,0 +1,68 @@
+// 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_RECORDER_H_
+#define SERVICES_RESOURCE_COORDINATOR_TRACING_RECORDER_H_
+
+#include "base/callback_forward.h"
+#include "base/memory/ref_counted.h"
+#include "base/sequenced_task_runner.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "services/resource_coordinator/public/interfaces/tracing/tracing.mojom.h"
+
+namespace {
+class DictionaryValue;
+}
+
+namespace tracing {
+
+class Recorder : public mojom::Recorder {
+ public:
+  Recorder(

[Primiano Tucci (use gerrit), 2017/05/25 13:15:01]

can you add some comments to explain what these arguments are for?

[chiniforooshan, 2017/05/25 15:34:24]

Done.

+      mojom::RecorderRequest request,
+      bool is_array,

[Primiano Tucci (use gerrit), 2017/05/25 13:15:01]

data_is_array ? I understood what this was only at the end of the cl

[chiniforooshan, 2017/05/25 15:34:24]

Done.

+      const base::Closure& recorder_change_callback,

[Primiano Tucci (use gerrit), 2017/05/25 13:15:01]

Isn't this clearer if you call it on_data_change_callback?

[chiniforooshan, 2017/05/25 15:34:24]

Yes, although the callback is also run when the connection is closed, to signal
the end of receiving data; but, that's kind of a data change, too :) Done.

+      const scoped_refptr<base::SequencedTaskRunner>& background_task_runner);
+  ~Recorder() override;
+
+  const std::string& data() const {
+    // All access to |data_| should be done on the background thread.
+    DCHECK(background_task_runner_->RunsTasksOnCurrentThread());
+    return data_;
+  }
+
+  void clear_data() {
+    // All access to |data_| should be done on the background thread.
+    DCHECK(background_task_runner_->RunsTasksOnCurrentThread());
+    data_.clear();
+  }
+
+  const base::DictionaryValue& metadata() const { return metadata_; }
+  bool is_recording() const { return is_recording_; }
+  bool is_array() const { return is_array_; }
+
+ private:
+  friend class RecorderTest;  // For testing.
+  // mojom::Recorder

[Primiano Tucci (use gerrit), 2017/05/25 13:15:01]

small&not-strong style thing: not sure if there is a guideline for this, I tend
to prefer the mojo implementation methods to be in the public: section. I know
they don't have to, but makes easier to read the class, as they are de-facto the
main methods that the class is exposing.
But honestly don't know what happens in other mojo implementaitons and I might
be the outlier here?

[chiniforooshan, 2017/05/25 15:34:25]

There are examples of both in the code base. I cannot say which one is
clearly more popular.

My preference is private by default unless we need them to be public mainly
for thread safety. When they are private, it's easier to see that they are
called by Mojo internals only, and Mojo promises to call them from one
thread only. If they are public, it's harder to prevent future random calls
from random threads; we have to add a thread_checker or something.

+  void AddChunk(const std::string& chunk) override;

[Primiano Tucci (use gerrit), 2017/05/25 13:15:01]

Can you please add some comments to specify who adds chunk into what? Services
are great for their loose code coupling, but at the same time that makes a bit
harder to understand the end-to-end call chain.

Maybe something like
// These methods are called by ??? to ???

[chiniforooshan, 2017/05/25 15:34:24]

Done.

+  void AddMetadata(std::unique_ptr<base::DictionaryValue> metadata) override;
+
+  void OnConnectionError();
+
+  std::string data_;
+  base::DictionaryValue metadata_;
+  bool is_recording_;
+  bool is_array_;
+  base::Closure recorder_change_callback_;
+  // To avoid blocking the UI thread if the tracing service is run on the UI
+  // thread, we make sure that buffering trace events and copying them to the
+  // final stream is done on a background thread.
+  scoped_refptr<base::SequencedTaskRunner> background_task_runner_;
+  mojo::Binding<mojom::Recorder> binding_;
+
+  DISALLOW_COPY_AND_ASSIGN(Recorder);
+};
+
+}  // namespace tracing
+#endif  // SERVICES_RESOURCE_COORDINATOR_TRACING_RECORDER_H_