fullscreen in apps v2 ShellWindow via app.window.create.

fullscreen in apps v2 ShellWindow via app.window.create.

This allows packaged applications to launch windows
already into a fullscreen, maximized, or minimized state.

Design doc: https://docs.google.com/a/google.com/document/d/1__SPIxd_sFVYFNdP55CouE-p61uLOWEL-X-2TEOJFdY/edit

BUG=163626
TEST=Manual testing is appropriate as this functionality is tightly coupled with operating system functionality and related automated tests have always been disabled flaky. Please use the window-state sample: https://github.com/GoogleChrome/chrome-app-samples/tree/master/window-state.

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

[jeremya, 2013-04-05 20:00:34]

PS, your CL description says "applicatinos", which I guess are like little tiny
applications that you eat for breakfast?

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:184: }
default: NOTREACHED()?

Are these all okay to do before the window is shown?

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:510: } else if
(!fullscreen_for_window_api_) {
fdsdsffasafdsf.

There must be a better way :(

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.h (right):

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.h:300: bool fullscreen_for_tab_;
oh boy :<

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/gtk/extensi...
File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right):

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/gtk/extensi...
chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:160: return state_ &
GDK_WINDOW_STATE_FULLSCREEN;
nit: the functions above have parens around the &, either change them or add
parens here.

https://codereview.chromium.org/13609003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/app_current_window_internal.idl (right):

https://codereview.chromium.org/13609003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/app_current_window_internal.idl:22: static void
fullscreen();
I've forgotten why we decided that having an apps-specific fullscreen API was
necessary. Something to do with the webkit, err, blink API not being able to
specify fullscreen on launch?

https://codereview.chromium.org/13609003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/13609003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/app_window.idl:125: // Restore the window, exiting
a maximized, minimized, or fullscreen state.
maximize() -> fullscreen() -> restore() -> state == 'normal'?

[scheib, 2013-04-05 21:47:52]

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:184: }
On 2013/04/05 20:00:35, jeremya wrote:
> default: NOTREACHED()?

Google code style suggests NOT having a default if you intend to handle all
enumerations. It is a compiler error to not handle all enum values, so when a
new one is introduced you have a compile error until you fix all switch use
locations.

> 
> Are these all okay to do before the window is shown?

Empirically, yes.

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:510: } else if
(!fullscreen_for_window_api_) {
On 2013/04/05 20:00:35, jeremya wrote:
> fdsdsffasafdsf.
> 
> There must be a better way :(

I haven't thought of it. We can fold these two variables into an enum with 4
states, they're all valid:

enum {
  NOT_FULLSCREEN,
  FULLSCREEN_TAB,
  FULLSCREEN_WINDOW,
  FULLSCREEN_TAB_AND_WINDOW
}

but I don't see what that gets us here. (In fullscreen_controller, I think it is
a win, because there are many other complicating states).

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.h (right):

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.h:300: bool fullscreen_for_tab_;
On 2013/04/05 20:00:35, jeremya wrote:
> oh boy :<

Done. o|-<

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/gtk/extensi...
File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right):

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/gtk/extensi...
chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:160: return state_ &
GDK_WINDOW_STATE_FULLSCREEN;
On 2013/04/05 20:00:35, jeremya wrote:
> nit: the functions above have parens around the &, either change them or add
> parens here.

Done.

https://codereview.chromium.org/13609003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/app_current_window_internal.idl (right):

https://codereview.chromium.org/13609003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/app_current_window_internal.idl:22: static void
fullscreen();
On 2013/04/05 20:00:35, jeremya wrote:
> I've forgotten why we decided that having an apps-specific fullscreen API was
> necessary. Something to do with the webkit, err, blink API not being able to
> specify fullscreen on launch?

https://code.google.com/p/chromium/issues/detail?id=163626
Yes:
- Fullscreen on create
- Fullscreen at the application's control without limitations such as HTML5's
required user gesture.
- Fullscreen for the app window without confusing web content that doesn't
expect to be in a fullscreen state unless it caused its self to go fullscreen.

https://codereview.chromium.org/13609003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/13609003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/app_window.idl:125: // Restore the window, exiting
a maximized, minimized, or fullscreen state.
On 2013/04/05 20:00:35, jeremya wrote:
> maximize() -> fullscreen() -> restore() -> state == 'normal'?

Out of the scope of this change, just as maximize() -> minimize() -> restore()
-> state == ?? is out of this scope and currently tied to whatever each window
manager decides.

[jeremya, 2013-04-08 17:13:54]

I'm pushing back a little on this to make sure we make the right decision here,
since if we add this API we'll have to support two fullscreen modes forever and
that makes me sad. The change technically LGTM, but even better would be to
expand the capabilities of the web API.

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:184: }
On 2013/04/05 21:47:52, scheib wrote:
> On 2013/04/05 20:00:35, jeremya wrote:
> > default: NOTREACHED()?
> 
> Google code style suggests NOT having a default if you intend to handle all
> enumerations. It is a compiler error to not handle all enum values, so when a
> new one is introduced you have a compile error until you fix all switch use
> locations.

I hope nobody ever upcasts an int into that enum, then :/

https://codereview.chromium.org/13609003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:510: } else if
(!fullscreen_for_window_api_) {
On 2013/04/05 21:47:52, scheib wrote:
> On 2013/04/05 20:00:35, jeremya wrote:
> > fdsdsffasafdsf.
> > 
> > There must be a better way :(
> 
> I haven't thought of it. We can fold these two variables into an enum with 4
> states, they're all valid:
> 
> enum {
>   NOT_FULLSCREEN,
>   FULLSCREEN_TAB,
>   FULLSCREEN_WINDOW,
>   FULLSCREEN_TAB_AND_WINDOW
> }
> 
> but I don't see what that gets us here. (In fullscreen_controller, I think it
is
> a win, because there are many other complicating states).

I guess there's no way to combine it all into one bool, possibly changing
behaviour? Perhaps we'd be better off hacking blink to allow us to create pages
in fullscreen mode, rather than adding Yet Another Fullscreen Mode.

https://codereview.chromium.org/13609003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/app_current_window_internal.idl (right):

https://codereview.chromium.org/13609003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/app_current_window_internal.idl:22: static void
fullscreen();
On 2013/04/05 21:47:52, scheib wrote:
> On 2013/04/05 20:00:35, jeremya wrote:
> > I've forgotten why we decided that having an apps-specific fullscreen API
was
> > necessary. Something to do with the webkit, err, blink API not being able to
> > specify fullscreen on launch?
> 
> https://code.google.com/p/chromium/issues/detail?id=163626
> Yes:
> - Fullscreen on create
> - Fullscreen at the application's control without limitations such as HTML5's
> required user gesture.
> - Fullscreen for the app window without confusing web content that doesn't
> expect to be in a fullscreen state unless it caused its self to go fullscreen.

#1 and #2 could be fixed with a judicious blink change, I think. Could you
expand on #3? It seems like the web API should be able to deal with that case...
possibly with the application of <iframe>s.

In general, if we can stick to using web APIs in apps, we should do that as much
as possible.

[scheib, 2013-04-08 21:26:41]

This was discussed with jeremya, asargent, and erikkay, and we agreed that the
app.window API should be separate from the HTML fullscreen api, as covered in
the design doc. http://goo.gl/xfpuK

[commit-bot: I haz the power, 2013-04-08 21:29:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/13609003/11001

[commit-bot: I haz the power, 2013-04-08 23:36:31]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-04-08 23:46:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/13609003/11001

[commit-bot: I haz the power, 2013-04-08 23:58:58]

Change committed as 192952