Fix mouse lock perf issue on Windows.

Fix mouse lock perf issue on Windows.

Don't move the cursor back to the center of the view unless it approaches the border.

BUG=106054
TEST=None


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

[yzshen1, 2011-12-03 00:17:30]

Hi, Brett and Vincent.

Please take a look at this CL. Thanks!

[scheib, 2011-12-03 04:13:00]

LGTM, but I do have a paranoid question:

http://codereview.chromium.org/8791001/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8791001/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_win.cc:2417: if
(LOWORD(lparam) == center.x && HIWORD(lparam) == center.y) {
Perhaps I'm being paranoid, but: consider caching the target point of the mouse
move and using that as an exit condition of ignoring mouse movements.
Daydreaming the scenario: If the window rect moves we'd be stuck. In the future
if we take out full screen requirement and JS is moving a window around while
mouse lock is active...

[scheib, 2011-12-03 04:14:04]

Oh, and, obviously you tried manually testing that performance was improved? How
well did it do? E.g. can we get a sample rate of mouse events when locked and
unlocked?

[yzshen1, 2011-12-05 01:16:42]

> Oh, and, obviously you tried manually testing that performance was improved? 
> How well did it do? E.g. can we get a sample rate of mouse events when locked
and
> unlocked?

Yes, I have done some manual tests.
Roughly, this change doubles the mouse move events in mouse lock mode. This is
reasonable: previously we force the cursor back to the center after every move,
and eat the resulting mouse move message.

http://codereview.chromium.org/8791001/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8791001/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_win.cc:2417: if
(LOWORD(lparam) == center.x && HIWORD(lparam) == center.y) {
On 2011/12/03 04:13:01, scheib wrote:
> Perhaps I'm being paranoid, but: consider caching the target point of the
mouse
> move and using that as an exit condition of ignoring mouse movements.
> Daydreaming the scenario: If the window rect moves we'd be stuck. In the
future
> if we take out full screen requirement and JS is moving a window around while
> mouse lock is active...

I have also considered this scenario. But it won't get stuck: we don't discard
*every* message when |ignore_move_to_center_message_| is true; instead, we only
discard one mouse move message.

If the window is moved between MoveCursorToCenterIfNecessary() and receiving the
corresponding mouse move message, what will happen is:
1) the move-to-center message is used to compute mouse movement value, and may
result in a spike.
2) |ignore_move_to_center_message_| is not changed to false, so in the future,
it may absorb a 'normal' mouse move message, which happens to be at the center
of the view.

I think it is rare, and not a big deal. But your approach seems better. I have
made the change. Thanks!

[scheib, 2011-12-05 19:38:29]

http://codereview.chromium.org/8791001/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8791001/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_win.cc:2420: if
(move_to_center_request_.target.x() == center.x &&
;X Am I confused, or should this test be something like 

if (LOWORD(lparam) == move_to_center_request_.target.x() && ...

[yzshen1, 2011-12-05 20:05:35]

http://codereview.chromium.org/8791001/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8791001/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_win.cc:2420: if
(move_to_center_request_.target.x() == center.x &&
Sorry. I didn't change the variable name, which caused confusion.

We need to convert the position of the WM_MOUSEMOVE message to screen
coordinates.
On 2011/12/05 19:38:29, scheib wrote:
> ;X Am I confused, or should this test be something like 
> 
> if (LOWORD(lparam) == move_to_center_request_.target.x() && ...

[brettw, 2011-12-05 20:15:22]

lgtm

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

http://codereview.chromium.org/8791001/diff/11001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:92: // it
approaches the border. |kMouseLockBorderPercentage| specifies the width of
Can this comment also mention why we don't always move the mouse to the center?

[scheib, 2011-12-05 20:34:37]

LGTM

Though I think you should rename the center variable. ;)

[yzshen1, 2011-12-05 20:36:43]

Thanks!

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

http://codereview.chromium.org/8791001/diff/11001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:92: // it
approaches the border. |kMouseLockBorderPercentage| specifies the width of
On 2011/12/05 20:15:23, brettw wrote:
> Can this comment also mention why we don't always move the mouse to the
center?

Done.

[yzshen1, 2011-12-05 20:37:16]

On 2011/12/05 20:34:37, scheib wrote:
> LGTM
> 
> Though I think you should rename the center variable. ;)

Already did. Thanks for catching this! :)

[commit-bot: I haz the power, 2011-12-05 20:43:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/8791001/15001

[commit-bot: I haz the power, 2011-12-05 21:15:01]

Try job failure for 8791001-15001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-12-05 21:51:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/8791001/15001

[commit-bot: I haz the power, 2011-12-05 23:37:17]

Change committed as 113061