Add flag to enable immersive fullscreen for v2 apps

Add ash-enable-immersive-all-windows command line flag to enable immersive fullscreen for non-frameless v2 apps. (The goal is to add support for immersive fullscreen for v1 apps and miscellaneous windows such as the task manager shortly) When the flag is set, <F4> will put the app into immersive fullscreen.

In immersive fullscreen, the window header (the title bar and window controls) slide out as an overlay over the web contents when the mouse is hovered at the top of the display.

chrome.app.window.current().isFullscreen() returns false for immersive fullscreen windows
Entering fullscreen via the app.window api or webkitRequestFullScreen() quits immersive fullscreen.

BUG=307622
R=oshima,benwells
TBR=sky (For adding VIEWS_EXPORT to ui/views/widget/widget_observer.h)
TEST=Manual, see steps below:
1) Install: https://chrome.google.com/webstore/detail/window-state-sample/hcbhfbnaaancmblfhdknlnojpafjohbi
2) Enable "Enable immersive fullscreen for non browser windows" in chrome://flags
3) Hit <F4>
4) Ensure that the "isFullscreen" checkbox in the app is checked
5) Ensure that the shelf light bar is visible (The shelf is not completely hidden)
6) Hover the mouse at the top of the screen. Ensure that the window header (maximize and close button slide down and overlay the web contents)
7) Move the mouse off of the header. Ensure that the window header slides off screen
8) Press the Fullscreen button in the app (It may be useful to set the "chrome.app.window actions delay" to zero seconds)
9) Ensure that the shelf is completely hidden.
10) Hover the mouse at the top of the screen. Ensure that the header slides out again.
11) Press <F4>
12) Ensure that the "isFullscreen" checkbox is unchecked
13) Ensure that the window header is visible and does not slide out

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

[pkotwicz, 2013-11-07 06:21:36]

oshima@, can you please take a look? Can you take a preliminary look at the
changes to apps/

