Map fullscreen button to maximize

New Behavior on pressing F4:
Restored -> Maximized
Maximized -> Restored
Fullscreen -> not fullscreen (what is was before)

BUG=145402
TEST=On chromeos press F4 when in Restored, Maximize, Fullscreen windows.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157147

[tfarina, 2012-09-12 14:47:50]

https://chromiumcodereview.appspot.com/10914231/diff/1/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://chromiumcodereview.appspot.com/10914231/diff/1/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.cc:27: #include
"chrome/browser/ui/views/frame/browser_view.h"
no, no, no. You can't include this anymore!

[sky, 2012-09-12 14:57:33]

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.cc (right):

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:658: if (!window)
Is it intentional that hitting this button cycles windows if none are active?

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:660: if
(wm::IsWindowFullscreen(window) && shell->delegate())
On looking at this, why do you even need this? Doens't 662 handle this?

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:665: wm::MaximizeWindow(window);
This is problematic, it assumes all windows can maximized which isn't the case.
You need to add a property indicating if a window can be maximized and honor
that.

[sschmitz, 2012-09-12 23:45:49]

Re Fullscreen: Found better way by using existing FullscreenController.

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.cc (right):

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:658: if (!window)
On 2012/09/12 14:57:33, sky wrote:
> Is it intentional that hitting this button cycles windows if none are active?
After conferring with PM and sky@, took it out.
Done.

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:660: if
(wm::IsWindowFullscreen(window) && shell->delegate())
On 2012/09/12 14:57:33, sky wrote:
> On looking at this, why do you even need this? Doens't 662 handle this?
Yes. We get out of fullscreen mode when F4 is pressed.

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:665: wm::MaximizeWindow(window);
On 2012/09/12 14:57:33, sky wrote:
> This is problematic, it assumes all windows can maximized which isn't the
case.
> You need to add a property indicating if a window can be maximized and honor
> that.
Added wm::CanMaximizeWindow(window)(thanks sadrul@!)

https://chromiumcodereview.appspot.com/10914231/diff/1/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://chromiumcodereview.appspot.com/10914231/diff/1/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.cc:27: #include
"chrome/browser/ui/views/frame/browser_view.h"
On 2012/09/12 14:47:51, tfarina wrote:
> no, no, no. You can't include this anymore!

Thanks. Found a better way.
Done

[mazda, 2012-09-13 00:26:00]

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.cc (right):

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:658: if (!window)
On 2012/09/12 23:45:50, sschmitz wrote:
> On 2012/09/12 14:57:33, sky wrote:
> > Is it intentional that hitting this button cycles windows if none are
active?
> After conferring with PM and sky@, took it out.
> Done.

This is for http://crbug.com/133337. 
If we've decided not to support restoring windows in launcher by this command,
line 639-642 should also be modified.

[sky, 2012-09-13 00:46:22]

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.cc (right):

https://chromiumcodereview.appspot.com/10914231/diff/1/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:660: if
(wm::IsWindowFullscreen(window) && shell->delegate())
On 2012/09/12 23:45:50, sschmitz wrote:
> On 2012/09/12 14:57:33, sky wrote:
> > On looking at this, why do you even need this? Doens't 662 handle this?
> Yes. We get out of fullscreen mode when F4 is pressed.

My question is, why do you need to route to the delegate at all here? Does
wm::RestoreWindow do the right thing if the window is fullscreen?

https://chromiumcodereview.appspot.com/10914231/diff/5002/ash/accelerators/ac...
File ash/accelerators/accelerator_controller.cc (right):

https://chromiumcodereview.appspot.com/10914231/diff/5002/ash/accelerators/ac...
ash/accelerators/accelerator_controller.cc:667: break;
no break.

