Introduce WindowStateDelegate::ToggleFullscreen

Introduce WindowStateDelegate::ToggleFullscreen
Replace kAnimateToFullscreenKey with a boolean flag
  in WindowState

I removed  #if defined(OS_WIN).. #endif in chrome_shell_delegate.cc because you'll never get window in desktop environment there.

Next step. I'll look into if we can change so that WindowState::Restore can restore from fullscreen state properly.

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

[pkotwicz, 2013-10-25 05:16:17]

https://codereview.chromium.org/42353002/diff/70001/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/42353002/diff/70001/ash/wm/window_state.h#new...
ash/wm/window_state.h:127: bool maximize_when_fullscreen() const {
We need to be careful as to how much we want to respect this attribute.
V1 apps are maximzed instead of put in fullscreen when the user presses the
fullscreen button on the keyboard. A v1 app can still be fullscreened though.
(Chrome remote desktop is a v1 app which most definately can be fullscreened)

Separately I am working on making it possible to put v1 and v2 apps into
immersive fullscreen. I want to have a flag in chrome://flags to do this by the
end of next week.

oshima@, can we further discuss your plans wrt to fullscreen. This is a
fullscreen related CL which will be landing on Friday (today)
https://codereview.chromium.org/32943006/

[oshima, 2013-10-25 13:55:55]

On 2013/10/25 05:16:17, pkotwicz wrote:
> https://codereview.chromium.org/42353002/diff/70001/ash/wm/window_state.h
> File ash/wm/window_state.h (right):
> 
>
https://codereview.chromium.org/42353002/diff/70001/ash/wm/window_state.h#new...
> ash/wm/window_state.h:127: bool maximize_when_fullscreen() const {
> We need to be careful as to how much we want to respect this attribute.
> V1 apps are maximzed instead of put in fullscreen when the user presses the
> fullscreen button on the keyboard. A v1 app can still be fullscreened though.
> (Chrome remote desktop is a v1 app which most definately can be fullscreened)

Yes, I understand. This CL doesn't change the exiting behavior though.

> Separately I am working on making it possible to put v1 and v2 apps into
> immersive fullscreen. I want to have a flag in chrome://flags to do this by
the
> end of next week.
> 
> oshima@, can we further discuss your plans wrt to fullscreen. This is a
> fullscreen related CL which will be landing on Friday (today)
> https://codereview.chromium.org/32943006/

sure.

[oshima, 2013-10-25 15:24:19]

please hold. Given that fullscreen in chrome is more than just fullscreen, I'll
take different approach.

[pkotwicz, 2013-10-25 19:56:37]

https://codereview.chromium.org/42353002/diff/140001/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/42353002/diff/140001/ash/wm/window_state.cc#n...
ash/wm/window_state.cc:154: window_->SetProperty(aura::client::kShowStateKey,
ui::SHOW_STATE_NORMAL);
In a future CL, you will need to make Restore() call delegate_->ExitFullscreen()
if the window is currently in fullscreen.

https://codereview.chromium.org/42353002/diff/140001/ash/wm/window_state.cc#n...
ash/wm/window_state.cc:168: Restore();
When exiting fullscreen, we should ideally have the same behavior as
NativeWidgetAura::SetFullscreen(). I am fine if you do this as a separate CL
though.

https://codereview.chromium.org/42353002/diff/140001/ash/wm/window_state_dele...
File ash/wm/window_state_delegate.h (right):

https://codereview.chromium.org/42353002/diff/140001/ash/wm/window_state_dele...
ash/wm/window_state_delegate.h:20: // Invoked when the user uses Shift+F4 to
toggle the window fullscreen state.
F4 toggles fullscreen too.

I would rather have two methods EnterFullscreen() and ExitFullscreen().
In order to implement putting v1 and v2 packaged apps into immersive fullscreen,
I will add
EnterImmersiveFullscreen() (Possibly removing EnterFullscreen())

Planned behavior for v2 apps:
- Pressing <F4> should enter immersive fullscreen
- chrome.app.window.current().fullscreen() (aka ShellWindow::Fullscreen())
should not enter immersive fullscreen.
I am really concerned about confusion as to what ToggleFullscreen() does once I
make that change.

https://codereview.chromium.org/42353002/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/42353002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:194: new
NativeAppWindowStateDelegate());
It MAY be worth passing in |shell_window_| to NativeAppWindowStateDelegate. I
have not looked at whether this is koscher wrt to ShellWindow and
NativeAppWindowStateDelegate's lifetime.

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

https://codereview.chromium.org/42353002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:36: DCHECK(browser);
I would make this CHECK() and remmove the following if statement

https://codereview.chromium.org/42353002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:135:
window_state->set_animate_to_fullscreen(!browser->is_type_tabbed());
FYI: This property seems to be to avoid animating to immersive fullscreen and to
tab fullscreen. It is going to be fun to make this work properly once v1 and v2
apps can be some of the time put into immersive fullscreen.

[James Cook, 2013-10-25 20:17:27]

Peter, can you briefly summarize the long-term plan for fullscreen from the Ash
point of view?

Is the long term goal to have two different fullscreen states in Ash?
1) Window controls slide in when you hit the top of the screen
2) Nothing slides in

