Windowed mode mouse lock addded to fullscreen controller.

Windowed mode mouse lock addded to fullscreen controller.

Allows mouse lock to be entered outside of fullscreen.

The root changes are in 
chrome/browser/ui/fullscreen_controller

They require the user interface bubble to be more tolerant of state changes. Three versions of that code are modified:
Views:
chrome/browser/ui/views/frame/browser_view.cc
GTK:
chrome/browser/ui/gtk/browser_window_gtk.cc
Mac:
chrome/browser/ui/cocoa/...

Testing is expanded in:
chrome/browser/ui/browser_browsertest.cc

BUG=107013
TEST=Entering and exiting of mouse lock and fullscreen. chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138150

[scheib, 2012-04-30 17:52:34]

Nico, could you look over the cocoa changes in this patch?

[yzshen1, 2012-04-30 19:32:40]

First round of comments for fullscreen_controller.{h,cc}

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:72:
base::Bind(&FullscreenController::NotifyMouseLockChange, this));
Is it sufficient to send chrome::NOTIFICATION_MOUSE_LOCK_CHANGED when
mouse_lock_state_ changes?

If that is true, the code can be simplified a great deal, and it is more clear
when such a notification gets sent.
(You can replace mouse_lock_state_ = ... statements with SetMouseLockState() and
send notification in it.)

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:87: mouse_lock_state_ =
MOUSELOCK_REQUESTED;
Please add more comment about why we need to merge (click is required so we
cannot go into mouselock, etc.)

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:88: break;
You could use an "else" on line 89 instead of two breaks in this case.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:95: mouse_lock_tab_ = NULL;
Shall we explicitly set mouse_lock_state_ to not requested in this case?

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:99: mouse_lock_tab_ = NULL;
Shall we set mouse_lock_state_ to not requested in this case?

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:276:
UpdateFullscreenExitBubbleContent();
nit: It is previously in the "if (mouse_lock)" block because
ExitTabbedFullscreenModeIfNecessary() will update bubble content if necessary.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:301:
ExitTabbedFullscreenModeIfNecessary();
I don't get it: in windowed mouse lock mode, why do we need to
ExitTabbedFullscreenModeIfNecessary()? Maybe we should change the name/meaning
of the method?

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:326:
web_contents->GotResponseToLockMouseRequest(false);
If in ACCEPTED mode, we shouldn't call GotResponseToLockMouseRequest, right?

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:347: else if (mouse_lock_tab_)
Is this order right?
Consider the following two sequences:
1) enter extension-triggered fullscreen -> enter mouse lock
2) enter mouse lock -> enter extension-triggered fullscreen

in these two cases, which URL shall we show? the URL of the extension or the
mouse lock tab?

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:354: && IsMouseLocked()
&& at the end of the previous line, please.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:356:
mouse_lock_tab_->web_contents()->GotResponseToLockMouseRequest(false);
GotResponseToLockMouseRequest() should only be called when REQUESTED. At least
that is the original design, maybe you want to do what line 318 does, or change
the meaning/name of GotResponseToLockMouseRequest. (It is good to unify the
behavior with line 318 as well.)

Besides, you should change mouse_lock_state_ and mouse_lock_tab_ here.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:396: return
FEB_TYPE_BROWSER_EXTENSION_FULLSCREEN_EXIT_INSTRUCTION;
Here: what if the mouse lock is requested, etc.?

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:453: in_or_entering_fullscreen_ =
!window_->IsFullscreen();
Is it possible that there are paths to change fullscreen state that don't go
through TogglePresentationModeInternal/ToggleFullscreenModeInternal?
(I think there are. Say, tab is closed.)

In that case, in_or_entering_fullscreen_ doesn't reflect the reality any more.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:130: bool in_or_entering_fullscreen_;
[something to consider/discuss]
As the state gets more and more complex[1], I am thinking it may be good to have
a method, say, ValidateState() to check that all these variables are in a
consistent state. (For example, fullscreened_tab_ is never non-NULL when
in_or_entering_fullscreen_ is false.) It will serve as documentation of the
relation between these variables. And we can use it with DCHECK so it won't run
in release build.