https://chromiumcodereview.appspot.com/10914231/diff/5002/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://chromiumcodereview.appspot.com/10914231/diff/5002/ash/wm/window_util....
ash/wm/window_util.cc:69: views::Widget* widget =
views::Widget::GetWidgetForNativeWindow(window);
Instead of this, add a new property that says whether the window can be
maximized. By property I mean something like 
window->GetProperty(aura::client::kCanMaximizeKey). NativeWidgetAura would set
this appropriately.

[sschmitz, 2012-09-13 19:34:13]

Mainly: added "Can Maximize" window prop per suggestion from sky

http://codereview.chromium.org/10914231/diff/1/ash/accelerators/accelerator_c...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/10914231/diff/1/ash/accelerators/accelerator_c...
ash/accelerators/accelerator_controller.cc:658: if (!window)
On 2012/09/13 00:26:00, mazda wrote:
> On 2012/09/12 23:45:50, sschmitz wrote:
> > On 2012/09/12 14:57:33, sky wrote:
> > > Is it intentional that hitting this button cycles windows if none are
> active?
> > After conferring with PM and sky@, took it out.
> > Done.
> 
> This is for http://crbug.com/133337. 
> If we've decided not to support restoring windows in launcher by this command,
> line 639-642 should also be modified.

On further discussion PM decided to leave as is.

http://codereview.chromium.org/10914231/diff/1/ash/accelerators/accelerator_c...
ash/accelerators/accelerator_controller.cc:660: if
(wm::IsWindowFullscreen(window) && shell->delegate())
On 2012/09/12 14:57:33, sky wrote:
> On looking at this, why do you even need this? Doens't 662 handle this?
Two Reasons for this:
1. PM changed behavior when in Fullscreen, we will got to "not fullscreen" but
leave the window maximized or restored as it was before fullscreen.
2. 662 did not cleanup. The FullscreenExitBubble hung around for a few secs
(when you pressed F4 immediately after going to fullscreen)

http://codereview.chromium.org/10914231/diff/5002/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/10914231/diff/5002/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:667: break;
On 2012/09/13 00:46:22, sky wrote:
> no break.

Done.

http://codereview.chromium.org/10914231/diff/5002/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

http://codereview.chromium.org/10914231/diff/5002/ash/wm/window_util.cc#newco...
ash/wm/window_util.cc:69: views::Widget* widget =
views::Widget::GetWidgetForNativeWindow(window);
On 2012/09/13 00:46:22, sky wrote:
> Instead of this, add a new property that says whether the window can be
> maximized. By property I mean something like 
> window->GetProperty(aura::client::kCanMaximizeKey). NativeWidgetAura would set
> this appropriately.
Thanks for your help on this.
Done.

[sky, 2012-09-13 19:36:33]

http://codereview.chromium.org/10914231/diff/1/ash/accelerators/accelerator_c...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/10914231/diff/1/ash/accelerators/accelerator_c...
ash/accelerators/accelerator_controller.cc:660: if
(wm::IsWindowFullscreen(window) && shell->delegate())
On 2012/09/13 19:34:13, sschmitz wrote:
> On 2012/09/12 14:57:33, sky wrote:
> > On looking at this, why do you even need this? Doens't 662 handle this?
> Two Reasons for this:
> 1. PM changed behavior when in Fullscreen, we will got to "not fullscreen" but
> leave the window maximized or restored as it was before fullscreen.
> 2. 662 did not cleanup. The FullscreenExitBubble hung around for a few secs
> (when you pressed F4 immediately after going to fullscreen)
> 

If that's the case, then it probably means BrowserFrameAura is not cleaning up
when the window state changes. BrowserFrameAura should listen for when the
window goes out of fullscreen mode and hide the bubble.

[sschmitz, 2012-09-14 19:43:50]

Much simpler; restored chrome's F4 shortcut; Ash F4 handles max/restore when not
in fullscreen; Chrome F4 handles getting out of fullscreen when in fullscreen.

[sky, 2012-09-14 22:48:04]

http://codereview.chromium.org/10914231/diff/7025/ash/accelerators/accelerato...
File ash/accelerators/accelerator_table.cc (right):

