Convert Gamepad IPC messages into mojo interface.

content/browser/renderer_host/gamepad_monitor.cc

Index: content/browser/renderer_host/gamepad_monitor.cc
diff --git a/content/browser/renderer_host/gamepad_monitor.cc b/content/browser/renderer_host/gamepad_monitor.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4234557d132d532bf2b8ec557add185fdc9b8387
--- /dev/null
+++ b/content/browser/renderer_host/gamepad_monitor.cc
@@ -0,0 +1,71 @@
+// Copyright (c) 2011 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/renderer_host/gamepad_monitor.h"
+
+#include "base/memory/shared_memory.h"
+#include "content/browser/gamepad/gamepad_service.h"
+#include "content/common/gamepad_hardware_buffer.h"
+#include "content/public/browser/browser_thread.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "mojo/public/cpp/system/platform_handle.h"
+
+namespace content {
+
+GamepadMonitor::GamepadMonitor() : is_started_(false) {}
+
+GamepadMonitor::~GamepadMonitor() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (is_started_)
+    GamepadService::GetInstance()->RemoveConsumer(this);
+}
+
+// static
+void GamepadMonitor::Create(device::mojom::GamepadMonitorRequest request) {
+  mojo::MakeStrongBinding(base::MakeUnique<GamepadMonitor>(),
+                          std::move(request));
+}
+
+void GamepadMonitor::OnGamepadConnected(unsigned index,
+                                        const blink::WebGamepad& gamepad) {
+  if (gamepad_observer_)
+    gamepad_observer_->GamepadConnected(index, gamepad);
+}
+
+void GamepadMonitor::OnGamepadDisconnected(unsigned index,
+                                           const blink::WebGamepad& gamepad) {
+  if (gamepad_observer_)
+    gamepad_observer_->GamepadDisconnected(index, gamepad);
+}
+
+void GamepadMonitor::GamepadStartPolling(
+    const GamepadStartPollingCallback& callback) {
+  GamepadService* service = GamepadService::GetInstance();
+  DCHECK(!is_started_);
+  if (is_started_)
+    return;
+
+  is_started_ = true;
+  service->ConsumerBecameActive(this);
+  base::SharedMemoryHandle renderer_handle =
+      service->GetSharedMemoryHandleForProcess(base::GetCurrentProcessHandle());

[blundell, 2016/11/29 15:36:24]

I don't think that this is the same as what the code was doing before, was it?
This is returning the handle for *this process* (i.e., the browser process),
whereas BrowserMessageFilter::PeerHandle() is returning a handle for the
renderer process associated with that message filter. Unless I'm
misunderstanding how one or both of those methods work?

[ke.he, 2016/11/30 05:45:11]

The logic is here on purpose.
In the original base::sharedMemory mechanism, we need to set the PeerHandle to
specify the target process we want to share memory with. But in mojo's
sharedMemory mechanism, we don't need to specify the target process, because the
shared memory handle is passed through mojo channel, mojo already knows who is
the right target process.
The most straight forward way here is to add a new memory function
"GetSharedMemoryHandle()" in GameService, the "GetSharedMemoryHandle()" is
similar to "GetSharedMemoryHandleForProcess(peerHandle)" but the former doesn't
set PeerHandle. Then we wrap the base::sharedMemoryHandle into a
mojo::ScopedSharedMemoryHandle, and pass it to render process by mojo call.
But I didn't add a new "GetSharedMemoryHandle()" since we found if we pass
"current process handle" to GetSharedMemoryHandleForProcess(), the effect is
almost the same. I feel it make sense for GetSharedMemoryHandleForProcess() that
"set current process as peerHandle" means "doesn't specify the target process".
What do you think about this?  Thanks very much. 

[blundell, 2016/12/01 16:01:26]

OK, this makes sense, thanks. The key is that mojo::UnwrapSharedMemoryHandle()
creates a new PlatformHandle with the target process proc ID (this is what you
were getting at in your above explanation).

To make the fact that we don't care about the target process more explicit, I
would do the following:

- Change the naming to GamepadService::DuplicateHandle() (and similarly for the
other methods with the same name)
- Change the impl to call
shared_memory()->DuplicateHandle(shared_memory()->handle()).

This should have the same effect while avoiding the oddness of looking like
we're asking to share to the browser process :), instead being explicit that we
only care about giving the client a handle to the shared memory.

I would also file a TODO() and create a bug for moving GamepadSharedBufferImpl
to using a Mojo buffer rather than base::SharedMemory. Then we could hide all of
this manipulation entirely.

[blundell, 2016/12/01 16:04:39]

Sorry, the GamepadService method name should probably be something like
GetSharedMemoryHandle() instead; DuplicateHandle() is too low-level of a name.
This is more-or-less what you were suggesting as an option I think. I think it's
the way to go.

[ke.he, 2016/12/02 13:20:47]

blundell@, Thanks! 
1) Add a new GetSharedMemoryHandle() alongside the
GetSharedMemoryHandleForProcess(). --->done.
2) Create bug and add TODO() for "Using a Mojo buffer rather than
base::SharedMemory". ---> I create an issue
https://bugs.chromium.org/p/chromium/issues/detail?id=670655 for this. By the
way, I don't have the permission of editing issues and trigger DryRun. May I
apply? Thanks.

+  callback.Run(mojo::WrapSharedMemoryHandle(
+      renderer_handle, sizeof(GamepadHardwareBuffer), true /* read_only */));
+}
+
+void GamepadMonitor::GamepadStopPolling() {
+  DCHECK(is_started_);
+  if (!is_started_)
+    return;
+
+  is_started_ = false;
+  GamepadService::GetInstance()->ConsumerBecameInactive(this);
+}
+
+void GamepadMonitor::SetObserver(
+    device::mojom::GamepadObserverPtr gamepad_observer) {
+  gamepad_observer_ = std::move(gamepad_observer);
+}
+
+}  // namespace content