Mouse lock implementation, including the renderer side and the Windows version of the browser side.

Mouse lock implementation, including the renderer side and the Windows version of the browser side.

BUG=41781
TEST=Manual test in ppapi/examples/mouse_lock


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102234

[scheib, 2011-09-09 22:00:06]

I've read through all the logic - it looks pretty good, though there are areas
of the code I'm not a master of. There are a few nits mostly to help
readability.

http://codereview.chromium.org/7863003/diff/1/content/renderer/pepper_plugin_...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7863003/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl.cc:1052:
webkit::ppapi::PluginInstance* temp = mouse_lock_owner_;
nit: but perhaps rename "temp" to "last_mouse_lock_owner" or something to help
readability; also in OnMouseLockLost().

http://codereview.chromium.org/7863003/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl.cc:1564: } else {
nit: Following logic segments are for when mouse_lock_owner_ == instance. For
readability may I suggest calling "instance->" to make it clear when skimming
that this logic will only return ACK messages to the same instance that called
Lock?

http://codereview.chromium.org/7863003/diff/1/content/renderer/pepper_plugin_...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/7863003/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl.h:329: bool UsingMouseLock() const
{
nit: Consider name, "MouseLockedOrPending"?

http://codereview.chromium.org/7863003/diff/1/ppapi/examples/mouse_lock/mouse...
File ppapi/examples/mouse_lock/mouse_lock.cc (right):

http://codereview.chromium.org/7863003/diff/1/ppapi/examples/mouse_lock/mouse...
ppapi/examples/mouse_lock/mouse_lock.cc:62: if (mouse_locked_) {
I suggest also displaying movement when the mouse is not locked, it should be
valid still.

[yzshen1, 2011-09-09 23:16:04]

Thanks Vincent.

http://codereview.chromium.org/7863003/diff/1/content/renderer/pepper_plugin_...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7863003/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl.cc:1052:
webkit::ppapi::PluginInstance* temp = mouse_lock_owner_;
On 2011/09/09 22:00:06, scheib wrote:
> nit: but perhaps rename "temp" to "last_mouse_lock_owner" or something to help
> readability; also in OnMouseLockLost().

Done.

http://codereview.chromium.org/7863003/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl.cc:1564: } else {
Good idea!
On 2011/09/09 22:00:06, scheib wrote:
> nit: Following logic segments are for when mouse_lock_owner_ == instance. For
> readability may I suggest calling "instance->" to make it clear when skimming
> that this logic will only return ACK messages to the same instance that called
> Lock?

http://codereview.chromium.org/7863003/diff/1/content/renderer/pepper_plugin_...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/7863003/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl.h:329: bool UsingMouseLock() const
{
On 2011/09/09 22:00:06, scheib wrote:
> nit: Consider name, "MouseLockedOrPending"?

Done.

http://codereview.chromium.org/7863003/diff/1/ppapi/examples/mouse_lock/mouse...
File ppapi/examples/mouse_lock/mouse_lock.cc (right):

http://codereview.chromium.org/7863003/diff/1/ppapi/examples/mouse_lock/mouse...
ppapi/examples/mouse_lock/mouse_lock.cc:62: if (mouse_locked_) {
On 2011/09/09 22:00:06, scheib wrote:
> I suggest also displaying movement when the mouse is not locked, it should be
> valid still.

Done.

[yzshen1, 2011-09-13 22:51:11]

Hi, Vincent, Brett.

Please take a look. Thanks!

Vincent: make sure that the behavior conforms to the spec.
Brett: everything else.

[scheib, 2011-09-14 18:13:23]

LGTM still with one minor nit on the example.

http://codereview.chromium.org/7863003/diff/13002/ppapi/examples/mouse_lock/m...
File ppapi/examples/mouse_lock/mouse_lock.cc (right):

http://codereview.chromium.org/7863003/diff/13002/ppapi/examples/mouse_lock/m...
ppapi/examples/mouse_lock/mouse_lock.cc:69: if (mouse_locked_) {
I still think it's worth verifying that the movement graphically displays the
same when under mouse lock or not. There's no need for this conditional, right?

[brettw, 2011-09-14 22:17:57]

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.h:332: virtual bool
LockMouse();
These should probably be pure virtual and moved above with the other virtual
functions to match the existing style. Can you also add a comment here about
what the return value is?

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1528:
::ClipCursor(&rect);
Is this the behavior we want? What if the window is very small and I move the
mouse a lot? Can we just do SetCapture instead? Maybe this is the best way, I
just want to be sure.

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.h:397: int last_x_;
Can you use a gfx::Point for both of these cases? It will handle initialization
to 0 in the constructor and also remove one extra member variable. Maybe you
could even use end-of-line comments for the "Relative to..." comments and then
everything is only 2 lines.

http://codereview.chromium.org/7863003/diff/13002/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/7863003/diff/13002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:365: // |mouse_lock_owner_| is
not NULL doesn't indicate that currently the mouse
is not NULL -> being non-NULL

http://codereview.chromium.org/7863003/diff/13002/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/7863003/diff/13002/content/renderer/render_wid...
content/renderer/render_widget.cc:449: bool mouse_event_absorbed = false;
I think this would be more clear if this were called "prevent_default" or
something like that. The current name is too similar to "processed".

http://codereview.chromium.org/7863003/diff/13002/content/renderer/render_wid...
content/renderer/render_widget.cc:451: mouse_event_absorbed =
BeforeHandleMouseEvent(
Can you rename this to "WillHandleMouseEvent" which better matches the
Before/Did scheme we use in other places.

http://codereview.chromium.org/7863003/diff/13002/content/renderer/render_wid...
File content/renderer/render_widget.h (right):

http://codereview.chromium.org/7863003/diff/13002/content/renderer/render_wid...
content/renderer/render_widget.h:312: virtual bool BeforeHandleMouseEvent(const
WebKit::WebMouseEvent& event) {
Can you implement this in the .cc file? I realize some of the other functions
aren't, but this will prevent virtual function bloat (it has to be inlined in
every file that includes it).

Can you also move this to before "DidHandleMouseEvent", and mention that
DidHandleMouseEvent will not be called if you return true.

http://codereview.chromium.org/7863003/diff/13002/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/7863003/diff/13002/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:455: // Unlocks the mouse if |instance|
currently owns the mouse lock. Whenever an
I'd put a blank line above here.

[scheib, 2011-09-14 22:24:50]

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1528:
::ClipCursor(&rect);
On 2011/09/14 22:17:58, brettw wrote:
> Is this the behavior we want? What if the window is very small and I move the
> mouse a lot? Can we just do SetCapture instead? Maybe this is the best way, I
> just want to be sure.

On Linux I found that clipping the cursor was necessary beyond capture. Capture
didn't protect against a button up to button down state change, only a persisted
down.

We should re-evaluate on Windows if we haven't yet. However, I also found on
Linux that windows had to be pretty small to have an impact, e.g. less than
400x400.

[yzshen1, 2011-09-19 20:48:41]

Thanks Brett and Vincent!

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.h:332: virtual bool
LockMouse();
Good idea, thanks!

On 2011/09/14 22:17:58, brettw wrote:
> These should probably be pure virtual and moved above with the other virtual
> functions to match the existing style. Can you also add a comment here about
> what the return value is?

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1528:
::ClipCursor(&rect);
Vincent and I have discussed this issue before.

SetCapture doesn't work for this scenario because it sends out-of-window mouse
messages to the window only when the mouse button was pressed while the mouse
was over the capturing window and the button is still down.

But you are right: if the window is very small and you move the mouse a lot, you
will hit the boundaries. :) Currently, we only plan to allow this feature in
fullscreen mode, so it won't be a big problem for now.

We have considered the idea of using raw mouse input device data (WM_INPUT), in
order to avoid manipulating cursor in such a hacky way. But it comes with
another problem -- we cannot obtain post-ballistics movement easily. If we
finally decide to allow this feature in non-fullscreen mode, we could
re-consider WM_INPUT and emulate the system ballistic algorithms.


On 2011/09/14 22:17:58, brettw wrote:
> Is this the behavior we want? What if the window is very small and I move the
> mouse a lot? Can we just do SetCapture instead? Maybe this is the best way, I
> just want to be sure.

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/7863003/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.h:397: int last_x_;
On 2011/09/14 22:17:58, brettw wrote:
> Can you use a gfx::Point for both of these cases? It will handle
initialization
> to 0 in the constructor and also remove one extra member variable. Maybe you
> could even use end-of-line comments for the "Relative to..." comments and then
> everything is only 2 lines.

Done. (Didn't use end-of-line comments because I cannot fit them in the same
line as the definitions now.)

http://codereview.chromium.org/7863003/diff/13002/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/7863003/diff/13002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:365: // |mouse_lock_owner_| is
not NULL doesn't indicate that currently the mouse
On 2011/09/14 22:17:58, brettw wrote:
> is not NULL -> being non-NULL

Done.

http://codereview.chromium.org/7863003/diff/13002/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/7863003/diff/13002/content/renderer/render_wid...
content/renderer/render_widget.cc:449: bool mouse_event_absorbed = false;
On 2011/09/14 22:17:58, brettw wrote:
> I think this would be more clear if this were called "prevent_default" or
> something like that. The current name is too similar to "processed".

Done. It looks much better.

http://codereview.chromium.org/7863003/diff/13002/content/renderer/render_wid...
content/renderer/render_widget.cc:451: mouse_event_absorbed =
BeforeHandleMouseEvent(
On 2011/09/14 22:17:58, brettw wrote:
> Can you rename this to "WillHandleMouseEvent" which better matches the
> Before/Did scheme we use in other places.

Done.

http://codereview.chromium.org/7863003/diff/13002/content/renderer/render_wid...
File content/renderer/render_widget.h (right):

http://codereview.chromium.org/7863003/diff/13002/content/renderer/render_wid...
content/renderer/render_widget.h:312: virtual bool BeforeHandleMouseEvent(const
WebKit::WebMouseEvent& event) {
On 2011/09/14 22:17:58, brettw wrote:
> Can you implement this in the .cc file? I realize some of the other functions
> aren't, but this will prevent virtual function bloat (it has to be inlined in
> every file that includes it).
> 
> Can you also move this to before "DidHandleMouseEvent", and mention that
> DidHandleMouseEvent will not be called if you return true.

Done. Thanks!

http://codereview.chromium.org/7863003/diff/13002/ppapi/examples/mouse_lock/m...
File ppapi/examples/mouse_lock/mouse_lock.cc (right):

http://codereview.chromium.org/7863003/diff/13002/ppapi/examples/mouse_lock/m...
ppapi/examples/mouse_lock/mouse_lock.cc:69: if (mouse_locked_) {
On 2011/09/14 18:13:23, scheib wrote:
> I still think it's worth verifying that the movement graphically displays the
> same when under mouse lock or not. There's no need for this conditional,
right?

Done. I make the testing plugin also show mouse movement when the mouse is not
locked. (But it doesn't work currently because it depends on the change in
WebKit.)

http://codereview.chromium.org/7863003/diff/13002/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/7863003/diff/13002/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:455: // Unlocks the mouse if |instance|
currently owns the mouse lock. Whenever an
On 2011/09/14 22:17:58, brettw wrote:
> I'd put a blank line above here.

Done.

[brettw, 2011-09-20 05:02:41]

LGTM, I mostly did a high-level review

http://codereview.chromium.org/7863003/diff/27001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/7863003/diff/27001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.h:345:
RenderWidgetHostView() : popup_type_(WebKit::WebPopupTypeNone),
Would you mind fixing the style here and moving this to the .cc file since
you're touching it?

[commit-bot: I haz the power, 2011-09-22 03:36:39]

Change committed as 102234