http://codereview.chromium.org/10914231/diff/7025/ash/accelerators/accelerato...
ash/accelerators/accelerator_table.cc:175: WINDOW_MAXIMIZE_RESTORE, // F4
Can we call this TOGGLE_MAXIMIZED everywhere?

http://codereview.chromium.org/10914231/diff/7025/chrome/browser/ui/views/acc...
File chrome/browser/ui/views/accelerator_table_unittest.cc (right):

http://codereview.chromium.org/10914231/diff/7025/chrome/browser/ui/views/acc...
chrome/browser/ui/views/accelerator_table_unittest.cc:48:
exception_entry.keycode = ui::VKEY_F4; // F4 has Ash and Chrome shortcuts
Isn't your ifdef chromeos specific?

[sschmitz, 2012-09-14 23:44:56]

http://codereview.chromium.org/10914231/diff/7025/ash/accelerators/accelerato...
File ash/accelerators/accelerator_table.cc (right):

http://codereview.chromium.org/10914231/diff/7025/ash/accelerators/accelerato...
ash/accelerators/accelerator_table.cc:175: WINDOW_MAXIMIZE_RESTORE, // F4
On 2012/09/14 22:48:05, sky wrote:
> Can we call this TOGGLE_MAXIMIZED everywhere?

Done.

http://codereview.chromium.org/10914231/diff/7025/chrome/browser/ui/views/acc...
File chrome/browser/ui/views/accelerator_table_unittest.cc (right):

http://codereview.chromium.org/10914231/diff/7025/chrome/browser/ui/views/acc...
chrome/browser/ui/views/accelerator_table_unittest.cc:48:
exception_entry.keycode = ui::VKEY_F4; // F4 has Ash and Chrome shortcuts
On 2012/09/14 22:48:05, sky wrote:
> Isn't your ifdef chromeos specific?
Yes. Added #if defined(OS_CHROMEOS) around exception entry.
Done.

[sky, 2012-09-16 16:08:03]

LGTM

http://codereview.chromium.org/10914231/diff/13009/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/10914231/diff/13009/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:662: // fullscreen. If we got here
via VKEY_F4, chrome's action will
Change that last sentence to 'Return false so Chrome processes F4 and exits
fullscreen.

http://codereview.chromium.org/10914231/diff/13009/chrome/browser/ui/views/ac...
File chrome/browser/ui/views/accelerator_table_unittest.cc (right):

http://codereview.chromium.org/10914231/diff/13009/chrome/browser/ui/views/ac...
chrome/browser/ui/views/accelerator_table_unittest.cc:49:
exception_entry.keycode = ui::VKEY_F4; // F4 has Ash and Chrome shortcuts
This comment is better placed above this line:
// Both chrome and Ash have a shortcut for F4.

[sschmitz, 2012-09-17 15:22:04]

improved comments and rebased

http://codereview.chromium.org/10914231/diff/13009/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/10914231/diff/13009/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:662: // fullscreen. If we got here
via VKEY_F4, chrome's action will
On 2012/09/16 16:08:03, sky wrote:
> Change that last sentence to 'Return false so Chrome processes F4 and exits
> fullscreen.

Done.

http://codereview.chromium.org/10914231/diff/13009/chrome/browser/ui/views/ac...
File chrome/browser/ui/views/accelerator_table_unittest.cc (right):

http://codereview.chromium.org/10914231/diff/13009/chrome/browser/ui/views/ac...
chrome/browser/ui/views/accelerator_table_unittest.cc:49:
exception_entry.keycode = ui::VKEY_F4; // F4 has Ash and Chrome shortcuts
On 2012/09/16 16:08:03, sky wrote:
> This comment is better placed above this line:
> // Both chrome and Ash have a shortcut for F4.

Done.

[commit-bot: I haz the power, 2012-09-17 15:22:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/10914231/12060

[commit-bot: I haz the power, 2012-09-17 17:41:05]

Change committed as 157147