Handle more browser commands in ash.

Handle more browser commands in ash.

This CL modifies the shell accelerator controller to handle the shortcuts for
the following browser commands so that they work with no browser window shown.
- IDC_NEW_TAB
- IDC_RESTORE_TAB
- IDC_SHOW_BOOKMARK_MANAGER

BUG=124232, 120642
TEST=Checked all the shortcuts worked properly with no browser window shown. aura_shell_unittests

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

[Ben Goodger (Google), 2012-05-23 00:14:13]

I don't think we should do most of this CL.

The only things you should be able to get at from the shell are:

- task manager (which should just be a v2 app)
- undo close last window
- maybe new tab

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

http://codereview.chromium.org/10414064/diff/1/ash/accelerators/accelerator_c...
ash/accelerators/accelerator_controller.cc:97: bool HandleOpenBookmarkManager()
{
ash shouldn't know anything about browser commands like this

http://codereview.chromium.org/10414064/diff/1/ash/accelerators/accelerator_c...
ash/accelerators/accelerator_controller.cc:102: bool
HandleOpenClearBrowsingData() {
or this

http://codereview.chromium.org/10414064/diff/1/ash/accelerators/accelerator_c...
ash/accelerators/accelerator_controller.cc:107: bool HandleOpenDownloads() {
or this

http://codereview.chromium.org/10414064/diff/1/ash/accelerators/accelerator_c...
ash/accelerators/accelerator_controller.cc:112: bool HandleOpenHelpPage() {
or this

[mazda, 2012-05-23 01:32:47]

On 2012/05/23 00:14:13, Ben Goodger (Google) wrote:
> I don't think we should do most of this CL.
> 
> The only things you should be able to get at from the shell are:
> 
> - task manager (which should just be a v2 app)
> - undo close last window
> - maybe new tab

Thank you for the comments.

Does that mean it's unnecessary for these commands to work when no window is
shown? Or, should I find another way to make the commands work when no window is
shown? 

Thanks,

[Ben Goodger (Google), 2012-05-23 02:03:11]

Right. We wouldn't necessarily promote this functionality for other apps
either.

-Ben

On Wed, May 23, 2012 at 1:32 AM, <mazda@chromium.org> wrote:

> On 2012/05/23 00:14:13, Ben Goodger (Google) wrote:
>
>> I don't think we should do most of this CL.
>>
>
>  The only things you should be able to get at from the shell are:
>>
>
>  - task manager (which should just be a v2 app)
>> - undo close last window
>> - maybe new tab
>>
>
> Thank you for the comments.
>
> Does that mean it's unnecessary for these commands to work when no window
> is
> shown? Or, should I find another way to make the commands work when no
> window is
> shown?
>
> Thanks,
>
>
http://codereview.chromium.**org/10414064/<http://codereview.chromium.org/104...
>

[mazda, 2012-05-23 22:11:24]

Thank you for the suggestions.

I removed the handlers except for IDC_NEW_TAB, IDC_RESTORE_TAB, and
IDC_SHOW_BOOKMARK_MANAGER.
It also modified ChromeShellDelegate::RestoreTab because I noticed that
Browser::RestoreTab doesn't work if TabRestoreService has not loaded the
entries.

Please take another look at the change.

[Ben Goodger (Google), 2012-05-24 22:21:28]

LGTM

[commit-bot: I haz the power, 2012-05-24 22:35:57]

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

[commit-bot: I haz the power, 2012-05-24 22:36:08]

Failed to apply patch for ash/shell_delegate.h:
While running patch -p1 --forward --force;
patching file ash/shell_delegate.h
Hunk #1 succeeded at 59 (offset -4 lines).
Hunk #2 FAILED at 81.
1 out of 2 hunks FAILED -- saving rejects to file ash/shell_delegate.h.rej

[mazda, 2012-05-24 23:10:34]

+sky
Could you do an OWNERS review for chrome/browser/sessions?

[sky, 2012-05-24 23:37:10]

LGTM

[commit-bot: I haz the power, 2012-05-24 23:47:14]

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

[commit-bot: I haz the power, 2012-05-25 00:08:14]

Try job failure for 10414064-21001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-05-25 00:25:53]

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

[commit-bot: I haz the power, 2012-05-25 03:17:00]

Change committed as 138982