Renderer reading side of gamepad

content/renderer/gamepad_util.cc

+// 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/renderer/gamepad_util.h"
+
+#include "base/debug/trace_event.h"
+#include "base/metrics/histogram.h"
+#include "content/common/gamepad_messages.h"
+#include "content/public/renderer/render_thread.h"
+#include "content/common/gamepad_hardware_buffer.h"
+#include "ipc/ipc_sync_message_filter.h"
+
+using namespace content;

[darin (slow to review), 2011/11/28 17:35:49]

nit: the google c++ style guide forbids 'using namespace foo'

note: i think that the contents of this file should live in the content
namespace since you are in the content/ subdir.

[scottmg, 2011/11/28 20:02:08]

Done.

+using namespace gamepad;
+
+GamepadUtil::GamepadUtil() {

[darin (slow to review), 2011/11/28 17:35:49]

please come up with a better name for this class.  GamepadUtil is overly
generic.  we have too many of these "Util" classes in chromium.

how about GamepadReader or GamepadSharedMemoryReader?

[scottmg, 2011/11/28 20:02:08]

Done.

+  memset(ever_interacted_with_, 0, sizeof(ever_interacted_with_));
+  CHECK(RenderThread::Get()->Send(new GamepadHostMsg_StartPolling(
+      &renderer_shared_memory_handle_)));
+  renderer_shared_memory_.reset(
+      new base::SharedMemory(renderer_shared_memory_handle_, true));
+  CHECK(renderer_shared_memory_->Map(sizeof(gamepad::GamepadHardwareBuffer)));
+  void *memory = renderer_shared_memory_->memory();
+  CHECK(memory);
+  gamepad_hardware_buffer_ = static_cast<GamepadHardwareBuffer*>(memory);
+}
+
+void GamepadUtil::SampleGamepads(WebKit::WebGamepads& gamepads) {
+  WebKit::WebGamepads read_into;
+  TRACE_EVENT0("GAMEPAD", "SampleGamepads");
+
+  // See gamepad_hardware_buffer.h for details on the read discipline.
+  base::subtle::Atomic32 start, end;
+  // Only try to read this many times before failing to avoid waiting here
+  // very long in case of contention with the writer. TODO(scottmg) Tune this
+  // number (as low as 1?) if histogram shows distribution as mostly
+  // 0-and-maximum.
+  const int maximum_contention_count = 10;
+  int contention_count = 0;
+  while (contention_count < maximum_contention_count) {
+    end = base::subtle::Acquire_Load(&gamepad_hardware_buffer_->end_marker);

[darin (slow to review), 2011/11/28 17:35:49]

i had expected to see a memory barrier here.  i admit, i'm not an expert on
these topics though.

i'm curious why you designed this to use Acquire_Load for the end and start
markers.  that doesn't equate to a memory barrier on x86.

[scottmg, 2011/11/28 20:02:08]

I'm afraid that's probably a bug then. I thought I'd understood the semantics of
acquire/release but perhaps I'm confused -- will revisit.

+    memcpy(&read_into, &gamepad_hardware_buffer_->buffer, sizeof(gamepads));

[darin (slow to review), 2011/11/28 17:35:49]

i'm not the expert on our dynamic annotations system, but you may want to
sprinkle some ANNOTATE_HAPPENS_{BEFORE,AFTER} macros in here.

you may also want to get this reviewed by timurrrr.  it may be wise to factor
this chunk of sensitive code out into a separate function.

[scottmg, 2011/11/28 20:02:08]

OK, will do.

+    start = base::subtle::Acquire_Load(&gamepad_hardware_buffer_->start_marker);
+    if (start == end)
+        break;
+    ++contention_count;
+    base::PlatformThread::YieldCurrentThread();
+  }
+  HISTOGRAM_COUNTS("Gamepad.ReadContentionCount", contention_count);
+
+  if (contention_count == maximum_contention_count) {
+      // We failed to successfully read, presumably because the hardware
+      // thread was taking unusually long. Don't copy the data to the output
+      // buffer, and simply leave what was there before.
+      return;
+  }
+
+  // New data was read successfully, copy it into the output buffer.
+  memcpy(&gamepads, &read_into, sizeof(gamepads));
+
+  // Override the "connected" with false until the user has interacted
+  // with the gamepad. This is to prevent fingerprinting on drive-by pages.
+  for (unsigned i = 0; i < WebKit::WebGamepads::itemsLengthCap; ++i) {
+    WebKit::WebGamepad& pad = gamepads.items[i];
+    // If the device is physically connected, then check if we should
+    // keep it disabled. We track if any of the primary 4 buttons have been
+    // pressed to determine a reasonable intentional interaction from the user.
+    if (pad.connected) {
+      if (ever_interacted_with_[i])
+        continue;
+      const unsigned primaryInteractionButtons = 4;

[darin (slow to review), 2011/11/28 17:35:49]

nit: use kFooBar for google c++ style constants.

[scottmg, 2011/11/28 20:02:08]

Done.

+      for (unsigned j = 0; j < primaryInteractionButtons; ++j)
+        ever_interacted_with_[i] |= pad.buttons[j] > 0.5f;
+      // If we've not previously set, and the user still hasn't touched
+      // these buttons, then don't pass the data on to the Chromium port.
+      if (!ever_interacted_with_[i])
+        pad.connected = false;
+    }
+  }
+}
+
+GamepadUtil::~GamepadUtil() {
+  RenderThread::Get()->Send(new GamepadHostMsg_StopPolling());

[darin (slow to review), 2011/11/28 17:35:49]

note: i'm not sure if you are doing this yet, but you'll want the browser-side
code to monitor renderer process crashes so that you can also StopPolling when a
renderer dies unexpectedly.

[scottmg, 2011/11/28 20:02:08]

After talking to John I have some other work to do on the background thread.
Thanks for the pointer, I'll make sure to do this.

+}