benwells@, I have cced you. I am hoping oshima@ can do a preliminary review of
the apps code. However, I would love to get your feedback on the design
(https://docs.google.com/a/google.com/document/d/1-7oBLvr7Amb5hge3Nau85W8KK0jC...)
The main goal is to make v1 and v2 apps use the same window frame and move away
from the really weird v1 app maximized frame on ChromeOS. This CL will provide a
power user feature on ChromeOS which will allow a user to put any window into
fullscreen.

[oshima, 2013-11-07 18:56:12]

https://codereview.chromium.org/59043013/diff/30001/apps/shell_window.h
File apps/shell_window.h (right):

https://codereview.chromium.org/59043013/diff/30001/apps/shell_window.h#newco...
apps/shell_window.h:109: FULLSCREEN_TYPE_IMMERSIVE,
Can you explain in the comment how other three types are different like this
one? (in addition to how it's activated)

https://codereview.chromium.org/59043013/diff/30001/apps/shell_window.h#newco...
apps/shell_window.h:310: // window does not support immersive fullscreen.
You can just refer to enum description.

https://codereview.chromium.org/59043013/diff/30001/apps/shell_window.h#newco...
apps/shell_window.h:312: 
nuke extra new line

https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc#newco...
ash/ash_switches.cc:99: "ash-enable-app-immersive-fullscreen";
shouldn't this be in apps/switches? (ash doesn't have app concept)

https://codereview.chromium.org/59043013/diff/30001/ash/wm/caption_buttons/fr...
File ash/wm/caption_buttons/frame_maximize_button_observer.h (right):

https://codereview.chromium.org/59043013/diff/30001/ash/wm/caption_buttons/fr...
ash/wm/caption_buttons/frame_maximize_button_observer.h:13: virtual void
OnMaximizeBubbleShown(views::Widget* bubble) = 0;
don't you have to declare views::Widget?

https://codereview.chromium.org/59043013/diff/30001/ash/wm/custom_frame_view_...
File ash/wm/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/59043013/diff/30001/ash/wm/custom_frame_view_...
ash/wm/custom_frame_view_ash.cc:49: HeaderView(views::Widget* frame);
explicit

https://codereview.chromium.org/59043013/diff/30001/ash/wm/window_state_deleg...
File ash/wm/window_state_delegate.h (right):

https://codereview.chromium.org/59043013/diff/30001/ash/wm/window_state_deleg...
ash/wm/window_state_delegate.h:25: virtual bool
EnterImmersiveFullscreen(WindowState* window_state);
I still think we should call EnterFullscreen because it may or may not go
ImmersiveFullscreen, and ImmersiveFullscreen is a variant of Fullscreen, but not
the other way around.

https://codereview.chromium.org/59043013/diff/30001/ash/wm/window_state_deleg...
ash/wm/window_state_delegate.h:30: virtual bool ExitFullscreen(WindowState*
window_state);
Thanks, I'll look into restoring from fullscreen state.

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:430: void
NativeAppWindowCocoa::SetFullscreen(ShellWindow::FullscreenType type) {
Is it worth DCHECKing not immersive here?

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/views/a...
chrome/browser/ui/views/apps/native_app_window_views.cc:161:
shell_window_->Fullscreen();
Isn't it better to do all these check and immersive/non immersive fullscreen in
ShellWindow::Fullscreen() ?

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/views/a...
chrome/browser/ui/views/apps/native_app_window_views.cc:706: #if
defined(OS_CHROMEOS)
why this is chromeos only? shouldn't we use the same for win8/metro?

[pkotwicz, 2013-11-07 22:21:48]

Oshima, can you please take another look?

https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc#newco...
ash/ash_switches.cc:99: "ash-enable-app-immersive-fullscreen";
It should probably stay here because the switch will apply to both v1 and v2
apps. My understanding is that most of the code in apps/ is for v2 apps.

https://codereview.chromium.org/59043013/diff/30001/ash/wm/window_state_deleg...
File ash/wm/window_state_delegate.h (right):

https://codereview.chromium.org/59043013/diff/30001/ash/wm/window_state_deleg...
ash/wm/window_state_delegate.h:25: virtual bool
EnterImmersiveFullscreen(WindowState* window_state);
The reason that I want to call this EnterImmersiveFullscreen() is that the
method enters immersive fullscreen if possible. It does not enter fullscreen
with the window's "default" fullscreen type. FULLSCREEN_TYPE_WINDOW is the
"default" fullscreen type for v2 apps.

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:430: void
NativeAppWindowCocoa::SetFullscreen(ShellWindow::FullscreenType type) {
Personally, I would rather skip the DCHECK() given that
NativeAppWindowCocoa::SupportsImmersiveFullscreen() returns false

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/views/a...
chrome/browser/ui/views/apps/native_app_window_views.cc:706: #if
defined(OS_CHROMEOS)
This is chromeos only because browser immersive fullscreen is chromeos only. I
think that at the very least there is additional complexity in making it
ChromeOS only, hence the TODO.

[oshima, 2013-11-08 00:26:29]

https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc#newco...
ash/ash_switches.cc:99: "ash-enable-app-immersive-fullscreen";
On 2013/11/07 22:21:48, pkotwicz wrote:
> It should probably stay here because the switch will apply to both v1 and v2
> apps. My understanding is that most of the code in apps/ is for v2 apps.

If that's the case, this should be in chrome/common

https://codereview.chromium.org/59043013/diff/30001/ash/wm/window_state_deleg...
File ash/wm/window_state_delegate.h (right):

https://codereview.chromium.org/59043013/diff/30001/ash/wm/window_state_deleg...
ash/wm/window_state_delegate.h:25: virtual bool
EnterImmersiveFullscreen(WindowState* window_state);
On 2013/11/07 22:21:48, pkotwicz wrote:
> The reason that I want to call this EnterImmersiveFullscreen() is that the
> method enters immersive fullscreen if possible. It does not enter fullscreen
> with the window's "default" fullscreen type. FULLSCREEN_TYPE_WINDOW is the
> "default" fullscreen type for v2 apps.
> 

But it doesn't enter ImmersiveFullscreen when not possible either right?

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/views/a...
chrome/browser/ui/views/apps/native_app_window_views.cc:706: #if
defined(OS_CHROMEOS)
On 2013/11/07 22:21:48, pkotwicz wrote:
> This is chromeos only because browser immersive fullscreen is chromeos only. I
> think that at the very least there is additional complexity in making it
> ChromeOS only, hence the TODO.
> 

So win8 ash mode doesn't support immersive fullscreen browser?

[pkotwicz, 2013-11-08 01:16:56]

https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc#newco...
ash/ash_switches.cc:99: "ash-enable-app-immersive-fullscreen";
I do not think that we want to eventually support immersive fullscreen on all
platforms. But I can move the flag to chrome/common if you prefer.

https://codereview.chromium.org/59043013/diff/30001/ash/wm/window_state_deleg...
File ash/wm/window_state_delegate.h (right):

https://codereview.chromium.org/59043013/diff/30001/ash/wm/window_state_deleg...
ash/wm/window_state_delegate.h:25: virtual bool
EnterImmersiveFullscreen(WindowState* window_state);
I can split EnterImmersiveFullscreen() into two methods:
EnterImmersiveFullscreen() and EnterDefaultFullscreen()/EnterFullscreen() if
that would make things clearer.

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/59043013/diff/30001/chrome/browser/ui/views/a...
chrome/browser/ui/views/apps/native_app_window_views.cc:706: #if
defined(OS_CHROMEOS)
No

[oshima, 2013-11-08 18:22:29]

Regaring EnterImmersiveFullscreen vs EnterFullscreen, I read through the code
again and here are my thoughts.

1) I think having separate methods is overkill. I also don't think ash can tell
which one to use  (can it?), and adding such method is even more overskill.

2) If immersive mode is cros only, we should call it InterFullscreen as because
it doesn't go into immersive mode on win8. Note that ash will be UI on win8.
(another candidate is EnterFullscreenOrImmersiveFullscreen, but it's ugly)

3) If win8 will also supports Immersive mode, I believe all windows will enter
immersive mode by fullscreen key, unless it's already in fullscreen state.
(Correct me if I'm wrong) I'm ok to call EnterImmersiveFullscreen, although I
still prefer to call it Immersive because using Immersive is chrome decision,
not ash decision.

+james in case his has an opinion.

https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc#newco...
ash/ash_switches.cc:99: "ash-enable-app-immersive-fullscreen";
On 2013/11/08 01:16:57, pkotwicz wrote:
> I do not think that we want to eventually support immersive fullscreen on all
> platforms. But I can move the flag to chrome/common if you prefer.

Ash should not know about apps v1/v2 stuff, and this flag is used in
chrome/browser, not in ash, so please move to chrome/common.

[oshima, 2013-11-08 18:23:15]

On 2013/11/08 18:22:29, oshima wrote:
> Regaring EnterImmersiveFullscreen vs EnterFullscreen, I read through the code
> again and here are my thoughts.
> 
> 1) I think having separate methods is overkill. I also don't think ash can
tell
> which one to use  (can it?), and adding such method is even more overskill.
> 
> 2) If immersive mode is cros only, we should call it InterFullscreen as
because
> it doesn't go into immersive mode on win8. Note that ash will be UI on win8.
> (another candidate is EnterFullscreenOrImmersiveFullscreen, but it's ugly)
> 
> 3) If win8 will also supports Immersive mode, I believe all windows will enter
> immersive mode by fullscreen key, unless it's already in fullscreen state.
> (Correct me if I'm wrong) I'm ok to call EnterImmersiveFullscreen, although I
> still prefer to call it Immersive because using Immersive is chrome decision,
> not ash decision.

opps, I meant

still prefer to call it EnterFullscreen

> 
> +james in case his has an opinion.
> 
> https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc
> File ash/ash_switches.cc (right):
> 
>
https://codereview.chromium.org/59043013/diff/30001/ash/ash_switches.cc#newco...
> ash/ash_switches.cc:99: "ash-enable-app-immersive-fullscreen";
> On 2013/11/08 01:16:57, pkotwicz wrote:
> > I do not think that we want to eventually support immersive fullscreen on
all
> > platforms. But I can move the flag to chrome/common if you prefer.
> 
> Ash should not know about apps v1/v2 stuff, and this flag is used in
> chrome/browser, not in ash, so please move to chrome/common.

[James Cook, 2013-11-08 20:58:16]

On 2013/11/08 18:23:15, oshima wrote:
> On 2013/11/08 18:22:29, oshima wrote:
> > Regaring EnterImmersiveFullscreen vs EnterFullscreen, I read through the
code
> > again and here are my thoughts.
> > 
> > 1) I think having separate methods is overkill. I also don't think ash can
> tell
> > which one to use  (can it?), and adding such method is even more overskill.
> > 
> > 2) If immersive mode is cros only, we should call it InterFullscreen as
> because
> > it doesn't go into immersive mode on win8. Note that ash will be UI on win8.
> > (another candidate is EnterFullscreenOrImmersiveFullscreen, but it's ugly)
> > 
> > 3) If win8 will also supports Immersive mode, I believe all windows will
enter
> > immersive mode by fullscreen key, unless it's already in fullscreen state.
> > (Correct me if I'm wrong) I'm ok to call EnterImmersiveFullscreen, although
I
> > still prefer to call it Immersive because using Immersive is chrome
decision,
> > not ash decision.
> 
> opps, I meant
> 
> still prefer to call it EnterFullscreen
> 
> > 
> > +james in case his has an opinion.

I don't really have an opinion here. The proliferation of fullscreen types is
confusing (shell_window.h) but I don't see a better way to do it.

[pkotwicz, 2013-11-08 22:36:10]

ash::wm::WindowStateDelegate::ToggleFullscreen() is back. Oshima can you please
take another look?

[oshima, 2013-11-08 23:25:32]

https://codereview.chromium.org/59043013/diff/400031/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/59043013/diff/400031/ash/ash_switches.h#newco...
ash/ash_switches.h:46: ASH_EXPORT extern const char
kAshEnableAppImmersiveFullscreen[];
chrome/common/chrome_switches?

https://codereview.chromium.org/59043013/diff/400031/ash/wm/caption_buttons/f...
File ash/wm/caption_buttons/frame_caption_button_container_view.h (right):

https://codereview.chromium.org/59043013/diff/400031/ash/wm/caption_buttons/f...
ash/wm/caption_buttons/frame_caption_button_container_view.h:82:
FrameMaximizeButton* GetOldStyleSizeButton() const;
no const when returning non const internal object.

https://codereview.chromium.org/59043013/diff/400031/ash/wm/caption_buttons/f...
File ash/wm/caption_buttons/frame_maximize_button_observer.h (right):

https://codereview.chromium.org/59043013/diff/400031/ash/wm/caption_buttons/f...
ash/wm/caption_buttons/frame_maximize_button_observer.h:1: #ifndef
ASH_WM_CAPTION_BUTTONS_FRAME_MAXIMIZE_BUTTON_OBSERVER_H_
copyright

https://codereview.chromium.org/59043013/diff/400031/ash/wm/custom_frame_view...
File ash/wm/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/59043013/diff/400031/ash/wm/custom_frame_view...
ash/wm/custom_frame_view_ash.cc:185: canvas,
4 indent

https://codereview.chromium.org/59043013/diff/400031/ash/wm/custom_frame_view...
File ash/wm/custom_frame_view_ash.h (right):

https://codereview.chromium.org/59043013/diff/400031/ash/wm/custom_frame_view...
ash/wm/custom_frame_view_ash.h:29: // packaged apps, dialogs and other
non-browser windows. When in immersive
Can you rephrase so that whether or not this is used depends on the
WindowStateDelegate::EnterFullscreen implementation, and cros uses this for
packaged apps, and non browser windows (but not on win8)?

https://codereview.chromium.org/59043013/diff/400031/ash/wm/custom_frame_view...
ash/wm/custom_frame_view_ash.h:43: // and hides |header_view_| in immersive
fullscreen.
can you mention ownership?

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

https://codereview.chromium.org/59043013/diff/400031/ash/wm/window_state_dele...
ash/wm/window_state_delegate.h:26: virtual bool ToggleFullscreen(WindowState*
window_state);
Why you reverted Enter/Exit change? Was there any issue? Sorry if it sounded
like I didn't like Enter/Exit split (I'm fine with it).

