discardable_memory: Using mojo IPC to replace Chrome IPC

components/discardable_memory/client/client_discardable_shared_memory_manager.h

Index: components/discardable_memory/client/client_discardable_shared_memory_manager.h
diff --git a/components/discardable_memory/client/client_discardable_shared_memory_manager.h b/components/discardable_memory/client/client_discardable_shared_memory_manager.h
index 76215bc099f65ccd6cd36299f09ea531b29b1a4a..652bca4fb2ce4c36094a1fdef53c104fd69b7bfc 100644
--- a/components/discardable_memory/client/client_discardable_shared_memory_manager.h
+++ b/components/discardable_memory/client/client_discardable_shared_memory_manager.h
@@ -15,7 +15,11 @@
 #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 {
 
@@ -25,20 +29,9 @@ 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:
@@ -70,14 +63,26 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
 
  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);
 };