=======================
[1] It took me a while to recall why (fullscreened_tab_ != NULL) is not the same
as in_or_entering_fullscreen_.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:133: TabContentsWrapper*
mouse_lock_tab_;
Maybe add comment that |mouse_lock_tab_| is not NULL <==>
!MOUSELOCK_NOT_REQUESTED.

[scheib, 2012-05-01 03:27:07]

yzshen: All comments addressed, please take another look.

hashimoto: Please review changes related to fullscreen for extentions in
fullscreen_controller.cc:

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/fullscreen_...
 search text: 'extension_caused_fullscreen_'
In particular yzshen's concerns raised here http://goo.gl/kTWUn and here
http://goo.gl/PccCA.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:72:
base::Bind(&FullscreenController::NotifyMouseLockChange, this));
On 2012/04/30 19:32:40, yzshen1 wrote:
> Is it sufficient to send chrome::NOTIFICATION_MOUSE_LOCK_CHANGED when
> mouse_lock_state_ changes?
> 
> If that is true, the code can be simplified a great deal, and it is more clear
> when such a notification gets sent.
> (You can replace mouse_lock_state_ = ... statements with SetMouseLockState()
and
> send notification in it.)

Tests are the reason this notification exists. A test that requests mouse lock
without a user gesture needs to know that the request has been handled and a
decision made. A callback is made to JavaScript, but I didn't see an easy way to
have a browser test block until a JS callback is made. The notification here is
very easy for test code to wait on, but needs to fire in all decision events
(not only a state change).

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:87: mouse_lock_state_ =
MOUSELOCK_REQUESTED;
On 2012/04/30 19:32:40, yzshen1 wrote:
> Please add more comment about why we need to merge (click is required so we
> cannot go into mouselock, etc.)

Done.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:88: break;
On 2012/04/30 19:32:40, yzshen1 wrote:
> You could use an "else" on line 89 instead of two breaks in this case.

Done.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:95: mouse_lock_tab_ = NULL;
On 2012/04/30 19:32:40, yzshen1 wrote:
> Shall we explicitly set mouse_lock_state_ to not requested in this case?

Done.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:99: mouse_lock_tab_ = NULL;
On 2012/04/30 19:32:40, yzshen1 wrote:
> Shall we set mouse_lock_state_ to not requested in this case?

Done.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:276:
UpdateFullscreenExitBubbleContent();
On 2012/04/30 19:32:40, yzshen1 wrote:
> nit: It is previously in the "if (mouse_lock)" block because
> ExitTabbedFullscreenModeIfNecessary() will update bubble content if necessary.

Done. -- Reverted to previous code; added comment explaining why.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:301:
ExitTabbedFullscreenModeIfNecessary();
On 2012/04/30 19:32:40, yzshen1 wrote:
> I don't get it: in windowed mouse lock mode, why do we need to
> ExitTabbedFullscreenModeIfNecessary()? Maybe we should change the name/meaning
> of the method?

Done: Renamed to ExitTabFullscreenOrMouseLockIfNecessary().

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:326:
web_contents->GotResponseToLockMouseRequest(false);
On 2012/04/30 19:32:40, yzshen1 wrote:
> If in ACCEPTED mode, we shouldn't call GotResponseToLockMouseRequest, right?

Done. Changed to calling RVHV::UnlockMouse() when we aren't PENDING.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:347: else if (mouse_lock_tab_)
On 2012/04/30 19:32:40, yzshen1 wrote:
> Is this order right?
> Consider the following two sequences:
> 1) enter extension-triggered fullscreen -> enter mouse lock
> 2) enter mouse lock -> enter extension-triggered fullscreen
> 
> in these two cases, which URL shall we show? the URL of the extension or the
> mouse lock tab?

I've moved the logic for using the extension url lower. It should be a fallback
for when there is no other indication, as it's intent is more like a 'browser
mode fullscreen'.

That said, the 2 states you mentioned would evaluate the same either way, the
mouse lock api would win. I've tested this manually to see that the host
evaluates to a reasonable string when called from extensions.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:354: && IsMouseLocked()
On 2012/04/30 19:32:40, yzshen1 wrote:
> && at the end of the previous line, please.

