Remove TOGGLE_MAXIMIZED_* from Ash reserved actions list.

Remove TOGGLE_MAXIMIZED_* from Ash reserved actions list.

The following changes are for removing TOGGLE_MAXIMIZED_* from Ash reserved actions list.
- Add ShellDelegate::ToggleMaximized
- Remove VKEY_F4 from browser accelerators list

Also added a browser test for ChromeShellDelegate::ToggleMaximized.

BUG=152265
TEST=Manual

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

[mazda, 2012-10-06 03:58:05]

Sato-san,

I changed the order of processing accelerators in
FocusManager::ProcessAccelerator so that FocusManagerDelegate processes
accelerators first. Otherwise F4 key is always processed as browser accelerator
and ash::AcceleratorController cannot handle it even in non-fullscreen mode.
I believe this change is safe because only F4 key is processed by both handlers,
and I confirmed that it works properly. 
But if there is any concerns, please let me know.

Thanks,

[Yusuke Sato, 2012-10-08 17:13:23]

LGTM with some requests:

1) Please double check that Ash is the only user of the FocusManagerDelegate
interface.
2) Just in case, please also manually check that your CL works fine with
reserved/non-reserved Ash/Browser accelerators when windowed-app is/isn't
focused (8 cases in total).
3) This CL itself LGTM, but I'm still wondering if it's possible to remove F4
from the Browser accelerator table. If you think it's better, please add a TODO
to Browser's accelerator_table.cc.

-Yusuke

On 2012/10/06 03:58:05, mazda wrote:
> Sato-san,
> 
> I changed the order of processing accelerators in
> FocusManager::ProcessAccelerator so that FocusManagerDelegate processes
> accelerators first. Otherwise F4 key is always processed as browser
accelerator
> and ash::AcceleratorController cannot handle it even in non-fullscreen mode.
> I believe this change is safe because only F4 key is processed by both
handlers,
> and I confirmed that it works properly. 
> But if there is any concerns, please let me know.
> 
> Thanks,

[Yusuke Sato, 2012-10-08 20:16:17]

Please also try to remove F4 from the list of Browser's reserved accelerators.
That might also fix the original issue, 152265, and it's simpler than modifying
views::FocusManager.

On 2012/10/08 17:13:23, Yusuke Sato wrote:
> LGTM with some requests:
> 
> 1) Please double check that Ash is the only user of the FocusManagerDelegate
> interface.
> 2) Just in case, please also manually check that your CL works fine with
> reserved/non-reserved Ash/Browser accelerators when windowed-app is/isn't
> focused (8 cases in total).
> 3) This CL itself LGTM, but I'm still wondering if it's possible to remove F4
> from the Browser accelerator table. If you think it's better, please add a
TODO
> to Browser's accelerator_table.cc.
> 
> -Yusuke
> 
> On 2012/10/06 03:58:05, mazda wrote:
> > Sato-san,
> > 
> > I changed the order of processing accelerators in
> > FocusManager::ProcessAccelerator so that FocusManagerDelegate processes
> > accelerators first. Otherwise F4 key is always processed as browser
> accelerator
> > and ash::AcceleratorController cannot handle it even in non-fullscreen mode.
> > I believe this change is safe because only F4 key is processed by both
> handlers,
> > and I confirmed that it works properly. 
> > But if there is any concerns, please let me know.
> > 
> > Thanks,

[mazda, 2012-10-08 22:55:33]

I decided to remove F4 from browesr command now.
Please take a look at the change again.

> 1) Please double check that Ash is the only user of the FocusManagerDelegate
> interface.
I reverted the change of FocusManager.

> 2) Just in case, please also manually check that your CL works fine with
> reserved/non-reserved Ash/Browser accelerators when windowed-app is/isn't
> focused (8 cases in total).
I checked that the accelerator worked as before in all cases.

> 3) This CL itself LGTM, but I'm still wondering if it's possible to remove F4
> from the Browser accelerator table. If you think it's better, please add a
TODO
> to Browser's accelerator_table.cc.
Did it now.

> Please also try to remove F4 from the list of Browser's reserved accelerators.
> That might also fix the original issue, 152265, and it's simpler than
modifying
> views::FocusManager.
I tried removing F4 from the list of Browser's reserved accelerator, but it did
not work as expected without modifying views::FocusManager. Regardless of the
timing of F4 key handled (before or after renderer), F4 key is handled as
browser command in FocusManager::ProcessAccelerator anyway.
In this change, I removed F4 key from browser accelerators, so whether it is
browser's reserved accelerators or not does not matter anymore. I'll remove it
from reserved accelerators if necessary after further code inspection.