[pkotwicz, 2013-11-09 00:09:02]

Changes as requested. Oshima, can you please take another look?

https://codereview.chromium.org/59043013/diff/400031/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/59043013/diff/400031/ash/ash_switches.h#newco...
ash/ash_switches.h:46: ASH_EXPORT extern const char
kAshEnableAppImmersiveFullscreen[];
I am keeping it here. In terms of precedent, --ash-enable-immersive-fullscreen
was also in ash_switches.h (See https://chromiumcodereview.appspot.com/14366004)

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

https://codereview.chromium.org/59043013/diff/400031/ash/wm/window_state_dele...
ash/wm/window_state_delegate.h:26: virtual bool ToggleFullscreen(WindowState*
window_state);
There is no reason to making the Enter/Exit change at this point. I made the
change to distinguish that EnterImmersiveFullscreen() and ExitFullscreen() are
different. In particular, ExitFullscreen() always exits fullscreen regardless of
the type while EnterImmersiveFullscreen() only enters immersive fullscreen.

[oshima, 2013-11-09 01:11:17]

lgtm if the flag is moved.

https://codereview.chromium.org/59043013/diff/400031/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/59043013/diff/400031/ash/ash_switches.h#newco...
ash/ash_switches.h:46: ASH_EXPORT extern const char
kAshEnableAppImmersiveFullscreen[];
On 2013/11/09 00:09:02, pkotwicz wrote:
> I am keeping it here. In terms of precedent, --ash-enable-immersive-fullscreen
> was also in ash_switches.h (See
https://chromiumcodereview.appspot.com/14366004)

That flag shouldn't have been in ash first place IMO. Flag should be in the
component where the feature is toggled.

[pkotwicz, 2013-11-13 00:10:12]

benwells@, can you please look at the changes in apps/ and
chrome/browser/ui/*/apps

oshima@, can you please take another look I fixed a few bugs with my changes in
CustomFrameViewAsh

https://codereview.chromium.org/59043013/diff/400031/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/59043013/diff/400031/ash/ash_switches.h#newco...
ash/ash_switches.h:46: ASH_EXPORT extern const char
kAshEnableAppImmersiveFullscreen[];
We unfortunately need to keep the flag in ash. I will need to use it (from
CustomFrameViewAsh) in order to implement putting non-app windows like the task
manager into immersive fullscreen.
I plan on implementing this via a custom WindowStateDelegate if
|WindowState::delegate_| is non-NULL

I have renamed the flag to kAshEnableImmersiveFullscreenForAllWindows to make
its intent clearer

https://codereview.chromium.org/59043013/diff/600001/ash/wm/custom_frame_view...
File ash/wm/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/59043013/diff/600001/ash/wm/custom_frame_view...
ash/wm/custom_frame_view_ash.cc:342:
header_view_->set_paint_as_active(ShouldPaintAsActive());
In a separate CL, I will remove 
NonClientFrameView::ShouldPaintAsActiveChanged() because it is confusing.

https://codereview.chromium.org/59043013/diff/1070001/apps/app_window_content...
File apps/app_window_contents.cc (right):

https://codereview.chromium.org/59043013/diff/1070001/apps/app_window_content...
apps/app_window_contents.cc:111: // for the sake of the app.window api.
I am not sure whether telling the app that we are not fullscreen when in
immersive fullscreen is worth it.
With the code as is, chrome.app.window.current().maximize() exits immersive
fullscreen, so I should probably fix that.

benwells@, what do you think?

https://codereview.chromium.org/59043013/diff/1070001/apps/shell_window.h
File apps/shell_window.h (right):

https://codereview.chromium.org/59043013/diff/1070001/apps/shell_window.h#new...
apps/shell_window.h:99: FULLSCREEN_TYPE_WINDOW,
I suspect that we want to use this type of fullscreen for "MacOS Lion
Fullscreen"

[oshima, 2013-11-13 07:21:36]

just for my understanding. where is the height of immersive mode's light bar is
specified and handled?

https://codereview.chromium.org/59043013/diff/400031/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/59043013/diff/400031/ash/ash_switches.h#newco...
ash/ash_switches.h:46: ASH_EXPORT extern const char
kAshEnableAppImmersiveFullscreen[];
On 2013/11/13 00:10:13, pkotwicz wrote:
> We unfortunately need to keep the flag in ash. I will need to use it (from
> CustomFrameViewAsh) in order to implement putting non-app windows like the
task
> manager into immersive fullscreen.
> I plan on implementing this via a custom WindowStateDelegate if
> |WindowState::delegate_| is non-NULL
> 
> I have renamed the flag to kAshEnableImmersiveFullscreenForAllWindows to make
> its intent clearer

Ok.

[benwells, 2013-11-13 07:41:27]

https://codereview.chromium.org/59043013/diff/1070001/apps/app_window_content...
File apps/app_window_contents.cc (right):

https://codereview.chromium.org/59043013/diff/1070001/apps/app_window_content...
apps/app_window_contents.cc:111: // for the sake of the app.window api.
On 2013/11/13 00:10:13, pkotwicz wrote:
> I am not sure whether telling the app that we are not fullscreen when in
> immersive fullscreen is worth it.
> With the code as is, chrome.app.window.current().maximize() exits immersive
> fullscreen, so I should probably fix that.
> 
> benwells@, what do you think?

I think it is probably full screen enough to say it is fullsceen. If it isn't,
we shouldn't have a FULLSCREEN_TYPE_IMMERSIVE but should do it some other way.

Why did you remove IsFullscreenOrPending? Was it only used here? I'm not super
familiar with this, but can you replace this call to just:
fullscreen = fullscreen_type != ShellWindow::FULLSCREEN_TYPE_NONE? That is, do
you need to check with the native_app_window at all?

https://codereview.chromium.org/59043013/diff/1070001/apps/shell_window.h
File apps/shell_window.h (right):

https://codereview.chromium.org/59043013/diff/1070001/apps/shell_window.h#new...
apps/shell_window.h:99: FULLSCREEN_TYPE_WINDOW,
On 2013/11/13 00:10:13, pkotwicz wrote:
> I suspect that we want to use this type of fullscreen for "MacOS Lion
> Fullscreen"

Can we call this FULLSCREEN_TYPE_WINDOW_API? or FULLSCREEN_TYPE_CHROME_API, or
FULLSCREEN_TYPE_CHROME?

Why would we use this for Mac OS X full screen? That feels more analogous to
FULLSCREEN_TYPE_IMMERSIVE, which might be better named FULLSCREEN_TYPE_OS.

https://codereview.chromium.org/59043013/diff/1070001/apps/shell_window.h#new...
apps/shell_window.h:104: FULLSCREEN_TYPE_TAB,
This would also be better as FULLSCREEN_TYPE_HTML_API, I think, or just
FULLSCREEN_TYPE_HTML.

https://codereview.chromium.org/59043013/diff/1070001/apps/ui/native_app_wind...
File apps/ui/native_app_window.h (right):

https://codereview.chromium.org/59043013/diff/1070001/apps/ui/native_app_wind...
apps/ui/native_app_window.h:24: // Returns true if the window supports immersive
fullscreen.
Maybe this is bikeshedding, but 'immersive fullscreen' feels like a tautology.
Is there non-immersive fullscreen?

[pkotwicz, 2013-11-13 22:06:00]

benwells@ can you please take another look?

https://codereview.chromium.org/59043013/diff/1070001/apps/app_window_content...
File apps/app_window_contents.cc (right):

https://codereview.chromium.org/59043013/diff/1070001/apps/app_window_content...
apps/app_window_contents.cc:111: // for the sake of the app.window api.
IsFullscreenOrPending() is back. I removed it in the previous CL because there
was starting to be many fullscreen related methods in NativeAppWindow.

I changed |fullscreen_type| to be a bit field. This allows all of the fullscreen
types to be combined together.
This allows a user to be in both OS fullscreen and HTML fullscreen which we
allow for browser windows as well.

https://codereview.chromium.org/59043013/diff/1070001/apps/ui/native_app_wind...
File apps/ui/native_app_window.h (right):

https://codereview.chromium.org/59043013/diff/1070001/apps/ui/native_app_wind...
apps/ui/native_app_window.h:24: // Returns true if the window supports immersive
fullscreen.
I agree that immersive fullscreen is not a good name, especially given that
immersive fullscreen is less immersive than "traditional/normal fullscreen.
There is a historical reason why we use the name, but eventually we should
probably rename the feature.

[benwells, 2013-11-14 00:06:53]

Does going into fullscreen on a Mac via the window frame control go into OS
fullscreen on Lion?

https://codereview.chromium.org/59043013/diff/1280002/apps/shell_window.cc
File apps/shell_window.cc (right):

https://codereview.chromium.org/59043013/diff/1280002/apps/shell_window.cc#ne...
apps/shell_window.cc:420: fullscreen_types_ = FULLSCREEN_TYPE_NONE;
Is there a logical change here? It was checking IsFullscreenOrPending before.

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

https://codereview.chromium.org/59043013/diff/1280002/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_views.cc:147: // TODO(pkotwicz):
This is a hack. Remove ASAP.
Could you file a bug for this?

[pkotwicz, 2013-11-14 01:05:16]

No, the ShellWindow is not notified when the user enters/exits fullscreen via
the MacOS window frame control. (crbug.com/230664) I do not mind fixing this in
a separate CL. We have this working for browser windows.

[pkotwicz, 2013-11-14 01:05:24]

https://codereview.chromium.org/59043013/diff/1280002/apps/shell_window.cc
File apps/shell_window.cc (right):

https://codereview.chromium.org/59043013/diff/1280002/apps/shell_window.cc#ne...
apps/shell_window.cc:420: fullscreen_types_ = FULLSCREEN_TYPE_NONE;
This is not a logical change as far as I can tell. (It better not be a logical
change given that IsFullscreenForTabOrPending() checks |fullscreen_types_|)

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

https://codereview.chromium.org/59043013/diff/1280002/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_views.cc:147: // TODO(pkotwicz):
This is a hack. Remove ASAP.
On 2013/11/14 00:06:54, benwells wrote:
> Could you file a bug for this?

Done.

[benwells, 2013-11-14 02:31:29]

On 2013/11/14 01:05:24, pkotwicz wrote:
> https://codereview.chromium.org/59043013/diff/1280002/apps/shell_window.cc
> File apps/shell_window.cc (right):
> 
>
https://codereview.chromium.org/59043013/diff/1280002/apps/shell_window.cc#ne...
> apps/shell_window.cc:420: fullscreen_types_ = FULLSCREEN_TYPE_NONE;
> This is not a logical change as far as I can tell. (It better not be a logical
> change given that IsFullscreenForTabOrPending() checks |fullscreen_types_|)
> 
>
https://codereview.chromium.org/59043013/diff/1280002/chrome/browser/ui/views...
> File chrome/browser/ui/views/apps/native_app_window_views.cc (right):
> 
>
https://codereview.chromium.org/59043013/diff/1280002/chrome/browser/ui/views...
> chrome/browser/ui/views/apps/native_app_window_views.cc:147: //
TODO(pkotwicz):
> This is a hack. Remove ASAP.
> On 2013/11/14 00:06:54, benwells wrote:
> > Could you file a bug for this?
> 
> Done.

lgtm, looking forward to playing with this :)

[pkotwicz, 2013-11-14 02:45:27]

oshima@, can you please take another look I fixed a few bugs with my changes in
CustomFrameViewAsh

Almost there!

[oshima, 2013-11-14 19:27:29]

lgtm

[commit-bot: I haz the power, 2013-11-15 22:52:44]

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

[commit-bot: I haz the power, 2013-11-16 00:19:06]

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

[commit-bot: I haz the power, 2013-11-16 03:31:54]

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

[commit-bot: I haz the power, 2013-11-18 16:04:19]

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

[commit-bot: I haz the power, 2013-11-18 17:41:35]

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

[commit-bot: I haz the power, 2013-11-18 18:25:13]

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

[commit-bot: I haz the power, 2013-11-18 20:42:37]

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

[commit-bot: I haz the power, 2013-11-18 20:54:45]

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

[commit-bot: I haz the power, 2013-11-19 01:01:33]

Change committed as 235861