Leave fullscreen mode in an app window when ESC key is pressed.

Leave fullscreen mode in an app window when ESC key is pressed.

A few things might need to be discussed like the ability for an app to prevent
the default behaviour and letting the user know about the ability to leave the
fullscreen mode using ESC.

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

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

[koz (OOO until 15th September), 2013-11-19 07:29:57]

https://codereview.chromium.org/62763003/diff/1/chrome/browser/apps/app_inter...
File chrome/browser/apps/app_interactive_uitest.cc (right):

https://codereview.chromium.org/62763003/diff/1/chrome/browser/apps/app_inter...
chrome/browser/apps/app_interactive_uitest.cc:18: ,
initial_fullscreen_state_(window_->IsFullscreen())
nit: commas go on previous lines and variables line up, like

    : window_(window),
      initial_fullscreen_state_(...),

https://codereview.chromium.org/62763003/diff/1/chrome/browser/apps/app_inter...
chrome/browser/apps/app_interactive_uitest.cc:20: {}
nit: curlies should look like

      already_changed_(false) {
  }

or
      already_changed_(false) {}

https://codereview.chromium.org/62763003/diff/1/chrome/browser/apps/app_inter...
chrome/browser/apps/app_interactive_uitest.cc:36: bool            
initial_fullscreen_state_;
nit: should only be one space between type and variable name.

https://codereview.chromium.org/62763003/diff/1/chrome/browser/apps/app_inter...
chrome/browser/apps/app_interactive_uitest.cc:54:
content::RunAllPendingInMessageLoop();
If we have the app send a message after it goes into fullscreen and wait for
that can we avoid this loop? Same with the other while(!...) loops - can they be
replaced by messages?

https://codereview.chromium.org/62763003/diff/1/chrome/browser/apps/app_inter...
chrome/browser/apps/app_interactive_uitest.cc:79: // renderer process such as
the key event so they should arrive in the same
"renderer process before the keypress and should be received in that order."

[mlamouri (slow - plz ping), 2013-11-20 00:01:15]

All comments applied.

PTAL.

[koz (OOO until 15th September), 2013-11-20 00:02:48]

Awesome, thanks for adding the comment.

lgtm

[benwells, 2013-11-20 04:07:55]

On 2013/11/20 00:02:48, koz wrote:
> Awesome, thanks for adding the comment.
> 
> lgtm

From the bug comments, I think we should start with this overridable.

[mlamouri (slow - plz ping), 2013-11-21 08:04:12]

I've updated this to make ESC overridable and have a permission to do that.

PTAL.

[koz (OOO until 15th September), 2013-11-21 23:12:41]

https://codereview.chromium.org/62763003/diff/210001/chrome/browser/apps/app_...
File chrome/browser/apps/app_interactive_uitest.cc (right):

https://codereview.chromium.org/62763003/diff/210001/chrome/browser/apps/app_...
chrome/browser/apps/app_interactive_uitest.cc:36: bool already_changed_;
DISALLOW_COPY_AND_ASSIGN(FullscreenChangeHelper);

https://codereview.chromium.org/62763003/diff/210001/chrome/browser/apps/app_...
chrome/browser/apps/app_interactive_uitest.cc:53: FullscreenChangeHelper
fs_changed(GetFirstShellWindow()->GetBaseWindow());
I'd say rename this to FullscreenChangeWatcher, and add the following while loop
as a method, so this looks like...

FullscreenChangeWatcher fs_changed(GetFirstShellWindow()->GetBaseWindow());
launched_listener.Reply("window");
fs_changed.Wait();

Also, while launched_listener.Reply("window") above is asynchronous and so is
probably not racy, it's probably good practice to send that message after
constructing FullscreenChange. I think it makes the flow of the test clearer,
too.

https://codereview.chromium.org/62763003/diff/210001/chrome/browser/apps/app_...
chrome/browser/apps/app_interactive_uitest.cc:165:
EXPECT_TRUE(GetFirstShellWindow()->GetBaseWindow()->IsFullscreen());
Seeing as we expect fullscreen to occur asynchronously this is probably a false
positive even now, as the window will still be fullscreen at this point even if
we intend for it to be dismissed. Therefore we should remove this test
altogether.