[Yusuke Sato, 2012-10-08 23:02:53]

On 2012/10/08 22:55:33, mazda wrote:
> I decided to remove F4 from browesr command now.
> Please take a look at the change again.
> 
> > 1) Please double check that Ash is the only user of the FocusManagerDelegate
> > interface.
> I reverted the change of FocusManager.
> 
> > 2) Just in case, please also manually check that your CL works fine with
> > reserved/non-reserved Ash/Browser accelerators when windowed-app is/isn't
> > focused (8 cases in total).
> I checked that the accelerator worked as before in all cases.
> 
> > 3) This CL itself LGTM, but I'm still wondering if it's possible to remove
F4
> > from the Browser accelerator table. If you think it's better, please add a
> TODO
> > to Browser's accelerator_table.cc.
> Did it now.

Nice! LGTM. Thanks for adding the test.


> 
> > Please also try to remove F4 from the list of Browser's reserved
accelerators.
> > That might also fix the original issue, 152265, and it's simpler than
> modifying
> > views::FocusManager.
> I tried removing F4 from the list of Browser's reserved accelerator, but it
did
> not work as expected without modifying views::FocusManager. Regardless of the
> timing of F4 key handled (before or after renderer), F4 key is handled as
> browser command in FocusManager::ProcessAccelerator anyway.
> In this change, I removed F4 key from browser accelerators, so whether it is
> browser's reserved accelerators or not does not matter anymore. I'll remove it
> from reserved accelerators if necessary after further code inspection.

[mazda, 2012-10-08 23:28:31]

+sky
Could you do an OWNERS review?

[sky, 2012-10-09 00:16:57]

Why is the current approach bad?

[mazda, 2012-10-09 00:40:14]

On 2012/10/09 00:16:57, sky wrote:
> Why is the current approach bad?

We'd like to avoid using kReservedActions as possible because that causes
problems for Apps v2 app. Please see the following link for more details.
https://docs.google.com/a/google.com/presentation/d/1pKVLusS4NAJFHknd5HVWZ79J...

Current approach does work when  the app is in fullscreen, but I'd like to avoid
the special treatment.

[Yusuke Sato, 2012-10-09 00:46:21]

On 2012/10/09 00:40:14, mazda wrote:
> On 2012/10/09 00:16:57, sky wrote:
> > Why is the current approach bad?
> 
> We'd like to avoid using kReservedActions as possible because that causes
> problems for Apps v2 app.

More specifically, the current approach does not allow v2 apps to receive a F4
keypress event, which could be a problem for Chromoting, Secure Shell and other
RDP-like apps.

[sky, 2012-10-09 15:50:53]

Thanks for the clarification as to why we need this.

http://codereview.chromium.org/11082002/diff/3007/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/11082002/diff/3007/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_shell_delegate.cc:194: if
(ash::wm::IsWindowMaximized(window))
Promote this to a common place so TestShellDelegate and this can share it.

http://codereview.chromium.org/11082002/diff/3007/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_shell_delegate_browsertest.cc (right):

http://codereview.chromium.org/11082002/diff/3007/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_shell_delegate_browsertest.cc:8: #include
"chrome/browser/ui/ash/chrome_shell_delegate.h"
This should be the first include.

[mazda, 2012-10-09 20:50:33]

Thank you for the review.
Please take another look.

http://codereview.chromium.org/11082002/diff/3007/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/11082002/diff/3007/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_shell_delegate.cc:194: if
(ash::wm::IsWindowMaximized(window))
On 2012/10/09 15:50:53, sky wrote:
> Promote this to a common place so TestShellDelegate and this can share it.

I added ToggleMaximizedWindow to ash/wm/window_util.h and moved this part to its
implementation. Does this sound good?

http://codereview.chromium.org/11082002/diff/3007/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_shell_delegate_browsertest.cc (right):

http://codereview.chromium.org/11082002/diff/3007/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_shell_delegate_browsertest.cc:8: #include
"chrome/browser/ui/ash/chrome_shell_delegate.h"
On 2012/10/09 15:50:53, sky wrote:
> This should be the first include.

Done.

[sky, 2012-10-09 22:41:01]

Yes, thanks. LGTM

[commit-bot: I haz the power, 2012-10-09 23:01:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/11082002/6016

[commit-bot: I haz the power, 2012-10-09 23:56:46]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-10-10 00:01:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/11082002/6016

[commit-bot: I haz the power, 2012-10-10 02:34:11]

Change committed as 161015