Only allow to lock the mouse when the tab is in fullscreen mode.

Only allow to lock the mouse when the tab is in fullscreen mode.

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


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

[yzshen1, 2011-09-28 21:55:03]

Hi, Scott, James, Vincent.

Would you please review this change? Thanks!

Scott: Everything in chrome/ and content/.
James: Please take a look at the change in Browser::ToggleFullscreenModeForTab.
Vincent: Please make sure this conforms to the current spec, and also take a
look at mouse_lock.html.

Thanks!

[scheib, 2011-09-28 22:48:31]

LGTM

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

http://codereview.chromium.org/8072011/diff/1/ppapi/examples/mouse_lock/mouse...
ppapi/examples/mouse_lock/mouse_lock.html:56: <li>right click; or</li>
Consider for this change, or a future one: I suggest we remove the "right click
to exit" feature of the example. We want to be able to test that right click
doesn't break mouse lock.

[sky, 2011-09-28 22:58:04]

http://codereview.chromium.org/8072011/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8072011/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:3810: tab_caused_fullscreen_ = true;
Does this need to be reset for the if (!enter_fullscreen) case?

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_view_host.h:432: virtual bool
HasMouseLockPermission() const;
OVERRIDE

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_widget_host.cc:385: void
RenderWidgetHost::Blur() {
What if the user minimizes the window when you have mouse lock?

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_widget_host.h:473: virtual bool
HasMouseLockPermission() const;
Add a real description. Also, what about CanLockMouse?

[koz (OOO until 15th September), 2011-09-29 04:13:07]

browser.h/cc changes LGTM

http://codereview.chromium.org/8072011/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8072011/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:3810: tab_caused_fullscreen_ = true;
On 2011/09/28 22:58:04, sky wrote:
> Does this need to be reset for the if (!enter_fullscreen) case?

It should get reset by the call to Toggle{Presentation,Fullscreen}Mode() below.

[yzshen1, 2011-09-29 20:41:04]

Thanks!

Please take another look.

http://codereview.chromium.org/8072011/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8072011/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:3810: tab_caused_fullscreen_ = true;
On 2011/09/29 04:13:07, koz wrote:
> On 2011/09/28 22:58:04, sky wrote:
> > Does this need to be reset for the if (!enter_fullscreen) case?
> 
> It should get reset by the call to Toggle{Presentation,Fullscreen}Mode()
below.

Yes. If currently fullscreen mode is on, Toggle{Presentation,Fullscreen}Mode()
will cause NotifyTabOfFullscreenExitIfNecessary() to be called, which resets
|tab_caused_fullscreen_|.

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_view_host.h:432: virtual bool
HasMouseLockPermission() const;
On 2011/09/28 22:58:04, sky wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_widget_host.cc:385: void
RenderWidgetHost::Blur() {
On 2011/09/28 22:58:04, sky wrote:
> What if the user minimizes the window when you have mouse lock?

Chrome will lose focus so it exits the mouse lock mode.
(I haven't tested platforms other than Windows, though. Because support for
mouse lock on other platforms hasn't been checked in yet.)

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/8072011/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_widget_host.h:473: virtual bool
HasMouseLockPermission() const;
On 2011/09/28 22:58:04, sky wrote:
> Add a real description. Also, what about CanLockMouse?

Done.

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

http://codereview.chromium.org/8072011/diff/1/ppapi/examples/mouse_lock/mouse...
ppapi/examples/mouse_lock/mouse_lock.html:56: <li>right click; or</li>
On 2011/09/28 22:48:32, scheib wrote:
> Consider for this change, or a future one: I suggest we remove the "right
click
> to exit" feature of the example. We want to be able to test that right click
> doesn't break mouse lock.

I would like to have a way to exit by mouse click, in order to verify that mouse
click events are sent correctly in the mouse lock mode. I have changed to use
the middle click.

[sky, 2011-09-29 21:17:34]

LGTM

[commit-bot: I haz the power, 2011-09-30 17:05:21]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/8072011/7001

[commit-bot: I haz the power, 2011-09-30 17:05:27]

Presubmit check for 8072011-7001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
ppapi/examples/mouse_lock/mouse_lock.cc,ppapi/examples/mouse_lock/mouse_lock.html

Presubmit checks took 1.9s to calculate.

[yzshen1, 2011-09-30 17:08:37]

On 2011/09/30 17:05:27, I haz the power (commit-bot) wrote:
> Presubmit check for 8072011-7001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
>
ppapi/examples/mouse_lock/mouse_lock.cc,ppapi/examples/mouse_lock/mouse_lock.html
> 
> Presubmit checks took 1.9s to calculate.

Hi, Brett.
Would you please do an OWNER review for mouse_lock.html?
Thanks!

[brettw, 2011-09-30 23:32:22]

PPAPI owners rubberstamp LGTM

[commit-bot: I haz the power, 2011-09-30 23:34:57]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/8072011/7001

[commit-bot: I haz the power, 2011-10-01 01:38:14]

Change committed as 103612