Add idl for managing focus in app_shell

Add idl for managing window visibility in app_shell

BUG=424650
Committed: https://crrev.com/a713479270ad0ed2b4827eda0268eb9d265a85de
Cr-Commit-Position: refs/heads/master@{#302338}

[Zachary Kuznia, 2014-10-28 21:26:12]

zork@chromium.org changed reviewers:
+ jamescook@chromium.org, kaznacheev@chromium.org

[James Cook, 2014-10-28 22:34:49]

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
File extensions/shell/common/api/_api_features.json (right):

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/_api_features.json:15: "shell.focus": {
I wouldn't call this focus. It's more about window activation. I suspect we may
need other app_shell window management functions so I would call this
shell.window or similar.

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
File extensions/shell/common/api/shell_focus.idl (right):

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/shell_focus.idl:9: DOMString appId;
I would allow showing an app by either ID or name. During development it's a
pain to make an unpacked extension always have the same ID.

That implies this should be an optional field.

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/shell_focus.idl:10: };
What about an animation type?

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/shell_focus.idl:15: static void showApp(ShowInfo
showInfo);
Do you want the master app to be able to forcibly hide another app?

Also, I doubt we want to enforce in the code that only the master app can show
other apps. That seems like a policy decision or convention in the suite of
apps, not something the C++ code should enforce. I would kill that part of the
comment.

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/shell_focus.idl:18: static void requestShow();
Why do you want individual apps to be able to do this?  Can we get away with the
master app controlling everything?

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/shell_focus.idl:25: static void onShow();
Document who is listening to these events. "Invoked when the current app is
shown" or similar.

[Zachary Kuznia, 2014-10-28 23:04:02]

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
File extensions/shell/common/api/_api_features.json (right):

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/_api_features.json:15: "shell.focus": {
On 2014/10/28 22:34:49, James Cook wrote:
> I wouldn't call this focus. It's more about window activation. I suspect we
may
> need other app_shell window management functions so I would call this
> shell.window or similar.

Done.

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
File extensions/shell/common/api/shell_focus.idl (right):

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/shell_focus.idl:9: DOMString appId;
On 2014/10/28 22:34:49, James Cook wrote:
> I would allow showing an app by either ID or name. During development it's a
> pain to make an unpacked extension always have the same ID.
> 
> That implies this should be an optional field.

Done.

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/shell_focus.idl:10: };
On 2014/10/28 22:34:49, James Cook wrote:
> What about an animation type?

Added hide and show animation types.

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/shell_focus.idl:15: static void showApp(ShowInfo
showInfo);
On 2014/10/28 22:34:49, James Cook wrote:
> Do you want the master app to be able to forcibly hide another app?
> 
> Also, I doubt we want to enforce in the code that only the master app can show
> other apps. That seems like a policy decision or convention in the suite of
> apps, not something the C++ code should enforce. I would kill that part of the
> comment.

We can make this enforced via a permission, or just leave it as convention. 
TBD.

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/shell_focus.idl:18: static void requestShow();
On 2014/10/28 22:34:49, James Cook wrote:
> Why do you want individual apps to be able to do this?  Can we get away with
the
> master app controlling everything?

Added the listener for the master app to clarify this.

https://codereview.chromium.org/686983002/diff/1/extensions/shell/common/api/...
extensions/shell/common/api/shell_focus.idl:25: static void onShow();
On 2014/10/28 22:34:49, James Cook wrote:
> Document who is listening to these events. "Invoked when the current app is
> shown" or similar.

Added comments.

[James Cook, 2014-10-28 23:17:17]

LGTM with nits

https://codereview.chromium.org/686983002/diff/20001/extensions/shell/common/...
File extensions/shell/common/api/shell_window.idl (right):

https://codereview.chromium.org/686983002/diff/20001/extensions/shell/common/...
extensions/shell/common/api/shell_window.idl:30: // Request the current app be
brought to the foreground.
Comment that the master app will receive this request.

https://codereview.chromium.org/686983002/diff/20001/extensions/shell/common/...
extensions/shell/common/api/shell_window.idl:33: // Request the current app be
hidden.
ditto

[James Cook, 2014-10-28 23:17:45]

Oh, you should change the issue title/description since it isn't "focus"
anymore.

[Zachary Kuznia, 2014-10-30 00:09:19]

I've updated the CL to include stub implementations of the functions, and a
unittest stub.  Could you take another look?

Thanks,
-Zach

https://codereview.chromium.org/686983002/diff/20001/extensions/shell/common/...
File extensions/shell/common/api/shell_window.idl (right):

https://codereview.chromium.org/686983002/diff/20001/extensions/shell/common/...
extensions/shell/common/api/shell_window.idl:30: // Request the current app be
brought to the foreground.
On 2014/10/28 23:17:17, James Cook wrote:
> Comment that the master app will receive this request.

Done.

https://codereview.chromium.org/686983002/diff/20001/extensions/shell/common/...
extensions/shell/common/api/shell_window.idl:33: // Request the current app be
hidden.
On 2014/10/28 23:17:17, James Cook wrote:
> ditto

Done.

[James Cook, 2014-10-30 16:17:43]

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/app_she...
File extensions/shell/app_shell.gyp (right):

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/app_she...
extensions/shell/app_shell.gyp:54: 'browser/api/window/window_api.cc',
The naming convention for a foo.bar api is foo_bar/foo_bar_api.{h|cc} so I would
make this shell_window/shell_window_api.{h|cc}

Identity is different because it's chrome.identity not chrome.shell.identity.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/app_she...
extensions/shell/app_shell.gyp:240: 'browser/api/window/window_api_unittest.cc',
Hooray for unit tests in the first commit!

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/api/window/window_api.h (right):

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api.h:12: namespace shell {
I'm on the fence about whether we need this namespace. I used it for identity
because otherwise we end up with identically named classes in identical
namespaces in chrome vs. app_shell. None of the rest of the app_shell code uses
namespace shell.

WDYT?

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api.h:22: virtual
~WindowRequestHideFunction();
nit: ~WindowRequestHideFunction() override;

And below.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api.h:25: virtual ResponseAction
Run() override;
nit: no "virtual"

And below.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api.h:51:
DECLARE_EXTENSION_FUNCTION("app.window.showApp", UNKNOWN);
nit: Put these classes in the same order as the functions are listed in the IDL.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/api/window/window_api_unittest.cc (right):

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api_unittest.cc:27: {
You don't need all these extra {} blocks, you can just do:
scoped_ptr<Value> result = RunFunctionAndReturnValue();
result = RunFunctionAndReturnValue();

Or just use different local variable names for each.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api_unittest.cc:36:
scoped_ptr<base::Value> result =
optional: I'm one of those people who like "using", so feel free to "using
base::Value" above.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/common/...
File extensions/shell/common/api/_api_features.json (right):

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/common/...
extensions/shell/common/api/_api_features.json:16: "window": {
shell.window?

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/common/...
File extensions/shell/common/api/window.idl (right):

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/common/...
extensions/shell/common/api/window.idl:6: namespace window {
I thought this was going to be shell.window? I'm concerned about confusion
between chrome.app.window and chrome.window.

[Zachary Kuznia, 2014-10-30 20:39:57]

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/app_she...
File extensions/shell/app_shell.gyp (right):

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/app_she...
extensions/shell/app_shell.gyp:54: 'browser/api/window/window_api.cc',
On 2014/10/30 16:17:42, James Cook wrote:
> The naming convention for a foo.bar api is foo_bar/foo_bar_api.{h|cc} so I
would
> make this shell_window/shell_window_api.{h|cc}
> 
> Identity is different because it's chrome.identity not chrome.shell.identity.

Done.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/api/window/window_api.h (right):

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api.h:12: namespace shell {
On 2014/10/30 16:17:43, James Cook wrote:
> I'm on the fence about whether we need this namespace. I used it for identity
> because otherwise we end up with identically named classes in identical
> namespaces in chrome vs. app_shell. None of the rest of the app_shell code
uses
> namespace shell.
> 
> WDYT?

I'd say keep it for now.  It's easy to remove later, but harder to add, and it
makes using these functions explicit.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api.h:22: virtual
~WindowRequestHideFunction();
On 2014/10/30 16:17:42, James Cook wrote:
> nit: ~WindowRequestHideFunction() override;
> 
> And below.

Done.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api.h:25: virtual ResponseAction
Run() override;
On 2014/10/30 16:17:42, James Cook wrote:
> nit: no "virtual"
> 
> And below.

Done.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api.h:51:
DECLARE_EXTENSION_FUNCTION("app.window.showApp", UNKNOWN);
On 2014/10/30 16:17:43, James Cook wrote:
> nit: Put these classes in the same order as the functions are listed in the
IDL.

Done.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/api/window/window_api_unittest.cc (right):

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api_unittest.cc:27: {
On 2014/10/30 16:17:43, James Cook wrote:
> You don't need all these extra {} blocks, you can just do:
> scoped_ptr<Value> result = RunFunctionAndReturnValue();
> result = RunFunctionAndReturnValue();
> 
> Or just use different local variable names for each.

Done.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/browser...
extensions/shell/browser/api/window/window_api_unittest.cc:36:
scoped_ptr<base::Value> result =
On 2014/10/30 16:17:43, James Cook wrote:
> optional: I'm one of those people who like "using", so feel free to "using
> base::Value" above.

Done.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/common/...
File extensions/shell/common/api/_api_features.json (right):

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/common/...
extensions/shell/common/api/_api_features.json:16: "window": {
On 2014/10/30 16:17:43, James Cook wrote:
> shell.window?

Done.

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/common/...
File extensions/shell/common/api/window.idl (right):

https://codereview.chromium.org/686983002/diff/40001/extensions/shell/common/...
extensions/shell/common/api/window.idl:6: namespace window {
On 2014/10/30 16:17:43, James Cook wrote:
> I thought this was going to be shell.window? I'm concerned about confusion
> between chrome.app.window and chrome.window.

Done.

[James Cook, 2014-10-30 21:39:14]

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
File extensions/shell/browser/api/shell_window/shell_window_api.cc (right):

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
extensions/shell/browser/api/shell_window/shell_window_api.cc:43:
scoped_ptr<api::shell_window::ShowApp::Params> params(
optional: I'm OK with "namespace extensions::api::shell_window = shell_window;"
at the top-of-file if you want to shorten these.

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
File extensions/shell/browser/api/shell_window/shell_window_api.h (right):

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
extensions/shell/browser/api/shell_window/shell_window_api.h:12: namespace shell
{
Based on the email thread with me and Ken, please remove this namespace.

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
extensions/shell/browser/api/shell_window/shell_window_api.h:22:
~ShellWindowShowAppFunction() override;
Hooray for C++11 !

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
File extensions/shell/browser/api/shell_window/shell_window_api_unittest.cc
(right):

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
extensions/shell/browser/api/shell_window/shell_window_api_unittest.cc:16:
typedef ApiUnitTest ShellWindowApiTest;
We prefer "using ShellWindowApiTest = ApiUnitTest;" now.  I just read the weekly
C++ features email. :-)

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/common/...
File extensions/shell/common/api/shell_window.idl (right):

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/common/...
extensions/shell/common/api/shell_window.idl:23: Animation? animation;
Kinda weird that there's an animation type "hide" for a function "showApp".  I'm
thinking my suggest about adding animation type right now was a bad idea. Why
don't you take it out and we can add it back later when we know what types of
animations we really will use.

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/common/...
extensions/shell/common/api/shell_window.idl:28: // master app.
Document that one of appId or appName is required.

[Zachary Kuznia, 2014-10-30 22:14:41]

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
File extensions/shell/browser/api/shell_window/shell_window_api.cc (right):

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
extensions/shell/browser/api/shell_window/shell_window_api.cc:43:
scoped_ptr<api::shell_window::ShowApp::Params> params(
On 2014/10/30 21:39:14, James Cook wrote:
> optional: I'm OK with "namespace extensions::api::shell_window =
shell_window;"
> at the top-of-file if you want to shorten these.

Done.

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
File extensions/shell/browser/api/shell_window/shell_window_api.h (right):

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
extensions/shell/browser/api/shell_window/shell_window_api.h:12: namespace shell
{
On 2014/10/30 21:39:14, James Cook wrote:
> Based on the email thread with me and Ken, please remove this namespace.

Done.

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
extensions/shell/browser/api/shell_window/shell_window_api.h:22:
~ShellWindowShowAppFunction() override;
On 2014/10/30 21:39:14, James Cook wrote:
> Hooray for C++11 !

I know, right?!

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
File extensions/shell/browser/api/shell_window/shell_window_api_unittest.cc
(right):

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/browser...
extensions/shell/browser/api/shell_window/shell_window_api_unittest.cc:16:
typedef ApiUnitTest ShellWindowApiTest;
On 2014/10/30 21:39:14, James Cook wrote:
> We prefer "using ShellWindowApiTest = ApiUnitTest;" now.  I just read the
weekly
> C++ features email. :-)

Done.

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/common/...
File extensions/shell/common/api/shell_window.idl (right):

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/common/...
extensions/shell/common/api/shell_window.idl:23: Animation? animation;
On 2014/10/30 21:39:14, James Cook wrote:
> Kinda weird that there's an animation type "hide" for a function "showApp". 
I'm
> thinking my suggest about adding animation type right now was a bad idea. Why
> don't you take it out and we can add it back later when we know what types of
> animations we really will use.

Done.

https://codereview.chromium.org/686983002/diff/60001/extensions/shell/common/...
extensions/shell/common/api/shell_window.idl:28: // master app.
On 2014/10/30 21:39:14, James Cook wrote:
> Document that one of appId or appName is required.

Done.

[James Cook, 2014-10-30 22:50:34]

LGTM!

[Zachary Kuznia, 2014-10-31 19:16:49]

The CQ bit was checked by zork@chromium.org

[Zachary Kuznia, 2014-10-31 19:16:56]

The CQ bit was unchecked by zork@chromium.org

[Zachary Kuznia, 2014-10-31 19:17:56]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-10-31 19:19:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/686983002/80001

[commit-bot: I haz the power, 2014-10-31 19:29:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-31 19:29:59]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zachary Kuznia, 2014-10-31 20:51:37]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-10-31 20:53:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/686983002/100001

[commit-bot: I haz the power, 2014-10-31 22:33:29]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-31 22:34:07]

Patchset 6 (id:??) landed as
https://crrev.com/a713479270ad0ed2b4827eda0268eb9d265a85de
Cr-Commit-Position: refs/heads/master@{#302338}