Done.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:356:
mouse_lock_tab_->web_contents()->GotResponseToLockMouseRequest(false);
On 2012/04/30 19:32:40, yzshen1 wrote:
> GotResponseToLockMouseRequest() should only be called when REQUESTED. At least
> that is the original design, maybe you want to do what line 318 does, or
change
> the meaning/name of GotResponseToLockMouseRequest. (It is good to unify the
> behavior with line 318 as well.)
> 
> Besides, you should change mouse_lock_state_ and mouse_lock_tab_ here.

Done. Changed to calling RVHV::UnlockMouse().

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:396: return
FEB_TYPE_BROWSER_EXTENSION_FULLSCREEN_EXIT_INSTRUCTION;
On 2012/04/30 19:32:40, yzshen1 wrote:
> Here: what if the mouse lock is requested, etc.?

Done. This should be used similar to Browser mode fullscreen, I have moved it
lower.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:453: in_or_entering_fullscreen_ =
!window_->IsFullscreen();
On 2012/04/30 19:32:40, yzshen1 wrote:
> Is it possible that there are paths to change fullscreen state that don't go
> through TogglePresentationModeInternal/ToggleFullscreenModeInternal?
> (I think there are. Say, tab is closed.)
> 
> In that case, in_or_entering_fullscreen_ doesn't reflect the reality any more.

Closing a tab does call Toggle, and this state is correct / consistent within
FullscreenController.

I've renamed the variable toggled_into_fullscreen_ to be explicit about what it
represents. It's currently used in only one place, selecting the bubble type
between none, browser fullscreen, extension fullscreen. I believe any outside
cause of changing the window state would have other more serious problems if not
informing the fullscreen controller.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:130: bool in_or_entering_fullscreen_;
On 2012/04/30 19:32:40, yzshen1 wrote:
> [something to consider/discuss]
> As the state gets more and more complex[1], I am thinking it may be good to
have
> a method, say, ValidateState() to check that all these variables are in a
> consistent state. (For example, fullscreened_tab_ is never non-NULL when
> in_or_entering_fullscreen_ is false.) It will serve as documentation of the
> relation between these variables. And we can use it with DCHECK so it won't
run
> in release build.
> 
> =======================
> [1] It took me a while to recall why (fullscreened_tab_ != NULL) is not the
same
> as in_or_entering_fullscreen_.

ValidateState() is an interesting idea. It would also be pretty complex I think.
I've been expanding the browser tests (before this patch, and in this patch) to
do more scrutinizing of state.

For this variable, I've renamed it to toggled_into_fullscreen_, and added a
comment pointing out it is also used for browser fullscreen.

http://codereview.chromium.org/10261011/diff/2001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:133: TabContentsWrapper*
mouse_lock_tab_;
On 2012/04/30 19:32:40, yzshen1 wrote:
> Maybe add comment that |mouse_lock_tab_| is not NULL <==>
> !MOUSELOCK_NOT_REQUESTED.

Done.

[hashimoto, 2012-05-01 12:06:40]

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/fullscreen_...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/fullscreen_...
chrome/browser/ui/fullscreen_controller.cc:465: fprintf(stderr,
"ToggleFullscreenModeInternal ----------------\n");
Remove this.

[Nico, 2012-05-01 14:57:21]

jeremya, can you look at the mac bubble stuff?

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/browser_bro...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/browser_bro...
chrome/browser/ui/browser_browsertest.cc:194: // Fullscreen transition
notification observer simplifies test code.
s/Fullscreen/Mouse lock/

Or remove the comment (and the one 10 lines above)

[jeremya, 2012-05-07 01:23:41]

lgtm. I've heard on the grapevine that there are plans to change the fullscreen
UI, though, so you might want to talk to Glen about how that might affect the
windowed mouse lock stuff.

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/cocoa/fulls...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/cocoa/fulls...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:141: origin.y =
maxY - NSHeight(windowFrame);
These two lines are dead code now.

[yzshen1, 2012-05-18 01:00:45]

LGTM
Only a few nits.

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/browser_bro...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/browser_bro...
chrome/browser/ui/browser_browsertest.cc:1082: browser(), ui::VKEY_D, false,
false, false, false,
wrong indent (and some other places).
It should be 4 spaces.

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/fullscreen_...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/fullscreen_...
chrome/browser/ui/fullscreen_controller.cc:185: mouse_lock_state_ =
MOUSELOCK_NOT_REQUESTED;
Shall we reset the mouse lock tab here?