https://codereview.chromium.org/62763003/diff/210001/chrome/browser/apps/app_...
chrome/browser/apps/app_interactive_uitest.cc:168:
IN_PROC_BROWSER_TEST_F(AppInteractiveTest, ESCDoesNotLeaveFullscreenDOM) {
Again, I think this is impossible to test unless more signals are exposed so
just remove this test.

https://codereview.chromium.org/62763003/diff/210001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/prevent_leave_fullscreen/main.js
(right):

https://codereview.chromium.org/62763003/diff/210001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/prevent_leave_fullscreen/main.js:25:
console.log('b received');
Remove logging.

https://codereview.chromium.org/62763003/diff/210001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/prevent_leave_fullscreen/main.js:39:
console.log('b received');
Remove logging.

[mlamouri (slow - plz ping), 2013-11-22 00:27:32]

On 2013/11/21 23:12:41, koz wrote:
>
https://codereview.chromium.org/62763003/diff/210001/chrome/browser/apps/app_...
> chrome/browser/apps/app_interactive_uitest.cc:165:
> EXPECT_TRUE(GetFirstShellWindow()->GetBaseWindow()->IsFullscreen());
> Seeing as we expect fullscreen to occur asynchronously this is probably a
false
> positive even now, as the window will still be fullscreen at this point even
if
> we intend for it to be dismissed. Therefore we should remove this test
> altogether.
> 
>
https://codereview.chromium.org/62763003/diff/210001/chrome/browser/apps/app_...
> chrome/browser/apps/app_interactive_uitest.cc:168:
> IN_PROC_BROWSER_TEST_F(AppInteractiveTest, ESCDoesNotLeaveFullscreenDOM) {
> Again, I think this is impossible to test unless more signals are exposed so
> just remove this test.

This test might fail to catch a failure but could definitively catch it given
that between ESC being sent and the check there is one IPC round trip and an
event loop round trip. It might be enough to catch the fullscreen mode being
left.

Other comments applied. PTAL.

[koz (OOO until 15th September), 2013-11-22 00:33:10]

lgtm

https://codereview.chromium.org/62763003/diff/300001/chrome/browser/apps/app_...
File chrome/browser/apps/app_interactive_uitest.cc (right):

https://codereview.chromium.org/62763003/diff/300001/chrome/browser/apps/app_...
chrome/browser/apps/app_interactive_uitest.cc:176:
ASSERT_TRUE(ui_test_utils::SendKeyPressToWindowSync(
nit: consider encapsulating this as well

SimulateKeyPress(ui::VKEY_A);

https://codereview.chromium.org/62763003/diff/300001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/prevent_leave_fullscreen/main.js
(right):

https://codereview.chromium.org/62763003/diff/300001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/prevent_leave_fullscreen/main.js:25:
setTimeout(function() {
nit: Add a comment explaining why we setTimeout() here.

[commit-bot: I haz the power, 2013-11-25 20:00:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/62763003/450001

[commit-bot: I haz the power, 2013-11-25 21:11:01]

Retried try job too often on linux_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-26 17:16:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/62763003/470001

[commit-bot: I haz the power, 2013-11-26 19:49:04]

Change committed as 237393

[Roger Tawa OOO till Jul 10th, 2013-11-26 20:49:13]

A revert of this CL has been created in
https://codereview.chromium.org/89183002/ by rogerta@chromium.org.

The reason for reverting is: This CL broke interactive tests on mac:
http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac10.6%20Tests%....

[commit-bot: I haz the power, 2013-11-27 17:57:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/62763003/520001

[commit-bot: I haz the power, 2013-11-28 01:16:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/62763003/520001

[commit-bot: I haz the power, 2013-11-28 01:59:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/62763003/520001

[commit-bot: I haz the power, 2013-11-28 02:18:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/62763003/520001

[commit-bot: I haz the power, 2013-11-28 02:26:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/62763003/520001

[commit-bot: I haz the power, 2013-11-28 02:57:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/62763003/520001

[commit-bot: I haz the power, 2013-11-28 03:27:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/62763003/520001

[commit-bot: I haz the power, 2013-11-28 03:49:38]

Change committed as 237666