Move around comments in TraceWriterBase hierarchy

TraceWriterImpl inherits from TraceWriter, which inherits from
TraceWriterBase.

This commit moves most of the comments to TraceWriterBase (in the spirit
of documenting the interface, not the implementation).

This also removes redundant pure virtual method declarations from
TraceWriter. This makes it easier to add methods in future commits (work
in progress).

Change-Id: I70735894007e19eb69a83592045496b6731db0bd
diff --git a/include/perfetto/ext/tracing/core/trace_writer.h b/include/perfetto/ext/tracing/core/trace_writer.h
index 1921371..f5e5eee 100644
--- a/include/perfetto/ext/tracing/core/trace_writer.h
+++ b/include/perfetto/ext/tracing/core/trace_writer.h
@@ -32,20 +32,7 @@
 }  // namespace pbzero
 }  // namespace protos
 
-// This is a single-thread write interface that allows to write protobufs
-// directly into the tracing shared buffer without making any copies.
-// It takes care of acquiring and releasing chunks from the
-// SharedMemoryArbiter and splitting protos over chunks.
-// The idea is that each data source creates one (or more) TraceWriter for each
-// thread it wants to write from. Each TraceWriter will get its own dedicated
-// chunk and will write into the shared buffer without any locking most of the
-// time. Locking will happen only when a chunk is exhausted and a new one is
-// acquired from the arbiter.
-
-// TODO: TraceWriter needs to keep the shared memory buffer alive (refcount?).
-// Otherwise if the shared memory buffer goes away (e.g. the Service crashes)
-// the TraceWriter will keep writing into unmapped memory.
-
+// See comments in include/perfetto/tracing/trace_writer_base.h
 class PERFETTO_EXPORT TraceWriter : public TraceWriterBase {
  public:
   using TracePacketHandle =
@@ -54,37 +41,8 @@
   TraceWriter();
   ~TraceWriter() override;
 
-  // Returns a handle to the root proto message for the trace. The message will
-  // be finalized either by calling directly handle.Finalize() or by letting the
-  // handle go out of scope. The returned handle can be std::move()'d but cannot
-  // be used after either: (i) the TraceWriter instance is destroyed, (ii) a
-  // subsequence NewTracePacket() call is made on the same TraceWriter instance.
-  // The returned packet handle is always valid, but note that, when using
-  // BufferExhaustedPolicy::kDrop and the SMB is exhausted, it may be assigned
-  // a garbage chunk and any trace data written into it will be lost. For more
-  // details on buffer size choices: https://perfetto.dev/docs/concepts/buffers.
-  TracePacketHandle NewTracePacket() override = 0;
-
-  // Commits the data pending for the current chunk into the shared memory
-  // buffer and sends a CommitDataRequest() to the service. This can be called
-  // only if the handle returned by NewTracePacket() has been destroyed (i.e. we
-  // cannot Flush() while writing a TracePacket).
-  // Note: Flush() also happens implicitly when destroying the TraceWriter.
-  // |callback| is an optional callback. When non-null it will request the
-  // service to ACK the flush and will be invoked after the service has
-  // acknowledged it. The callback might be NEVER INVOKED if the service crashes
-  // or the IPC connection is dropped. The callback should be used only by tests
-  // and best-effort features (logging).
-  // TODO(primiano): right now the |callback| will be called on the IPC thread.
-  // This is fine in the current single-thread scenario, but long-term
-  // trace_writer_impl.cc should be smarter and post it on the right thread.
-  void Flush(std::function<void()> callback = {}) override = 0;
-
   virtual WriterID writer_id() const = 0;
 
-  // Bytes written since creation. Is not reset when new chunks are acquired.
-  virtual uint64_t written() const override = 0;
-
  private:
   TraceWriter(const TraceWriter&) = delete;
   TraceWriter& operator=(const TraceWriter&) = delete;
diff --git a/include/perfetto/tracing/trace_writer_base.h b/include/perfetto/tracing/trace_writer_base.h
index 824be1a..83bf154 100644
--- a/include/perfetto/tracing/trace_writer_base.h
+++ b/include/perfetto/tracing/trace_writer_base.h
@@ -27,17 +27,46 @@
 }  // namespace pbzero
 }  // namespace protos
 
