[HTerm-Crosh] Add key shortcut for opening Crosh on ChromeOS Aura.

[HTerm-Crosh] Add key shortcut for opening Crosh on ChromeOS Aura.

Make Ctrl+Alt+T open Crosh in HTML Terminal on Aura build.
If HTerm is not installed nothing should happen..

BUG=105649
TEST= manual on Stumpy VM with HTerm extension installed.


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

[tbarzic, 2012-01-27 23:16:50]

Hey guys, can you take a look at this?

Ben -> browser/ui part
Jon -> browser/extensions part

Thanks!

https://chromiumcodereview.appspot.com/9297008/diff/6003/chrome/browser/exten...
File chrome/browser/extensions/api/terminal/terminal_extension_helper.cc
(right):

https://chromiumcodereview.appspot.com/9297008/diff/6003/chrome/browser/exten...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:25: const
std::string& extension_id) {
This method is moved from terminal_private_api.cc (no change in functionality)..

[Ben Goodger (Google), 2012-01-27 23:28:08]

In the vein of putting as much shell stuff into ash as possible, can we put the
keyboard shortcut for this in ash, and augment the ShellDelegate with a function
to open a URL in a new tab.

[tbarzic, 2012-01-30 17:53:55]

Ben, PTAL

[Ben Goodger (Google), 2012-01-30 19:06:15]

https://chromiumcodereview.appspot.com/9297008/diff/8001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://chromiumcodereview.appspot.com/9297008/diff/8001/ash/shell_delegate.h...
ash/shell_delegate.h:86: virtual void OpenNewTab(NewTabContentType type) = 0;
This function should take a const GURL& instead of a new enum type.

You will then also have to create a shell_urls.h/cc file in ash/ that contains
the URL of the terminal. You will want to create a pretty URL for it instead of
just loading it via an extension protocol.

Look at src/chrome/common/url_constants.cc/h for inspiration. 

The pretty URL would be something like this: chrome://terminal

Then you would have to add handling to the browser to resolve this pretty URL to
an extension. There is already code that does this for chrome://bookmarks (since
the bookmarks manager page is implemented as an extension). Look at extension.cc
and kChromeUIBookmarksHost.

[tbarzic, 2012-01-30 22:57:18]

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/ui/v...
File chrome/browser/ui/views/aura/chrome_shell_delegate.cc (right):

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/ui/v...
chrome/browser/ui/views/aura/chrome_shell_delegate.cc:124: new_tab_url =
have to fix spacing here..

[tbarzic, 2012-01-30 23:14:02]

On 2012/01/30 19:06:15, Ben Goodger (Google) wrote:
> https://chromiumcodereview.appspot.com/9297008/diff/8001/ash/shell_delegate.h
> File ash/shell_delegate.h (right):
> 
>
https://chromiumcodereview.appspot.com/9297008/diff/8001/ash/shell_delegate.h...
> ash/shell_delegate.h:86: virtual void OpenNewTab(NewTabContentType type) = 0;
> This function should take a const GURL& instead of a new enum type.
> 
> You will then also have to create a shell_urls.h/cc file in ash/ that contains
> the URL of the terminal. You will want to create a pretty URL for it instead
of
> just loading it via an extension protocol.
> 
> Look at src/chrome/common/url_constants.cc/h for inspiration. 
> 
> The pretty URL would be something like this: chrome://terminal
> 
> Then you would have to add handling to the browser to resolve this pretty URL
to
> an extension. There is already code that does this for chrome://bookmarks
(since
> the bookmarks manager page is implemented as an extension). Look at
extension.cc
> and kChromeUIBookmarksHost.

I don't think we can handle terminal the same way as bookmarks..
The bookmark_manager extension registers chrome_url_override for
chrome://bookmarks in its manifest file. Unfortunately, it looks like the list
of urls we can override this way is restricted, and adding chrome://terminal
override in terminal extension manifest file would brake the extension on M17
(even if we whitelist it on M18), which we'd like to avoid..

[jstritar, 2012-01-30 23:25:52]

extensions LGTM with the comments below

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
File chrome/browser/extensions/api/terminal/terminal_extension_helper.cc
(right):

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:13: // Keep
officail app first, so GetTerminalExtensionID checks it first.
officail --> official

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:25: const
std::string& extension_id) {
nit: both args on the same line, or line them up vertically

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:57: return
kAllowedExtensionIds[i];
will both extensions ever be installed?

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
File chrome/browser/extensions/api/terminal/terminal_extension_helper.h (right):

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
chrome/browser/extensions/api/terminal/terminal_extension_helper.h:17: static
bool AllowAccessToExtension(Profile* profile,
nit: add brief comments for these methods?

[tbarzic, 2012-01-30 23:38:13]

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
File chrome/browser/extensions/api/terminal/terminal_extension_helper.cc
(right):

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:13: // Keep
officail app first, so GetTerminalExtensionID checks it first.
On 2012/01/30 23:25:52, jstritar wrote:
> officail --> official

Done.

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:25: const
std::string& extension_id) {
On 2012/01/30 23:25:52, jstritar wrote:
> nit: both args on the same line, or line them up vertically

Done.

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:57: return
kAllowedExtensionIds[i];
On 2012/01/30 23:25:52, jstritar wrote:
> will both extensions ever be installed?

They might be, but in that case, we want to open the one from the gallery (the
other one should be only used for testing)..

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
File chrome/browser/extensions/api/terminal/terminal_extension_helper.h (right):

https://chromiumcodereview.appspot.com/9297008/diff/10002/chrome/browser/exte...
chrome/browser/extensions/api/terminal/terminal_extension_helper.h:17: static
bool AllowAccessToExtension(Profile* profile,
On 2012/01/30 23:25:52, jstritar wrote:
> nit: add brief comments for these methods?

Done.

[Ben Goodger (Google), 2012-01-31 19:20:06]

OK, I talked to zel about the various configurations. Given the short term
support matrix, I think it makes sense to go with your earlier patchset that
just adds this into the browser (i.e. leave ash alone). We can detangle this
once we get rid of pre-Aura configs.

[tbarzic, 2012-01-31 20:14:08]

OK, non extension part of the newest CL is the same as the patch 5..

[Ben Goodger (Google), 2012-01-31 22:50:33]

LGTM for patchset 5.

[commit-bot: I haz the power, 2012-02-01 01:09:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/9297008/20003

[commit-bot: I haz the power, 2012-02-01 03:20:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/9297008/20003

[commit-bot: I haz the power, 2012-02-01 04:57:58]

Change committed as 120034