[memory-infra] Remove MemoryDumpManagerDelegate

base/trace_event/memory_dump_manager.h

Index: base/trace_event/memory_dump_manager.h
diff --git a/base/trace_event/memory_dump_manager.h b/base/trace_event/memory_dump_manager.h
index d359a8c3e9722f8441c907d06a2ae3b91cc8b7c8..5253071174bca87d15366cef7ce3752b12b0f78b 100644
--- a/base/trace_event/memory_dump_manager.h
+++ b/base/trace_event/memory_dump_manager.h
@@ -24,11 +24,11 @@
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event.h"
 
-// Forward declare |MemoryDumpManagerDelegateImplTest| so that we can make it a
+// Forward declare |ProcessLocalDumpManagerImplTest| so that we can make it a
 // friend of |MemoryDumpManager| and give it access to |SetInstanceForTesting|.
 namespace memory_instrumentation {
 
-class MemoryDumpManagerDelegateImplTest;
+class ProcessLocalDumpManagerImplTest;
 
 }  // namespace memory_instrumentation
 
@@ -39,7 +39,6 @@ class Thread;
 
 namespace trace_event {
 
-class MemoryDumpManagerDelegate;
 class MemoryDumpProvider;
 class MemoryDumpSessionState;
 
@@ -63,10 +62,15 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   // On the other side, the MemoryDumpManager will not be fully operational
   // (i.e. will NACK any RequestGlobalMemoryDump()) until initialized.
   // Arguments:
-  //  delegate: inversion-of-control interface for embedder-specific behaviors
-  //      (multiprocess handshaking). See the lifetime and thread-safety
-  //      requirements in the |MemoryDumpManagerDelegate| docstring.
-  void Initialize(std::unique_ptr<MemoryDumpManagerDelegate> delegate);
+  //  request_dump_callback: Callback to invoke a global dump. Global dump
+  //      involves embedder-specific behaviors like multiprocess handshaking.
+  //  is_coordinator: True when current process coodinates the periodic dump
+  //      triggering.
+  using RequestGlobalDumpCallback =

[Primiano Tucci (use gerrit), 2017/04/19 15:44:34]

small nit: type declarations should go up, after public:
see https://google.github.io/styleguide/cppguide.html#Declaration_Order

we have a bunch of things called "callback" around these days. Can we pick a
more specific name for this?
I'd probably not call it "callback" at all (there is precedent for this *) as
this is semantically not quite a callback but more an inverted function call.
probably I'd call it RequestGlobalDumpFunction.

Also can you add a "TODO(primiano): in the long term clients should just reach
into the memory service. For the moment however that requires extra dependencies
and we use this as a way to reach the service from this base class."

*
https://cs.chromium.org/search/?q=using%5C+%5Cw%2Bfunction%5C+%3D%5C+.*Callba...

[ssid, 2017/04/20 01:18:21]

I don't think this should be moved. Let's say in future we have a new allocator
in base that does a GC and we want to request global dumps when GC is done. This
would become impossible because it can't reach the service. Similarly lot of
areas in codebase cannot depend on service, like sql/ and gin/. It will be
better if we keep this api in base.
Yeah I was expecting this comment. Just felt function was not used in chromium.
Fixed.

[Primiano Tucci (use gerrit), 2017/04/20 20:30:05]

Hmm good point. ACK

+      RepeatingCallback<void(const MemoryDumpRequestArgs& args,
+                             const GlobalMemoryDumpCallback& callback)>;
+  void Initialize(RequestGlobalDumpCallback request_dump_callback,
+                  bool is_coordinator);
 
   // (Un)Registers a MemoryDumpProvider instance.
   // Args:
@@ -119,6 +123,14 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   void RequestGlobalDump(MemoryDumpType dump_type,
                          MemoryDumpLevelOfDetail level_of_detail);
 
+  // NOTE: Use RequestGlobalDump() to create memory dumps. Creates a memory dump

[Primiano Tucci (use gerrit), 2017/04/19 15:44:34]

Since you have the "This method should only be used..." I'd remote the NOTE
here.

[ssid, 2017/04/20 01:18:21]

I'd like to keep this note because it is really easy for someone to call this
function by mistake since it's named CreateProcessDump()

[Primiano Tucci (use gerrit), 2017/04/20 20:30:05]

Acknowledged.

+  // for the current process and appends it to the trace. |callback| will be
+  // invoked asynchronously upon completion on the same thread on which
+  // CreateProcessDump() was called. This method should only be used by the
+  // embedder while creating a global memory dump.
+  void CreateProcessDump(const MemoryDumpRequestArgs& args,
+                         const ProcessMemoryDumpCallback& callback);
+
   // TraceLog::EnabledStateObserver implementation.
   void OnTraceLogEnabled() override;
   void OnTraceLogDisabled() override;
@@ -165,9 +177,10 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
  private:
   friend std::default_delete<MemoryDumpManager>;  // For the testing instance.
   friend struct DefaultSingletonTraits<MemoryDumpManager>;
+  // todo fix

[fmeawad, 2017/04/18 14:48:11]

Don't forget this TODO

[ssid, 2017/04/20 01:18:21]

Done.

   friend class MemoryDumpManagerDelegate;
   friend class MemoryDumpManagerTest;
-  friend class memory_instrumentation::MemoryDumpManagerDelegateImplTest;
+  friend class memory_instrumentation::ProcessLocalDumpManagerImplTest;
 
   // Holds the state of a process memory dump that needs to be carried over
   // across task runners in order to fulfil an asynchronous CreateProcessDump()
@@ -237,13 +250,6 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   static void FinalizeDumpAndAddToTrace(
       std::unique_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
 
-  // Internal, used only by MemoryDumpManagerDelegate.
-  // Creates a memory dump for the current process and appends it to the trace.
-  // |callback| will be invoked asynchronously upon completion on the same
-  // thread on which CreateProcessDump() was called.
-  void CreateProcessDump(const MemoryDumpRequestArgs& args,
-                         const ProcessMemoryDumpCallback& callback);
-
   // Calls InvokeOnMemoryDump() for the next MDP on the task runner specified by
   // the MDP while registration. On failure to do so, skips and continues to
   // next MDP.
@@ -283,10 +289,15 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   std::unordered_set<StringPiece, StringPieceHash>
       strict_thread_check_blacklist_;
 
-  std::unique_ptr<MemoryDumpManagerDelegate> delegate_;
+  // Callback provided by the embedder to handle global dump requests.
+  RequestGlobalDumpCallback request_dump_callback_;

[Primiano Tucci (use gerrit), 2017/04/19 15:44:34]

s/callback/function/

[ssid, 2017/04/20 01:18:21]

Done.

+
+  // True when current process coordinates the periodic dump triggering.
+  bool is_coordinator_;
 
-  // Protects from concurrent accesses to the |dump_providers_*| and |delegate_|
-  // to guard against disabling logging while dumping on another thread.
+  // Protects from concurrent accesses to the |dump_providers_*| and
+  // |is_coordinator_| to guard against disabling logging while dumping on

[Primiano Tucci (use gerrit), 2017/04/19 15:44:34]

I think over time more stuff got serialized aroudn this lock (the dump_thread_
iirc) so I'd probably just say: "Protects form concurrent access to the local
state, e.g. to guard against ..."

[ssid, 2017/04/20 01:18:20]

Done.

+  // another thread.
   Lock lock_;
 
   // Optimization to avoid attempting any memory dump (i.e. to not walk an empty
@@ -310,29 +321,6 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   DISALLOW_COPY_AND_ASSIGN(MemoryDumpManager);
 };
 
-// The delegate is supposed to be long lived (read: a Singleton) and thread
-// safe (i.e. should expect calls from any thread and handle thread hopping).
-class BASE_EXPORT MemoryDumpManagerDelegate {
- public:
-  MemoryDumpManagerDelegate() {}
-  virtual ~MemoryDumpManagerDelegate() {}
-
-  virtual void RequestGlobalMemoryDump(
-      const MemoryDumpRequestArgs& args,
-      const GlobalMemoryDumpCallback& callback) = 0;
-
-  virtual bool IsCoordinator() const = 0;
-
- protected:
-  void CreateProcessDump(const MemoryDumpRequestArgs& args,
-                         const ProcessMemoryDumpCallback& callback) {
-    MemoryDumpManager::GetInstance()->CreateProcessDump(args, callback);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MemoryDumpManagerDelegate);
-};
-
 }  // namespace trace_event
 }  // namespace base