-// The bare-minimum subset of the TraceWriter interface that is exposed as a
-// fully public API.
-// See comments in /include/perfetto/ext/tracing/core/trace_writer.h.
+// This is a single-thread write interface that allows to write protobufs
+// directly into the tracing shared buffer without making any copies.
+// The idea is that each data source creates one (or more) TraceWriter for each
+// thread it wants to write from. Each TraceWriter will get its own dedicated
+// chunk and will write into the shared buffer without any locking most of the
+// time.
+
 class TraceWriterBase {
  public:
   virtual ~TraceWriterBase();
 
+  // Creates a new trace packet and returns a handle to a protozero Message that
+  // will write to it. The message will be finalized either by calling directly
+  // handle.Finalize() or by letting the handle go out of scope (the message
+  // should be finalized before a new call to NewTracePacket is made). The
+  // returned handle can be std::move()'d but cannot be used after either: (i)
+  // the TraceWriter instance is destroyed, (ii) a subsequence NewTracePacket()
+  // call is made on the same TraceWriter instance.
+  //
+  // The returned packet handle is always valid, but note that, when using
+  // BufferExhaustedPolicy::kDrop and the SMB is exhausted, it may be assigned
+  // a garbage chunk and any trace data written into it will be lost. For more
+  // details on buffer size choices: https://perfetto.dev/docs/concepts/buffers.
   virtual protozero::MessageHandle<protos::pbzero::TracePacket>
   NewTracePacket() = 0;
 
+  // Commits the data pending for the current chunk. This can be called
+  // only if the handle returned by NewTracePacket() has been destroyed (i.e. we
+  // cannot Flush() while writing a TracePacket).
+  //
+  // Note: Flush() also happens implicitly when destroying the TraceWriter.
+  //
+  // |callback| is an optional callback. When non-null it will request the
+  // service to ACK the flush and will be invoked after the service has
+  // acknowledged it. The callback might be NEVER INVOKED if the service crashes
+  // or the IPC connection is dropped. The callback should be used only by tests
+  // and best-effort features (logging).
   virtual void Flush(std::function<void()> callback = {}) = 0;
+
+  // Bytes written since creation. Not reset when new chunks are acquired.
   virtual uint64_t written() const = 0;
 };
 
diff --git a/src/tracing/core/null_trace_writer.h b/src/tracing/core/null_trace_writer.h
index 4f6c707..0875d03 100644
--- a/src/tracing/core/null_trace_writer.h
+++ b/src/tracing/core/null_trace_writer.h
@@ -26,7 +26,7 @@
 
 // A specialization of TraceWriter which no-ops all the writes routing them
 // into a fixed region of memory
-// See //include/perfetto/tracing/core/trace_writer.h for docs.
+// See //include/perfetto/ext/tracing/core/trace_writer.h for docs.
 class NullTraceWriter : public TraceWriter {
  public:
   NullTraceWriter();
diff --git a/src/tracing/core/trace_writer_for_testing.h b/src/tracing/core/trace_writer_for_testing.h
index 16c7c76..0038641 100644
--- a/src/tracing/core/trace_writer_for_testing.h
+++ b/src/tracing/core/trace_writer_for_testing.h
@@ -28,7 +28,7 @@
 
 // A specialization of TraceWriter for testing which writes into memory
 // allocated by the ScatteredHeapBuffer.
-// See //include/perfetto/tracing/core/trace_writer.h for docs.
+// See //include/perfetto/ext/tracing/core/trace_writer.h for docs.
 class TraceWriterForTesting : public TraceWriter {
  public:
   TraceWriterForTesting();
diff --git a/src/tracing/core/trace_writer_impl.h b/src/tracing/core/trace_writer_impl.h
index 9f3b970..7a52108 100644
--- a/src/tracing/core/trace_writer_impl.h
+++ b/src/tracing/core/trace_writer_impl.h
@@ -33,7 +33,15 @@
 
 class SharedMemoryArbiterImpl;
 
-// See //include/perfetto/tracing/core/trace_writer.h for docs.
+// See //include/perfetto/ext/tracing/core/trace_writer.h for docs.
+//
+// Locking will happen only when a chunk is exhausted and a new one is
+// acquired from the arbiter.
+//
+// TODO: TraceWriter needs to keep the shared memory buffer alive (refcount?).
+// Otherwise if the shared memory buffer goes away (e.g. the Service crashes)
+// the TraceWriter will keep writing into unmapped memory.
+//
 class TraceWriterImpl : public TraceWriter,
                         public protozero::ScatteredStreamWriter::Delegate {
  public:
@@ -46,6 +54,11 @@
 
   // TraceWriter implementation. See documentation in trace_writer.h.
   TracePacketHandle NewTracePacket() override;
+  // Commits the data pending for the current chunk into the shared memory
+  // buffer and sends a CommitDataRequest() to the service.
+  // TODO(primiano): right now the |callback| will be called on the IPC thread.
+  // This is fine in the current single-thread scenario, but long-term
+  // trace_writer_impl.cc should be smarter and post it on the right thread.
   void Flush(std::function<void()> callback = {}) override;
   WriterID writer_id() const override;
   uint64_t written() const override {