[scheib, 2012-05-18 23:07:49]

sky, please take an owners look for code in chrome/browser/ui.

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/browser_bro...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/browser_bro...
chrome/browser/ui/browser_browsertest.cc:194: // Fullscreen transition
notification observer simplifies test code.
On 2012/05/01 14:57:21, Nico wrote:
> s/Fullscreen/Mouse lock/
> 
> Or remove the comment (and the one 10 lines above)

Done.

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/browser_bro...
chrome/browser/ui/browser_browsertest.cc:194: // Fullscreen transition
notification observer simplifies test code.
On 2012/05/01 14:57:21, Nico wrote:
> s/Fullscreen/Mouse lock/
> 
> Or remove the comment (and the one 10 lines above)

Done.

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/browser_bro...
chrome/browser/ui/browser_browsertest.cc:1082: browser(), ui::VKEY_D, false,
false, false, false,
On 2012/05/18 01:00:45, yzshen1 wrote:
> wrong indent (and some other places).
> It should be 4 spaces.

Done.

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/fullscreen_...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/fullscreen_...
chrome/browser/ui/fullscreen_controller.cc:185: mouse_lock_state_ =
MOUSELOCK_NOT_REQUESTED;
On 2012/05/18 01:00:45, yzshen1 wrote:
> Shall we reset the mouse lock tab here?

Done.

http://codereview.chromium.org/10261011/diff/10/chrome/browser/ui/fullscreen_...
chrome/browser/ui/fullscreen_controller.cc:465: fprintf(stderr,
"ToggleFullscreenModeInternal ----------------\n");
On 2012/05/01 12:06:40, hashimoto wrote:
> Remove this.

Done.

[hashimoto, 2012-05-21 07:37:05]

lgtm with a nit

http://codereview.chromium.org/10261011/diff/16011/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10261011/diff/16011/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:235: mouse_lock_state_ =
MOUSELOCK_ACCEPTED;
nit: same indent as 'else' below

[sky, 2012-05-21 15:18:38]

http://codereview.chromium.org/10261011/diff/16011/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10261011/diff/16011/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:72:
MessageLoop::current()->PostTask(FROM_HERE,
Why does this need to be delayed? At a minimum add a comment.

http://codereview.chromium.org/10261011/diff/16011/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:188:
base::Bind(&FullscreenController::NotifyMouseLockChange, this));
Why is this delayed?

With the delayed calls what happens if before the posttask is processed state
changes again?

http://codereview.chromium.org/10261011/diff/16011/chrome/common/chrome_notif...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/10261011/diff/16011/chrome/common/chrome_notif...
chrome/common/chrome_notification_types.h:1058: // Sent when the browser
changes, confirms, or denies mouse lock mode.
Document what the source/details are.

[scheib, 2012-05-21 20:29:13]

http://codereview.chromium.org/10261011/diff/16011/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10261011/diff/16011/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:72:
MessageLoop::current()->PostTask(FROM_HERE,
On 2012/05/21 15:18:38, sky wrote:
> Why does this need to be delayed? At a minimum add a comment.

Done: Delay not needed, removed PostTask.

http://codereview.chromium.org/10261011/diff/16011/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:188:
base::Bind(&FullscreenController::NotifyMouseLockChange, this));
On 2012/05/21 15:18:38, sky wrote:
> Why is this delayed?
> 
> With the delayed calls what happens if before the posttask is processed state
> changes again?

Done.

http://codereview.chromium.org/10261011/diff/16011/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:235: mouse_lock_state_ =
MOUSELOCK_ACCEPTED;
On 2012/05/21 07:37:05, hashimoto wrote:
> nit: same indent as 'else' below

Done.

http://codereview.chromium.org/10261011/diff/16011/chrome/common/chrome_notif...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/10261011/diff/16011/chrome/common/chrome_notif...
chrome/common/chrome_notification_types.h:1058: // Sent when the browser
changes, confirms, or denies mouse lock mode.
On 2012/05/21 15:18:38, sky wrote:
> Document what the source/details are.

Done.

[sky, 2012-05-21 20:53:58]

LGTM