refactoring mouse lock to support pepper and WebKit

content/renderer/mouse_lock_dispatcher.cc

+// Copyright (c) 2012 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/mouse_lock_dispatcher.h"
+
+#include "content/common/view_messages.h"
+#include "content/renderer/render_view_impl.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebInputEvent.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebWidget.h"
+
+MouseLockDispatcher::MouseLockDispatcher(RenderViewImpl* render_view_impl)
+    : content::RenderViewObserver(render_view_impl),
+      render_view_impl_(render_view_impl),
+      mouse_locked_(false),
+      pending_lock_request_(false),
+      pending_unlock_request_(false),
+      target_(NULL) {
+}
+
+MouseLockDispatcher::~MouseLockDispatcher() {
+}
+
+namespace {

[yzshen1, 2012/01/24 18:56:08]

It is better to put it on top of the file (and below those includes).

[scheib, 2012/01/25 00:27:11]

Done.

+
+class PluginInstanceLockTarget : public MouseLockDispatcher::LockTarget {
+ public:
+  PluginInstanceLockTarget(webkit::ppapi::PluginInstance* plugin)
+  : plugin_(plugin) { }

[piman, 2012/01/24 02:27:37]

nit: indentation before ':' should be +4 (6 total)

[yzshen1, 2012/01/24 18:56:08]

nit: I think we don't put a space between {}.
(and also line 51)

[scheib, 2012/01/25 00:27:11]

Done.

+
+  virtual void OnLockMouseACK(bool succeeded) OVERRIDE {
+    plugin_->OnLockMouseACK(succeeded);
+  }
+
+  virtual void OnMouseLockLost() OVERRIDE {
+    plugin_->OnMouseLockLost();
+  }
+
+  virtual bool HandleMouseLockedInputEvent(
+      const WebKit::WebMouseEvent &event) OVERRIDE {
+    plugin_->HandleMouseLockedInputEvent(event);
+    return true;
+  }
+
+  webkit::ppapi::PluginInstance* plugin_;
+};
+
+class WebWidgetLockTarget : public MouseLockDispatcher::LockTarget {
+ public:
+  WebWidgetLockTarget (WebKit::WebWidget* webwidget)
+  : webwidget_(webwidget) { }

[piman, 2012/01/24 02:27:37]

nit: indentation before ':' should be +4 (6 total)

[scheib, 2012/01/25 00:27:11]

Done.

+
+  virtual void OnLockMouseACK(bool succeeded) OVERRIDE {
+    // TODO(scheib): Enable after WebKit pointer lock API has landed.
+    // if (succeeded)
+    //   webwidget_->didAcquirePointerLock();
+    // else
+    //   webwidget_->didNotAcquirePointerLock();
+  }
+
+  virtual void OnMouseLockLost() OVERRIDE {
+    // TODO(scheib): Enable after WebKit pointer lock API has landed.
+    // webwidget_->didLosePointerLock();
+  }
+
+  virtual bool HandleMouseLockedInputEvent(
+      const WebKit::WebMouseEvent &event) OVERRIDE {
+    // The WebWidget handles mouse lock in WebKit's handleInputEvent().
+    return false;
+  }
+
+  WebKit::WebWidget* webwidget_;
+};

[piman, 2012/01/24 02:27:37]

High level comment: do these 2 classes need to be defined here, and only
accessible through the factory method below?
I'm thinking, RenderViewImpl could implement MouseLockDispatcher::LockTarget
(effectively these), and pass 'this' as the lock target.
I guess it doesn't save you much for the Pepper instance case, except maybe a
little bit of locality (in terms of new/delete for example). IMO It makes it
easier to convince oneself that PluginInstanceLockTarget won't outlive the
PluginInstance.

I don't feel strongly about this, so feel free to ignore if you think this is
better.

[scheib, 2012/01/25 00:27:11]

Done.

+
+}  // namespace
+
+// static
+MouseLockDispatcher::LockTarget* MouseLockDispatcher::CreateLockTarget(
+    webkit::ppapi::PluginInstance* plugin) {
+  return new PluginInstanceLockTarget(plugin);
+}
+
+// static
+MouseLockDispatcher::LockTarget* MouseLockDispatcher::CreateLockTarget(
+    WebKit::WebWidget* webwidget) {
+  return new WebWidgetLockTarget(webwidget);
+}
+
+bool MouseLockDispatcher::LockMouse(LockTarget* target) {
+  if (MouseLockedOrPendingAction())
+    return false;
+
+  pending_lock_request_ = true;
+  target_ = target;
+
+  Send(new ViewHostMsg_LockMouse(routing_id()));
+  return true;
+}
+
+void MouseLockDispatcher::UnlockMouse(LockTarget* target) {
+  if (target == target_ && !pending_unlock_request_) {

[piman, 2012/01/24 02:27:37]

When comparing pointers to compare identity (here and below), I'm always asking
myself - is there any way that we get into a situation where the instance is
destroyed, and another one gets allocated and gets the same address?
I think in this case we rely on the fact that target_ is always set to NULL
before the instance is destroyed, but it's worth a comment I think.

[yzshen1, 2012/01/24 18:56:08]

Please also check !NULL. Otherwise, an unlock request will be sent if currently
no one owns the lock and |target|==NULL.

[scheib, 2012/01/25 00:27:11]

It's a good consideration. But to add comments everywhere pointers are compared
would really bloat out the code. There's a lot incorrect behavior possible if
stale pointers are left and I don't see this concern being any more unusual or
prominent than the rest. I can add comments if you like but I think it will harm
readability.

[scheib, 2012/01/25 00:27:11]

Done.

+    pending_unlock_request_ = true;
+    Send(new ViewHostMsg_UnlockMouse(routing_id()));
+  }
+}
+
+void MouseLockDispatcher::UnlockMouseAndClearTarget(LockTarget* target) {

[yzshen1, 2012/01/24 18:56:08]

It is a little bit confusing: UnlockMouse() will also clear target. The only
difference is that it clears the target later.
Maybe call it something like OnLockTargetDestroyed()?

[scheib, 2012/01/25 00:27:11]

Done.

+  if (target == target_) {
+    UnlockMouse(target);
+    target_ = NULL;
+  }
+}
+
+bool MouseLockDispatcher::IsMouseLockedTo(LockTarget* target) {
+  return mouse_locked_ && target_ == target;
+}
+
+bool MouseLockDispatcher::WillHandleMouseEvent(
+    const WebKit::WebMouseEvent& event) {
+  if (mouse_locked_ && target_)
+    return target_->HandleMouseLockedInputEvent(event);
+  return false;
+}
+
+bool MouseLockDispatcher::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(MouseLockDispatcher, message)
+    IPC_MESSAGE_HANDLER(ViewMsg_LockMouse_ACK, OnLockMouseACK)
+    IPC_MESSAGE_HANDLER(ViewMsg_MouseLockLost, OnMouseLockLost)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void MouseLockDispatcher::OnLockMouseACK(bool succeeded) {
+  DCHECK(!mouse_locked_ && pending_lock_request_);
+
+  mouse_locked_ = succeeded;
+  pending_lock_request_ = false;
+  if (pending_unlock_request_ && !succeeded) {
+    // We have sent an unlock request after the lock request. However, since
+    // the lock request has failed, the unlock request will be ignored by the
+    // browser side and there won't be any response to it.
+    pending_unlock_request_ = false;
+  }
+
+  LockTarget* last_target = target_;
+  if (!succeeded)
+    target_ = NULL;
+
+  // Callbacks made after all state modification to prevent reentrant errors
+  // such as OnLockMouseACK() synchronously calling LockMouse().
+
+  if (last_target)
+    last_target->OnLockMouseACK(succeeded);
+
+  // Mouse Lock removes the system cursor and provides all mouse motion as
+  // .movementX/Y values on events all sent to a fixed target. This requires
+  // content to specifically request the mode to be entered.
+  // Mouse Capture is implicitly given for the duration of a drag event, and
+  // sends all mouse events to the initial target of the drag.
+  // If Lock is entered it supercedes any in progress Capture.
+  if (succeeded && render_view_impl_->webwidget())
+      render_view_impl_->webwidget()->mouseCaptureLost();
+}
+
+void MouseLockDispatcher::OnMouseLockLost() {
+  DCHECK(mouse_locked_ && !pending_lock_request_);
+
+  mouse_locked_ = false;
+  pending_unlock_request_ = false;
+
+  LockTarget* last_target = target_;
+  target_ = NULL;
+
+  // Callbacks made after all state modification to prevent reentrant errors
+  // such as OnMouseLockLost() synchronously calling LockMouse().
+
+  if (last_target)
+    last_target->OnMouseLockLost();
+}
+