Enable immersive fullscreen for hosted apps part 2

Enable immersive fullscreen for hosted apps part 2

BUG=307622
TEST=ImmersiveModeControllerAshTestHostedApp.Layout
Manual Test:
1) Enable "Enable immersive fullscreen for non browser windows" on ChromeOS
2) Install a hosted app (I chose "Cut the Rope")
3) Hit <F4>. Make sure that the app goes fullscreen
4) Hover the mouse at the top of the display and make sure that the window header slides out
5) Hit <F4>. Make sure that the app returns to its formal state.
6) Go to popuptest.com/popuptest1.html in a normal browser window
7) Active one of the popup windows and hit <F4>.
8) Make sure that the popup goes fullscreen
9) Hover the mouse at the top of the display and make sure that the window header slides out
10) Hit <F4>. Make sure that the popup returns to its formal state

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

[pkotwicz, 2013-11-23 21:37:15]

James, can you please take a look?

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (left):

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:272:
gfx::Point toolbar_origin(browser_view()->GetToolbarBounds().origin());
This code is wrong. BrowserNonClientFrameViewAsh::Layout() occurs before
BrowserView::Layout().

[James Cook, 2013-11-25 19:09:20]

LGTM with nits. Another epic patch.

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/ash/ac...
File chrome/browser/ui/ash/accelerator_commands_browsertest.cc (right):

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/ash/ac...
chrome/browser/ui/ash/accelerator_commands_browsertest.cc:190: }
Nice test.

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame_ash.cc (right):

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:67: #if defined(OS_CHROMEOS)
nit: Can this move into the OS_CHROMEOS block below?

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:161: // or
without the top-of-window views revealed)  When in immersive fullscreen
nit. Missing . after )

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:312: return
(rect.y() < GetTopInset());
nit: no need for extra parens

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:167: bool
ImmersiveModeControllerAsh::UpdateTabIndicators() {
Nice, I like consolidating all this logic here.

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right):

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:21: class
ImmersiveModeControllerAsh
Random thought for a future CL: It would be nice to have a header file somewhere
that has a mini-design comment, explaining the difference between immersive
fullscreen, tab fullscreen, etc. and then pointers to it in
ImmersiveModeController, maybe here, etc.

Kinda like histogram.h but not a 5-page discourse. :-)

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc
(right):

https://codereview.chromium.org/83343006/diff/100001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:295:
AddTab(browser(), GURL("about:blank"));
You might want to comment here about how it's still a hosted app even though
you're adding a tab.

https://codereview.chromium.org/83343006/diff/100001/chrome/test/base/browser...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/83343006/diff/100001/chrome/test/base/browser...
chrome/test/base/browser_with_test_window_test.cc:200: void
BrowserWithTestWindowTest::PopulateBrowserCreateParams(
nit: What do you think about making this Browser::CreateParams
GetBrowserCreateParams(Profile*, HostDesktopType) or similar?

[pkotwicz, 2013-12-02 16:21:39]

Scott, can you please take a look at the changes in chrome/test

[sky, 2013-12-02 21:50:18]

chrome/test LGTM

[commit-bot: I haz the power, 2013-12-02 23:07:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/83343006/420001

[commit-bot: I haz the power, 2013-12-03 00:59:28]

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

[commit-bot: I haz the power, 2013-12-03 01:25:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/83343006/420001

[commit-bot: I haz the power, 2013-12-03 03:27:17]

Change committed as 238288