Or does Ash need to know about the special browser-style immersive fullscreen
with tab highlights (I would hope not?)?

[pkotwicz, 2013-10-25 20:40:25]

Yes, the long term goal is to have something similar to having two different
fullscreen states in Ash. For apps, 2 states are sufficient.
Unfortunately, when the browser is immersive fullscreen,
- sometimes the shelf is forced to auto hide
- sometimes the shelf is forced hidden
- and the shelf is in ash

Per discussion with oshima@, we have decided that we will have a separate flag
dedicated to whether the shelf is forced hidden in fullscreen.

[James Cook, 2013-10-25 20:43:20]

oshima, overall approach seems fine, let me know when you're ready for final
review

[oshima, 2013-10-25 22:36:20]

https://codereview.chromium.org/42353002/diff/140001/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/42353002/diff/140001/ash/wm/window_state.cc#n...
ash/wm/window_state.cc:154: window_->SetProperty(aura::client::kShowStateKey,
ui::SHOW_STATE_NORMAL);
On 2013/10/25 19:56:38, pkotwicz wrote:
> In a future CL, you will need to make Restore() call
delegate_->ExitFullscreen()
> if the window is currently in fullscreen.

See my comment below.

https://codereview.chromium.org/42353002/diff/140001/ash/wm/window_state.cc#n...
ash/wm/window_state.cc:168: Restore();
On 2013/10/25 19:56:38, pkotwicz wrote:
> When exiting fullscreen, we should ideally have the same behavior as
> NativeWidgetAura::SetFullscreen(). I am fine if you do this as a separate CL
> though.

I think ash should restore to previous state, instead of views::Widget. This is
one of things I'm planning to do as a part
of restore improvement.

https://codereview.chromium.org/42353002/diff/140001/ash/wm/window_state_dele...
File ash/wm/window_state_delegate.h (right):

https://codereview.chromium.org/42353002/diff/140001/ash/wm/window_state_dele...
ash/wm/window_state_delegate.h:20: // Invoked when the user uses Shift+F4 to
toggle the window fullscreen state.
On 2013/10/25 19:56:38, pkotwicz wrote:
> F4 toggles fullscreen too.
> 
> I would rather have two methods EnterFullscreen() and ExitFullscreen().
> In order to implement putting v1 and v2 packaged apps into immersive
fullscreen,
> I will add
> EnterImmersiveFullscreen() (Possibly removing EnterFullscreen())
> 
> Planned behavior for v2 apps:
> - Pressing <F4> should enter immersive fullscreen
> - chrome.app.window.current().fullscreen() (aka ShellWindow::Fullscreen())
> should not enter immersive fullscreen.
> I am really concerned about confusion as to what ToggleFullscreen() does once
I
> make that change.

What I'm hoping for is that Restore() can restore chrome properly, so just
EnterFullscreen but no ExitFullscreen.

In any case, these change will be in separate CL.

https://codereview.chromium.org/42353002/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/42353002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:194: new
NativeAppWindowStateDelegate());
On 2013/10/25 19:56:38, pkotwicz wrote:
> It MAY be worth passing in |shell_window_| to NativeAppWindowStateDelegate. I
> have not looked at whether this is koscher wrt to ShellWindow and
> NativeAppWindowStateDelegate's lifetime.

I'm fine with either way. I'm just using what the original code was doing. let
me know if you want me to change this.

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

https://codereview.chromium.org/42353002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:36: DCHECK(browser);
On 2013/10/25 19:56:38, pkotwicz wrote:
> I would make this CHECK() and remmove the following if statement

