discardable_memory: Using mojo IPC to replace Chrome IPC

components/discardable_memory/client/client_discardable_shared_memory_manager.h

 // Copyright 2014 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 COMPONENTS_DISCARDABLE_MEMORY_CLIENT_CLIENT_DISCARDABLE_SHARED_MEMORY_MANAGER_H_
 #define COMPONENTS_DISCARDABLE_MEMORY_CLIENT_CLIENT_DISCARDABLE_SHARED_MEMORY_MANAGER_H_
 
 #include <stddef.h>
 
 #include "base/macros.h"
 #include "base/memory/discardable_memory_allocator.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/shared_memory_handle.h"
 #include "base/synchronization/lock.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "components/discardable_memory/common/discardable_memory_export.h"
 #include "components/discardable_memory/common/discardable_shared_memory_heap.h"
-#include "components/discardable_memory/common/discardable_shared_memory_id.h"
+#include "components/discardable_memory/public/interfaces/discardable_shared_memory_manager.mojom.h"
+
+namespace base {
+class SingleThreadTaskRunner;
+}
 
 namespace discardable_memory {
 
 // Implementation of DiscardableMemoryAllocator that allocates
 // discardable memory segments through the browser process.
 class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
     : public base::DiscardableMemoryAllocator,
       public base::trace_event::MemoryDumpProvider {
  public:
-  class Delegate {
-   public:
-    virtual void AllocateLockedDiscardableSharedMemory(
-        size_t size,
-        DiscardableSharedMemoryId id,
-        base::SharedMemoryHandle* handle) = 0;
-    virtual void DeletedDiscardableSharedMemory(
-        DiscardableSharedMemoryId id) = 0;
-
-   protected:
-    virtual ~Delegate() {}
-  };
-
-  explicit ClientDiscardableSharedMemoryManager(Delegate* delegate);
+  ClientDiscardableSharedMemoryManager(
+      mojom::DiscardableSharedMemoryManagerPtrInfo info,
+      base::SingleThreadTaskRunner* io_task_runner);
   ~ClientDiscardableSharedMemoryManager() override;
 
   // Overridden from base::DiscardableMemoryAllocator:
   std::unique_ptr<base::DiscardableMemory> AllocateLockedDiscardableMemory(
       size_t size) override;
 
   // Overridden from base::trace_event::MemoryDumpProvider:
   bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
                     base::trace_event::ProcessMemoryDump* pmd) override;
 
@@ skipping 11 matching lines @@
 
   struct Statistics {
     size_t total_size;
     size_t freelist_size;
   };
 
   Statistics GetStatistics() const;
 
  private:
   std::unique_ptr<base::DiscardableSharedMemory>
-  AllocateLockedDiscardableSharedMemory(size_t size,
-                                        DiscardableSharedMemoryId id);
+  AllocateLockedDiscardableSharedMemory(size_t size, int32_t id);
+  void AllocateOnIOThread(

[reveman, 2016/11/18 05:26:50]

nit: s/AllocateOnIOThread/AllocateOnIO/ as it's common practice in chromium code
to just suffix functions that are supposed to execute on IO or UI threads simply
with OnIO/OnUI. See content/browser/gpu/browser_gpu_memory_buffer_manager.h for
an example.

[Peng, 2016/11/18 16:28:28]

Done.

+      size_t size,
+      int32_t id,
+      std::unique_ptr<base::DiscardableSharedMemory>* memory,
+      base::WaitableEvent* event);
+  void OnAllocateCompletedOnIOThread(

[reveman, 2016/11/18 05:26:50]

nit: too many "On" :) I don't think we need the On prefix here. I'd prefer
AllocatedOnIO or AllocateCompletedOnIO

[Peng, 2016/11/18 16:28:28]

Done.

+      std::unique_ptr<base::DiscardableSharedMemory>* memory,
+      base::WaitableEvent* event,
+      mojo::ScopedSharedBufferHandle mojo_handle);
+
+  void DeletedDiscardableSharedMemory(int32_t id);
   void MemoryUsageChanged(size_t new_bytes_allocated,
                           size_t new_bytes_free) const;
 
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
+  std::unique_ptr<mojom::DiscardableSharedMemoryManagerPtr> manager_;

[reveman, 2016/11/18 05:26:50]

Can we rename this variable to have it be less confusing to have manager
instance with a manager member? manager_ipc_, manager_mojo_, just ipc_ or mojo_?
Something that allows language referring to this member to be less confusing.
Currently you have comments mentioning "manager" and it's not perfectly clear if
it refers to the instance of this class or this manager_ member. 

[Peng, 2016/11/18 16:28:28]

Done.

+
   mutable base::Lock lock_;
-  DiscardableSharedMemoryHeap heap_;
-  Delegate* const delegate_;
+  std::unique_ptr<DiscardableSharedMemoryHeap> heap_;

[reveman, 2016/11/18 05:26:50]

why std::unique_ptr instead of allocated as part of the class as before?

[Peng, 2016/11/18 16:28:28]

The destructor of |heap_| may call DeletedDiscardableSharedMemory (it posts task
to IO thread) of this class, and in destructor of CDSMM we will post a task to
delete |manager_| on IO thread.
So we want to delete |heap_| before posting the task for deleting |manager_|.

[reveman, 2016/11/18 21:55:25]

Got it. Can you add a comment where you reset this to make it clear that it
needs to be done before the mojo part is deleted?

 
   DISALLOW_COPY_AND_ASSIGN(ClientDiscardableSharedMemoryManager);
 };
 
 }  // namespace discardable_memory
 
 #endif  // COMPONENTS_DISCARDABLE_MEMORY_CLIENT_CLIENT_DISCARDABLE_SHARED_MEMORY_MANAGER_H_