Stop using callbacks in MemoryCoordinatorProxy

base/memory/memory_coordinator_proxy.h

Index: base/memory/memory_coordinator_proxy.h
diff --git a/base/memory/memory_coordinator_proxy.h b/base/memory/memory_coordinator_proxy.h
index 4148da5dceec0c59e39462a6f740569263432ba9..2adf5657de490b5f1bdbd05b6c4fbf5e5babe3ab 100644
--- a/base/memory/memory_coordinator_proxy.h
+++ b/base/memory/memory_coordinator_proxy.h
@@ -6,33 +6,38 @@
 #define BASE_MEMORY_MEMORY_COORDINATOR_PROXY_H_
 
 #include "base/base_export.h"
-#include "base/callback.h"
 #include "base/memory/memory_coordinator_client.h"
 #include "base/memory/singleton.h"
+#include "base/memory/weak_ptr.h"
 
 namespace base {
 
+// An interface to MemoryCoordinator. See comments in MemoryCoordinatorProxy for
+// method descriptions.
+class BASE_EXPORT MemoryCoordinatorInterface {
+ public:
+  virtual ~MemoryCoordinatorInterface() {}
+
+  virtual MemoryState GetLocalMemoryState() = 0;

[dcheng, 2016/12/12 07:27:07]

Two nits:
1) I think it's more common to document the interface and not document the
implementation. Should we do that here?

2) It's not very common to have "Interface" in the names of interfaces. I think
it's more common to just have the interface be called "MemoryCoordinator" and
have the implementation suffixed with Impl so it's called MemoryCoordinatorImpl.

[bashi, 2016/12/12 07:38:39]

Let me clarify your comments. Do you mean moving comments from Proxy to here?
Yeah, but I can find such naming[1]. I totally agree with your comments but we
already have MemoryCoordinator and MemoryCoordinatorImpl in content/. Do you
want us to rename them before this CL? What kind of names we should in that
case?

[1]
https://cs.chromium.org/chromium/src/chrome/browser/task_manager/task_manager...

[dcheng, 2016/12/12 08:31:13]

Yes.
If it would be hard for this CL, it's OK to defer. But I would like to think
about improving the names in a followup patch: generally speaking, "Interface"
isn't very meaningful in a type name.

[chrisha, 2016/12/13 15:42:47]

+1 to dropping "Interface" and putting the comments on the class defining the
interface.

+  virtual void SetMemoryStateForTesting(MemoryState state) = 0;
+};
+
 // The proxy of MemoryCoordinator to be accessed from components that are not
 // in content/browser e.g. net.
 class BASE_EXPORT MemoryCoordinatorProxy {
  public:
-  using GetCurrentMemoryStateCallback = base::Callback<MemoryState()>;
-  using SetCurrentMemoryStateCallback = base::Callback<void(MemoryState)>;
-
   static MemoryCoordinatorProxy* GetInstance();
 
-  // Returns the current memory state.
-  MemoryState GetCurrentMemoryState() const;
-
-  // Sets the current memory state. This function is for testing only.
-  void SetCurrentMemoryStateForTesting(MemoryState memory_state);
+  // Sets an instance of MemoryCoordinator
+  void Set(WeakPtr<MemoryCoordinatorInterface> instance);

[dcheng, 2016/12/12 07:27:07]

Does this need to be a weak pointer? Maybe it should just unregister itself on
destruction by calling this with nullptr?

[bashi, 2016/12/12 07:38:39]

Sure. But I think it would be better to have explicit logic to avoid
use-after-free rather than putting comments like "must be removed on
destruction". WDYT?

[dcheng, 2016/12/12 08:31:13]

I actually prefer that model: the WeakPtr make it seem like we might have to
dispatch calls on a potentially deleted object, when we can accurately manage
lifetimes ourselves.

I would just say that MemoryCoordinatorInterface implementations should just
self-unregister in their destructor. However it looks like
MemoryCoordinatorInterface is a leaky singleton, so it will never be destroyed
anyway?

[chrisha, 2016/12/13 15:42:47]

Ditto. I'd prefer explicit management here. We already have lifetime issues in
that GetInstance() returns a raw pointer, which a client may cache even after
the WeakPtr is invalidated.

 
-  // Sets state-getter callback.
-  void SetGetCurrentMemoryStateCallback(GetCurrentMemoryStateCallback callback);
+  // Returns the process's current memory state. Note that the local state
+  // could be different from the global memory state as some processes cannot
+  // be suspended.
+  MemoryState GetLocalMemoryState() const;
 
-  // Sets state-setter callback.
-  void SetSetCurrentMemoryStateForTestingCallback(
-      SetCurrentMemoryStateCallback callback);
+  // Sets the memory state of this process. This function is for testing only.
+  void SetMemoryStateForTesting(MemoryState memory_state);

[chrisha, 2016/12/13 15:42:47]

Why are these two functions needed here, if it's available on the Interface?

This class should simply by the injection point, and the place where a client
can get a pointer to a MemoryCoordinator, no?

 
  private:
   friend struct base::DefaultSingletonTraits<MemoryCoordinatorProxy>;
@@ -40,8 +45,7 @@ class BASE_EXPORT MemoryCoordinatorProxy {
   MemoryCoordinatorProxy();
   virtual ~MemoryCoordinatorProxy();
 
-  GetCurrentMemoryStateCallback getter_callback_;
-  SetCurrentMemoryStateCallback setter_callback_;
+  WeakPtr<MemoryCoordinatorInterface> instance_;
 
   DISALLOW_COPY_AND_ASSIGN(MemoryCoordinatorProxy);
 };