Make memory pressure notifier configurable

base/memory/memory_pressure_monitor.h

 // Copyright 2015 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 BASE_MEMORY_MEMORY_PRESSURE_MONITOR_H_
 #define BASE_MEMORY_MEMORY_PRESSURE_MONITOR_H_
 
 #include "base/base_export.h"
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/memory_pressure_listener.h"
 
 namespace base {
 
 // TODO(chrisha): Make this a concrete class with per-OS implementations rather

[danakj, 2016/06/30 18:48:05]

Is this something that will be done as part of this? Because on the list of
things that make me uncomfortable about code, near the top is classes that are
half abstract and half concrete. And that's where this goes now.

[bashi, 2016/06/30 23:33:10]

I don't plan to do that. I'm not sure this still applies because we need to have
a way to change notifier from MemoryPressureListener to MemoryCoordinator which
we are going to implement.

[danakj, 2016/07/01 20:49:28]

I think this comment is saying GetCurrentPressureLevel should not be virtual, we
should just have it as a concrete method, and the definition is simply in
different platform-specific places. I don't really want to leave us with this
half-abstract class forever. So what should be the plan?

[danakj, 2016/07/01 20:50:09]

An alternate could be to make the setter abstract and have each OS implement it.

[bashi, 2016/07/03 23:54:47]

Could you elaborate what's the benefit of doing that and avoiding half
abstraction? Do you want me to do refactoring before doing this kind of change?
I'm not really interested in refactoring this class but want to have a way to
change receiver of pressure notification.

[danakj, 2016/07/06 18:57:23]

Half abstraction looks fine in the first CL, but as a class evolves over time,
it gets increasingly murky where code should live, and trying to understand how
it works. I've watched this happen to a few classes in chromium over time, and
it has become a red flag for me.

It would be a pretty simply change in this CL to avoid it by just making
SetNotifier be a pure virtual method, and just write a defn for it in each
subclass of this.

[danakj, 2016/07/06 19:00:20]

The Notify() method could just be a concrete method in each subclass and would
not need to be part of this API (an example of how mixing concrete and abstract
goes wrong IMO, the real public API gets unclear). And it would only need to be
public for the Mac version?

[bashi, 2016/07/07 01:42:39]

I'm not fully convinced but updated the CL as you suggested  (dup of code for
non-planned future extension seems premature to me). I personally feel the
previous CL looks better but I want to land this as soon as possible.

[chrisha, 2016/07/07 03:09:19]

That's exactly what the comment is saying. MemoryPressureMonitor is simply going
to disappear in its current incarnation, being replaced with equivalent
functionality in the memory_coordinator component. This CL is mostly about
putting in place a seam to intercept the existing MP messages and to route them
via the new MC system, which is in progress. I don't think it's worth addressing
the comment in this code, as the component refactor will take care of it over
there.

 // than an abstract base class.
 
 // Declares the interface for a MemoryPressureMonitor. There are multiple
 // OS specific implementations of this class. An instance of the memory
 // pressure observer is created at the process level, tracks memory usage, and
 // pushes memory state change notifications to the static function
 // base::MemoryPressureListener::NotifyMemoryPressure. This is turn notifies
 // all MemoryPressureListener instances via a callback.
 class BASE_EXPORT MemoryPressureMonitor {
  public:
   using MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel;
 
   virtual ~MemoryPressureMonitor();
 
   // Return the singleton MemoryPressureMonitor.
   static MemoryPressureMonitor* Get();
 
   // Returns the currently observed memory pressure.
   virtual MemoryPressureLevel GetCurrentPressureLevel() const = 0;
 
+  // Sets a notification callback. Once a callback is set,

[danakj, 2016/06/30 18:48:05]

I tried to get this from the doc, but I think maybe it's hidden below the
surface of the larger design:
- Why is it a callback instead of a listener interface?
- Why is there only a single listener instead of a set?

[bashi, 2016/06/30 23:33:10]

We are implementing MemoryCoordinator which will replace MemoryPressureListener.
Currently MemoryPressureMonitor lives in base/ and MemoryCoordinator will live
in components/ so I use callback to avoid dependency violation.
I'm afraid you may misunderstand what I'm going to do here. There are some
listeners actually. MemoryPressureListener::NotifyMemoryPressure() is a static
method and  it dispatches listeners owned by a singleton called g_observer. And
what I'm going to do in this CL is to replace it with a callback.

[danakj, 2016/07/01 20:49:28]

A virtual interface would do the same, where MemoryPressureMonitorObserver is an
abstract class in base, and something in components implements it and passes
that to the SetObserver method here. I prefer virtual interfaces to callbacks
for things like this when they make sense (you don't need to bind state into it)
because they are easier to find/understand in codesearch.
I see, so there may be multiple observers, but they would register with the
thing that's listening to this class here.

[bashi, 2016/07/03 23:54:47]

That makes sense. Done.

+  // MemoryPressureMonitor pushes memory pressure notifications to the callback
+  // instead of base::MemoryPressureListener::NotifyMemoryPressure.
+  using Notifier = base::Callback<void(MemoryPressureLevel level)>;

[danakj, 2016/06/30 18:48:05]

"Notifier" is the wrong direction here. The notifier is the monitor. This would
be a Listener or an Observer or the like, it receives notifications.

[bashi, 2016/06/30 23:33:10]

The callback replaces MemoryPressureListener::NotifyMemoryPressure(), which
notifies pressure level.

[danakj, 2016/07/01 20:49:28]

Yes, and the monitor notifies it by that, so it *is* the notifier. The Set here
is coming from someone who wants to be notified, to receive notifications. So
the relationship needs to be inverted in the naming.

+  void SetNotifier(const Notifier& notifier) { notifier_ = notifier; }

[danakj, 2016/07/01 20:49:28]

Can we DCHECK that this is only set once? Or only set to/from null, but not
clobbering a non-null with a non-null? Or do we need to support that?

[bashi, 2016/07/03 23:54:47]

Added DCHECK.

+
+  void Notify(MemoryPressureLevel level);

[bashi, 2016/06/27 03:47:54]

public because this is called by a static method
(mac::MemoryPressureMonitor::NotifyMemoryPressureChanged()).

+
  protected:
   MemoryPressureMonitor();
 
  private:
+  Notifier notifier_;
+
   DISALLOW_COPY_AND_ASSIGN(MemoryPressureMonitor);
 };
 
 }  // namespace base
 
 #endif  // BASE_MEMORY_MEMORY_PRESSURE_MONITOR_H_