Add documentation for MemoryCoordinator-related primitives

content/public/browser/memory_coordinator.h

 // Copyright 2016 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 CONTENT_PUBLIC_BROWSER_MEMORY_COORDINATOR_H_
 #define CONTENT_PUBLIC_BROWSER_MEMORY_COORDINATOR_H_
 
 #include "base/memory/memory_coordinator_client.h"
 #include "base/process/process_handle.h"
 #include "content/common/content_export.h"
 
 namespace content {
 
 // The interface that represents the browser side of MemoryCoordinator.
 // MemoryCoordinator determines memory state of each process (both browser
 // and renderers) and dispatches state change notifications to its clients.
-// See comments on MemoryCoordinatorClient for details.
+// See comments on MemoryCoordinatorClient for how to use it.
+//
+// Currently we have three ways to control memory components:

[Ryan Sleevi, 2017/02/08 15:02:04]

Similar to my earlier remarks, the "we" here is ambiguous - is it "the owners of
//content/public/browser"? The owners of Chromium?

When looking through this CL, I was surprised to see that (effectively) it's a
coupling between //base/memory and //content/public/browser - which seems like a
layering violation that we're defining an interface in //base whose behaviour
and relationship can only be explained in terms of //content/public/browser

I know there are plans to move things around to better accomodate that, and I
would suggest that while it may not be a critical blocker relative to your other
tasks, for your colleagues such as myself, having things either logically make
sense or having a clearly documented roadmap (e.g. in source via Markdown) is
something to be prioritized over any feature work. That is - if folks are to use
//base/memory/memory_coordinator_client.h, then we should prioritize moving the
memory coordinator infrastructure to base.

This is especially important and critical for a project like //net, which does
not depend on //content/public, and for applications such as Cronet, which
similarly do not depend on this. Being able to understand the interfaces and
their relationship is an important part of code health.

I would suggest that because the other two classes in //base are effectively
saying "see //content/public" - it is a layering violation, the same as if base
#include'd "//content/public". The way to resolve this is like I mentioned -
using an explicit document that provides guidance.

I think especially useful for the transition is if you could consider something
like how https://google.github.io/styleguide/cppguide.html is structured with
respect to examples; that is, when there are N ways of doing something, it
provides examples of the 'syntactically valid but not acceptable/desirable'
form, and then provides an example of showing the 'right' form. We use that same
approach in
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
when discussion conditionals.

[haraken, 2017/02/09 02:37:31]

Yeah, the fact that we don't have MemoryCoordinator in //base is making things
complicated. And I think it should be fixed. As you mentioned, this causes a
practical problem when we want to use MemoryCoordinator in //net.

+// MemoryCoordinator, MemoryPressureListener and isLowEndDevice.
+//
+//   - MemoryPressureListener provides a way for memory components to listen
+//     memory-pressure notifications and purge memory when the system is under
+//     high memory pressure. This is a 'reactive' way to reduce memory
+//     footprint.
+//
+//   - isLowEndDevice provides a way for memory components to enter a low-memory
+//     mode on low-memory devices. In the low-memory mode, the memory components
+//     are exepcted to trade off performance for memory. This is a 'proactive'
+//     way to reduce memory footprint.
+//
+//   - It is a responsibility of each memory component to decide on which one(s)
+//     the memory component should use.
+//
+//   - MemoryCoordinator provides both ways. In the future,
+//     MemoryPressureListener and isLowEndDevice will be unified into
+//     MemoryCoordinator.
 class CONTENT_EXPORT MemoryCoordinator {
  public:
   virtual ~MemoryCoordinator() {}
 
   static MemoryCoordinator* GetInstance();
 
   // Returns the memory state of the given process handle.
   virtual base::MemoryState GetStateForProcess(base::ProcessHandle handle) = 0;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_MEMORY_COORDINATOR_H_