Done.

[oshima, 2013-10-25 22:36:29]

james, please review.

[pkotwicz, 2013-10-25 23:31:14]

A few more comments.

Also, were you planning on moving the browser tests in
chrome_shell_delegate_browsertest.cc as part of this CL?

https://codereview.chromium.org/42353002/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/42353002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:194: new
NativeAppWindowStateDelegate());
I think that it is worth investigating if passing in |shell_window_| is
possible.

https://codereview.chromium.org/42353002/diff/240001/ash/wm/window_state_dele...
File ash/wm/window_state_delegate.h (right):

https://codereview.chromium.org/42353002/diff/240001/ash/wm/window_state_dele...
ash/wm/window_state_delegate.h:20: // Invoked when the user uses Shift+F4/F4g to
toggle the window
Shift+F4/F4g -> F4

https://codereview.chromium.org/42353002/diff/240001/ash/wm/window_state_dele...
ash/wm/window_state_delegate.h:21: // fullscreen state.  |WindowState| fallbacks
to the default
Nit: WindowState -> |window_state|

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

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:24:
BrowserWindowStateDelegate() {}
I would much rather have a Browser* parameter be passed in.

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:27: // Invoked when the user
uses Shift+F4 to toggle the window fullscreen state.
Shift+F4 -> F4

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:41: //
AppNonClientFrameViewAsh shows only the window controls and no other
Would it be possible to set a custom delegate in AppNonClientFrameViewAsh to do
this handling? This change will make me happy :)
This changes the caveat that WindowState::SetDelegate() will only be called once
for a given WindowState object. (I will need to change that anyways for
CustomFrameViewAsh though)

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:98: // Don't do anything if
we don't have our non-client view yet.
Can you do me a favor and remove this method?

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:137: if
(!browser_view->browser()->bounds_overridden() &&
Might as well use |browser| in the remainder of the function

[pkotwicz, 2013-10-25 23:34:14]

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

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:98: // Don't do anything if
we don't have our non-client view yet.
I was going to remove it as part of https://codereview.chromium.org/25536010/
but we did a complete redesign

[pkotwicz, 2013-10-26 03:46:34]

https://codereview.chromium.org/42353002/diff/240001/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/42353002/diff/240001/ash/wm/window_state.cc#n...
ash/wm/window_state.cc:164: void WindowState::ToggleFullscreen() {
You probably want to check CanMaximize() in this function. Otherwise you will be
able to use <F4> to maximize the bookmark bubble and other non maximizable
widgets.

[James Cook, 2013-10-28 16:12:10]

oshima, do you want me to look now, or do you want to take a look at peter's
comments first?

[oshima, 2013-10-28 18:55:48]

On 2013/10/28 16:12:10, James Cook wrote:
> oshima, do you want me to look now, or do you want to take a look at peter's
> comments first?

let me address peter's comments first and I'll let you know.

[oshima, 2013-10-28 22:57:50]

> Also, were you planning on moving the browser tests in
> chrome_shell_delegate_browsertest.cc as part of this CL?

No I can't because it depends on chrome impl. I renamed to
accelerator_commands_browsertest.cc instead.

https://codereview.chromium.org/42353002/diff/240001/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/42353002/diff/240001/ash/wm/window_state.cc#n...
ash/wm/window_state.cc:164: void WindowState::ToggleFullscreen() {
On 2013/10/26 03:46:34, pkotwicz wrote:
> You probably want to check CanMaximize() in this function. Otherwise you will
be
> able to use <F4> to maximize the bookmark bubble and other non maximizable
> widgets.

Good point. Done.

https://codereview.chromium.org/42353002/diff/240001/ash/wm/window_state_dele...
File ash/wm/window_state_delegate.h (right):

https://codereview.chromium.org/42353002/diff/240001/ash/wm/window_state_dele...
ash/wm/window_state_delegate.h:20: // Invoked when the user uses Shift+F4/F4g to
toggle the window
On 2013/10/25 23:31:14, pkotwicz wrote:
> Shift+F4/F4g -> F4

I kept Shift+F4 because toggle fullscreen is still assigned to it.
There is a discussion about removing it, and this will be addressed separately.

https://codereview.chromium.org/42353002/diff/240001/ash/wm/window_state_dele...
ash/wm/window_state_delegate.h:21: // fullscreen state.  |WindowState| fallbacks
to the default
On 2013/10/25 23:31:14, pkotwicz wrote:
> Nit: WindowState -> |window_state|

This is intentional as it refers to the caller (technically identical, but
I want to emphasize that it's caller). Updated the comment to make this point
clear.

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

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:24:
BrowserWindowStateDelegate() {}
On 2013/10/25 23:31:14, pkotwicz wrote:
> I would much rather have a Browser* parameter be passed in.

Done.

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:27: // Invoked when the user
uses Shift+F4 to toggle the window fullscreen state.
On 2013/10/25 23:31:14, pkotwicz wrote:
> Shift+F4 -> F4
Changed to Shift+F4/F4

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:41: //
AppNonClientFrameViewAsh shows only the window controls and no other
On 2013/10/25 23:31:14, pkotwicz wrote:
> Would it be possible to set a custom delegate in AppNonClientFrameViewAsh to
do
> this handling? This change will make me happy :)
> This changes the caveat that WindowState::SetDelegate() will only be called
once
> for a given WindowState object. (I will need to change that anyways for
> CustomFrameViewAsh though)

Done.

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:98: // Don't do anything if
we don't have our non-client view yet.
On 2013/10/25 23:34:14, pkotwicz wrote:
> I was going to remove it as part of https://codereview.chromium.org/25536010/
> but we did a complete redesign

I'm happy to create CL, but may I do this in separate CL? I want to make sure my
CL won't change the behavior.

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:137: if
(!browser_view->browser()->bounds_overridden() &&
On 2013/10/25 23:31:14, pkotwicz wrote:
> Might as well use |browser| in the remainder of the function

Done.

[oshima, 2013-10-28 23:05:17]

Looks like the patch had merge problem. I'll update new patch and let you know.

[oshima, 2013-10-28 23:44:37]

uploaded new patch. peter, james, PTAL.

[pkotwicz, 2013-10-29 00:46:00]

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

https://codereview.chromium.org/42353002/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:98: // Don't do anything if
we don't have our non-client view yet.
I'll post a CL to remove this tomorrow (Tuesday)

https://codereview.chromium.org/42353002/diff/420001/ash/accelerators/acceler...
File ash/accelerators/accelerator_commands.h (right):

https://codereview.chromium.org/42353002/diff/420001/ash/accelerators/acceler...
ash/accelerators/accelerator_commands.h:25: // maximized, it exits maximized
mode. The behavior can be overridden
"If the window is already in maximized, it exits maximized mode." is not true

https://codereview.chromium.org/42353002/diff/420001/ash/wm/window_state_dele...
File ash/wm/window_state_delegate.h (right):

https://codereview.chromium.org/42353002/diff/420001/ash/wm/window_state_dele...
ash/wm/window_state_delegate.h:21: // fullscreen state.  The caller
(|ash::wm::WindowState|) fallbacks
Nits:
|ash::wm::WindowState| -> ash::wm::WindowState

fallbacks -> falls back

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

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/ash/ac...
chrome/browser/ui/ash/accelerator_commands_browsertest.cc:9: #include
"ash/ash_switches.h"
Nit: You do not need to include "ash/ash_switches.h" or "base/command_line.h"
either (I probably forgot to remove the includes)

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/ash/ac...
chrome/browser/ui/ash/accelerator_commands_browsertest.cc:16: #include
"chrome/browser/ui/browser_commands.h"
browser_commands.h is also unnecessary

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:139: DCHECK_EQ(
Nit: remove the DCHECK

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:145: if (is_fullscreen
|| window_state->CanMaximize()) {
I personally think that testing CanMaximize() here is redundant. But I do not
feel very strongly on this point

If you do keep the check for CanMaximize(), I would rather do this:
if (is_fullscreen) {
  shell_window_->Restore();
} else {
  if (window_state->CanMaximize())
    shell_window_->Fullscreen();
}

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

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:91:
ash::wm::GetWindowState(GetNativeWindow());
This class needs a WindowStateDelegate.
With the CL as is, pressing <F4> while in tab fullscreen does not get rid of the
fullscreen exit bubble

https://codereview.chromium.org/42353002/diff/420001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/42353002/diff/420001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:1416:
'browser/ui/ash/accelerator_commands_browsertest.cc',
Nit: alphabetize

[oshima, 2013-10-29 02:05:41]

https://codereview.chromium.org/42353002/diff/420001/ash/wm/window_state_dele...
File ash/wm/window_state_delegate.h (right):

https://codereview.chromium.org/42353002/diff/420001/ash/wm/window_state_dele...
ash/wm/window_state_delegate.h:21: // fullscreen state.  The caller
(|ash::wm::WindowState|) fallbacks
On 2013/10/29 00:46:01, pkotwicz wrote:
> Nits:
> |ash::wm::WindowState| -> ash::wm::WindowState
> 
> fallbacks -> falls back

Done.

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

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/ash/ac...
chrome/browser/ui/ash/accelerator_commands_browsertest.cc:9: #include
"ash/ash_switches.h"
On 2013/10/29 00:46:01, pkotwicz wrote:
> Nit: You do not need to include "ash/ash_switches.h" or "base/command_line.h"
> either (I probably forgot to remove the includes)

Done.

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/ash/ac...
chrome/browser/ui/ash/accelerator_commands_browsertest.cc:16: #include
"chrome/browser/ui/browser_commands.h"
On 2013/10/29 00:46:01, pkotwicz wrote:
> browser_commands.h is also unnecessary

This is necessary. (ToggleFullscreenMode)

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:139: DCHECK_EQ(
On 2013/10/29 00:46:01, pkotwicz wrote:
> Nit: remove the DCHECK

Done.

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:145: if (is_fullscreen
|| window_state->CanMaximize()) {
On 2013/10/29 00:46:01, pkotwicz wrote:
> I personally think that testing CanMaximize() here is redundant. But I do not
> feel very strongly on this point
> 
> If you do keep the check for CanMaximize(), I would rather do this:
> if (is_fullscreen) {
>   shell_window_->Restore();
> } else {
>   if (window_state->CanMaximize())
>     shell_window_->Fullscreen();
> }

Done.

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

https://codereview.chromium.org/42353002/diff/420001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:91:
ash::wm::GetWindowState(GetNativeWindow());
On 2013/10/29 00:46:01, pkotwicz wrote:
> This class needs a WindowStateDelegate.
> With the CL as is, pressing <F4> while in tab fullscreen does not get rid of
the
> fullscreen exit bubble

Sorry I misunderstood your original comment. Fixed.

https://codereview.chromium.org/42353002/diff/420001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/42353002/diff/420001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:1416:
'browser/ui/ash/accelerator_commands_browsertest.cc',
On 2013/10/29 00:46:01, pkotwicz wrote:
> Nit: alphabetize

Done.

[James Cook, 2013-10-29 16:13:04]

LGTM with small nits

https://codereview.chromium.org/42353002/diff/560001/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/42353002/diff/560001/ash/wm/window_state.h#ne...
ash/wm/window_state.h:9: #include "ash/wm/window_state_delegate.h"
nit: Do you need this include?  It doesn't look like it, but if you need it you
probably don't need the forward declaration below.

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

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/ash/ac...
chrome/browser/ui/ash/accelerator_commands_browsertest.cc:5: #include
"ash/accelerators/accelerator_commands.h"
nit: Maybe comment inside accelerator_commands_unittest.cc that this test suite
also exists, so the next programmer has an idea where to add tests?

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate.cc:24: #include
"chrome/browser/ui/browser_finder.h"
nit: Is this include still needed?  (It might be, I just don't see any function
calls starting with Find*)

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:128: class
NativeAppWindowStateDelegate : public ash::wm::WindowStateDelegate {
nit: Class comment saying this is needed for fullscreen?

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

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:44: class
AppWindowStateDelegate : public ash::wm::WindowStateDelegate {
nit: A class comment mentioning which type of app window would be nice.

[pkotwicz, 2013-10-29 16:45:13]

LGTM

https://codereview.chromium.org/42353002/diff/560001/ash/accelerators/acceler...
File ash/accelerators/accelerator_commands.h (right):

https://codereview.chromium.org/42353002/diff/560001/ash/accelerators/acceler...
ash/accelerators/accelerator_commands.h:25: // by
|WindowStateDelegate::ToggleFullscreen()|.
Nit: WindowStateDelegate::ToggleFullscreen() ->
WindowStateDelegate::ToggleFullscreen()

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:8: #include
"apps/shell_window_registry.h"
Nit: You do not need the above include anymore

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:139: // Windows which
cannot be maximized should not be fullscreened.
Nit: Move comment above DCHECK?

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

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:55: //
AppNonClientFrameViewAsh shows only the window controls and no other
This comment should probably probably be moved so that it is a comment for the
entire AppWindowStateDelegate class.

[oshima, 2013-10-29 17:45:52]

https://codereview.chromium.org/42353002/diff/560001/ash/accelerators/acceler...
File ash/accelerators/accelerator_commands.h (right):

https://codereview.chromium.org/42353002/diff/560001/ash/accelerators/acceler...
ash/accelerators/accelerator_commands.h:25: // by
|WindowStateDelegate::ToggleFullscreen()|.
On 2013/10/29 16:45:14, pkotwicz wrote:
> Nit: WindowStateDelegate::ToggleFullscreen() ->
> WindowStateDelegate::ToggleFullscreen()

It looks to me that you typed exactly same? Do you want to remove ||?

https://codereview.chromium.org/42353002/diff/560001/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/42353002/diff/560001/ash/wm/window_state.h#ne...
ash/wm/window_state.h:9: #include "ash/wm/window_state_delegate.h"
On 2013/10/29 16:13:04, James Cook wrote:
> nit: Do you need this include?  It doesn't look like it, but if you need it
you
> probably don't need the forward declaration below.

Done.

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

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/ash/ac...
chrome/browser/ui/ash/accelerator_commands_browsertest.cc:5: #include
"ash/accelerators/accelerator_commands.h"
On 2013/10/29 16:13:04, James Cook wrote:
> nit: Maybe comment inside accelerator_commands_unittest.cc that this test
suite
> also exists, so the next programmer has an idea where to add tests?

Done.

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate.cc:24: #include
"chrome/browser/ui/browser_finder.h"
On 2013/10/29 16:13:04, James Cook wrote:
> nit: Is this include still needed?  (It might be, I just don't see any
function
> calls starting with Find*)

Done.

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:8: #include
"apps/shell_window_registry.h"
On 2013/10/29 16:45:14, pkotwicz wrote:
> Nit: You do not need the above include anymore

Done.

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:128: class
NativeAppWindowStateDelegate : public ash::wm::WindowStateDelegate {
On 2013/10/29 16:13:04, James Cook wrote:
> nit: Class comment saying this is needed for fullscreen?

Done.

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:139: // Windows which
cannot be maximized should not be fullscreened.
On 2013/10/29 16:45:14, pkotwicz wrote:
> Nit: Move comment above DCHECK?

Done.

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

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:44: class
AppWindowStateDelegate : public ash::wm::WindowStateDelegate {
On 2013/10/29 16:13:04, James Cook wrote:
> nit: A class comment mentioning which type of app window would be nice.

Done.

https://codereview.chromium.org/42353002/diff/560001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash.cc:55: //
AppNonClientFrameViewAsh shows only the window controls and no other
On 2013/10/29 16:45:14, pkotwicz wrote:
> This comment should probably probably be moved so that it is a comment for the
> entire AppWindowStateDelegate class.

Done.

[commit-bot: I haz the power, 2013-10-29 18:45:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/42353002/690001

[oshima, 2013-10-29 19:22:56]

sky -> c/b/ui/views

[sky, 2013-10-29 20:52:21]

LGTM

https://codereview.chromium.org/42353002/diff/690001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/42353002/diff/690001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:188:
ash::wm::GetWindowState(GetNativeWindow())->SetDelegate(
Can SetDelegate take a scoped_ptr so I don't have to look through the code to
figure out ownership?

[oshima, 2013-10-29 22:54:41]

https://codereview.chromium.org/42353002/diff/690001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/42353002/diff/690001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/native_app_window_views.cc:188:
ash::wm::GetWindowState(GetNativeWindow())->SetDelegate(
On 2013/10/29 20:52:21, sky wrote:
> Can SetDelegate take a scoped_ptr so I don't have to look through the code to
> figure out ownership?

Done.

[sky, 2013-10-29 23:13:27]

Thanks, SLGTM

[commit-bot: I haz the power, 2013-10-29 23:16:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/42353002/780001

[commit-bot: I haz the power, 2013-10-30 04:06:49]

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, 2013-10-30 05:02:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/42353002/1050001

[commit-bot: I haz the power, 2013-10-30 17:14:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/42353002/1260001

[commit-bot: I haz the power, 2013-10-30 20:20:15]

Change committed as 231903