Make DOMStorageContextWrapper a client of memory coordinator

content/browser/dom_storage/dom_storage_context_wrapper.cc

 // Copyright 2013 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.
 
 #include "content/browser/dom_storage/dom_storage_context_wrapper.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/filesystem/public/interfaces/directory.mojom.h"
 #include "components/leveldb/public/interfaces/leveldb.mojom.h"
+#include "components/memory_coordinator/browser/memory_coordinator.h"
 #include "content/browser/dom_storage/dom_storage_area.h"
 #include "content/browser/dom_storage/dom_storage_context_impl.h"
 #include "content/browser/dom_storage/dom_storage_task_runner.h"
 #include "content/browser/dom_storage/session_storage_namespace_impl.h"
 #include "content/browser/leveldb_wrapper_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/local_storage_usage_info.h"
 #include "content/public/browser/session_storage_usage_info.h"
 #include "mojo/common/common_type_converters.h"
 #include "services/file/public/cpp/constants.h"
@@ skipping 246 matching lines @@
                         : data_path.AppendASCII(kLocalStorageDirectory),
       data_path.empty() ? data_path
                         : data_path.AppendASCII(kSessionStorageDirectory),
       special_storage_policy,
       new DOMStorageWorkerPoolTaskRunner(
           worker_pool,
           worker_pool->GetNamedSequenceToken("dom_storage_primary"),
           worker_pool->GetNamedSequenceToken("dom_storage_commit"),
           BrowserThread::GetTaskRunnerForThread(BrowserThread::IO).get()));
 
-  memory_pressure_listener_.reset(new base::MemoryPressureListener(
+  if (memory_coordinator::MemoryCoordinator::GetInstance()) {
+    memory_coordinator::MemoryCoordinator::GetInstance()->RegisterClient(this);
+  } else {
+    memory_pressure_listener_.reset(new base::MemoryPressureListener(
       base::Bind(&DOMStorageContextWrapper::OnMemoryPressure, this)));
+  }
 }
 
-DOMStorageContextWrapper::~DOMStorageContextWrapper() {}
+DOMStorageContextWrapper::~DOMStorageContextWrapper() {
+  if (memory_coordinator::MemoryCoordinator::GetInstance()) {

[michaeln, 2016/09/14 01:34:41]

Register() / Unregister() need to be called on the same thread but the dtor is
not guaranteed to be called on the same thread as the ctor.

Please move the call to Unregister() into the Shutdown() method so everything
happens on the UI thread.

[hajimehoshi, 2016/09/14 11:49:47]

Done.

+    memory_coordinator::MemoryCoordinator::GetInstance()->UnregisterClient(
+        this);
+  }
+}
 
 void DOMStorageContextWrapper::GetLocalStorageUsage(
     const GetLocalStorageUsageCallback& callback) {
   DCHECK(context_.get());
   context_->task_runner()->PostShutdownBlockingTask(
       FROM_HERE, DOMStorageTaskRunner::PRIMARY_SEQUENCE,
       base::Bind(&GetLocalStorageUsageHelper,
                  base::RetainedRef(base::ThreadTaskRunnerHandle::Get()),
                  base::RetainedRef(context_), callback));
 }
@@ skipping 75 matching lines @@
 void DOMStorageContextWrapper::OpenLocalStorage(
     const url::Origin& origin,
     mojom::LevelDBObserverPtr observer,
     mojom::LevelDBWrapperRequest request) {
   mojo_state_->OpenLocalStorage(
       origin, std::move(observer), std::move(request));
 }
 
 void DOMStorageContextWrapper::OnMemoryPressure(
     base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level) {
+  PurgeMemory(base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL);

[ssid, 2016/09/13 18:29:23]

Why are we always purging aggressively here? shouldn't this pass the correct
pressure level?
Also PurgeMemory takes a boolean now right?

[hajimehoshi, 2016/09/14 11:49:48]

Done.

+}
+
+void DOMStorageContextWrapper::OnMemoryStateChange(
+    memory_coordinator::MemoryState state) {

[michaeln, 2016/09/14 01:34:41]

enum class MemoryState : int32_t {
  UNKNOWN = -1,
  NORMAL = 0,
  THROTTLED = 1,
  SUSPENDED = 2,
};

What do those states mean? Is there a .mojom or .h file somewhere that defines
describes that.

[bashi, 2016/09/14 03:33:13]

Sorry for lack of explanation. I'll add comments in .mojom to describe what they
mean.

+  PurgeMemory(state == memory_coordinator::MemoryState::SUSPENDED);

[ssid, 2016/09/13 18:29:23]

The memory coordinator document states that the "suspended" state can only be
applied to backgrounded renderer. How does a suspended browser process make
sense? The doc does not explain about browser as far as I see.

Can this method be called with "Discarded" or "Throttled"? Why do we purge
non-aggressively when called with "Discarded"?

Also why are we purging for a state of "Unthrottled"?

[hajimehoshi, 2016/09/14 11:49:47]

I realized 'suspended' state can't come to browser components so far. I added
assertions to verify this.
There is not 'Discarded' state enum value. I added code to purge memory for
'Throttled'.
Fixed.

+}
+
+void DOMStorageContextWrapper::PurgeMemory(bool aggressively)

[michaeln, 2016/09/14 01:34:41]

might be cleaner to have the helper take a DOMStorageContextImpl::PurgeOption as
input and for each of the OnMemory methods to translate to one of the three
options

[hajimehoshi, 2016/09/14 11:49:48]

I don't think that is possible since DOMStorageContext.h can't refer
DOMStorageContextImpl.h.

[michaeln, 2016/09/14 19:40:48]

Maybe i'm missing something?  This method is a private method of
DOMStorageContextWrapper, the class is not in the content/public api and the
method is not an override of a content/public api. 

So i think content/browser/dom_storage_content_wrapper.h can include
content/browser/dom_storage_content_impl.h no problem.

[hajimehoshi, 2016/09/15 09:14:46]

Sorry but I was misunderstanding. Instead of adding a helper function, I've just
changed the argument from bool to PurgeOption.

[michaeln, 2016/09/15 20:46:27]

Thnx, that's exactly what i was suggesting :) The PurgeMemory() method IS the
helper i was referring to.

+{
   DOMStorageContextImpl::PurgeOption purge_option =
       DOMStorageContextImpl::PURGE_UNOPENED;
-  if (memory_pressure_level ==
-      base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL) {
+  if (aggressively) {
     purge_option = DOMStorageContextImpl::PURGE_AGGRESSIVE;
   }
   context_->task_runner()->PostTask(
       FROM_HERE,
       base::Bind(&DOMStorageContextImpl::PurgeMemory, context_, purge_option));
 }
 
 }  // namespace content