Enable all ESC-Fullscreen tests (Chrome Apps) for all platforms.

Enable all ESC-Fullscreen tests (Chrome Apps) for all platforms.

There were three issues:
1. a silly bug in the fullscreen change check;
2. there was no guarantee that the window would be actually focused;
3. changing the fullscreen state might come with an animation during which key events will not be received.

Note that 2 and 3 are a problem on MacOS only.

BUG=None

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

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

[mlamouri (slow - plz ping), 2013-12-17 18:04:35]

FYI, patch 5 is passing try (linux_aura is still running but I am confident
given that it passes in patch 4).

[mlamouri (slow - plz ping), 2014-01-08 12:14:49]

koz@, I think you forgot that review :) PTAL

[koz (OOO until 15th September), 2014-01-08 23:29:25]

lgtm with nits

Nice change, Mounir! Sorry about the delay in review!

https://codereview.chromium.org/116023002/diff/100001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_interactive_uitest.cc (right):

https://codereview.chromium.org/116023002/diff/100001/chrome/browser/apps/app...
chrome/browser/apps/app_window_interactive_uitest.cc:81:
GetFirstShellWindow()->GetNativeWindow()));
nit: I think the second line should be indented 4 spaces from ASSERT_TRUE(, ie:

  ASSERT_TRUE(...(
      GetFirstShellWindow(...)));

https://codereview.chromium.org/116023002/diff/100001/chrome/browser/apps/app...
chrome/browser/apps/app_window_interactive_uitest.cc:231: WaitUntilKeyFocus();
It might make sense to name this WaitForFullscreenAnimationToComplete() or
something, then the duplicated comments won't be necessary.

https://codereview.chromium.org/116023002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/leave_fullscreen/main.js (right):

https://codereview.chromium.org/116023002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/leave_fullscreen/main.js:18: if
(e.keyCode != 90) // 'z'
'z' is 122, 'Z' is 90. Consider using 'Z'.charCodeAt(0) instead of 90.

https://codereview.chromium.org/116023002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/leave_fullscreen/main.js:21:
chrome.test.sendMessage('KEY_RECEIVED');
nit: The other messages are in camel case so this should probably be
'KeyReceived'.

https://codereview.chromium.org/116023002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/prevent_leave_fullscreen/main.js
(right):

https://codereview.chromium.org/116023002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/prevent_leave_fullscreen/main.js:19:
if (e.keyCode != 90) // 'z'
same comments as above

[mlamouri (slow - plz ping), 2014-01-09 17:08:46]

https://codereview.chromium.org/116023002/diff/100001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_interactive_uitest.cc (right):

https://codereview.chromium.org/116023002/diff/100001/chrome/browser/apps/app...
chrome/browser/apps/app_window_interactive_uitest.cc:81:
GetFirstShellWindow()->GetNativeWindow()));
On 2014/01/08 23:29:25, koz wrote:
> nit: I think the second line should be indented 4 spaces from ASSERT_TRUE(,
ie:
> 
>   ASSERT_TRUE(...(
>       GetFirstShellWindow(...)));

Done.

https://codereview.chromium.org/116023002/diff/100001/chrome/browser/apps/app...
chrome/browser/apps/app_window_interactive_uitest.cc:231: WaitUntilKeyFocus();
On 2014/01/08 23:29:25, koz wrote:
> It might make sense to name this WaitForFullscreenAnimationToComplete() or
> something, then the duplicated comments won't be necessary.

But we might want to re-use that logic for other tests that require key focus.
If the name isn't clear about what the function is doing, we might miss this
re-usability.

https://codereview.chromium.org/116023002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/leave_fullscreen/main.js (right):

https://codereview.chromium.org/116023002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/leave_fullscreen/main.js:18: if
(e.keyCode != 90) // 'z'
On 2014/01/08 23:29:25, koz wrote:
> 'z' is 122, 'Z' is 90. Consider using 'Z'.charCodeAt(0) instead of 90.

keyCode and charCode are not the same thing and I would prefer not to try to
find a relationship between DOM KeyEvents and ECMAScript char codes.

https://codereview.chromium.org/116023002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/leave_fullscreen/main.js:21:
chrome.test.sendMessage('KEY_RECEIVED');
On 2014/01/08 23:29:25, koz wrote:
> nit: The other messages are in camel case so this should probably be
> 'KeyReceived'.

Done.

[commit-bot: I haz the power, 2014-01-09 17:09:59]

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

[commit-bot: I haz the power, 2014-01-09 18:49:12]

Change committed as 243938

[alph, 2014-01-09 19:39:50]

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

The reason for reverting is: The tests seem to be broken on Mac10.6
http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/2....

[commit-bot: I haz the power, 2014-01-12 23:55:05]

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

[commit-bot: I haz the power, 2014-01-13 04:20:36]

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

[commit-bot: I haz the power, 2014-01-13 23:18:19]

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

[commit-bot: I haz the power, 2014-01-14 03:08:46]

Change committed as 244640