Add ShellAppsClient, ShellAppDelegate, ShellNativeAppWindow

Add ShellAppsClient, ShellAppDelegate, ShellNativeAppWindow

BUG=387288
Committed: https://crrev.com/cb9416ecdaf84bf8e07fbdab7e6245d5a3b7aff7
Cr-Commit-Position: refs/heads/master@{#293666}

[hashimoto, 2014-09-05 09:55:31]

Patchset #1 (id:1) has been deleted

[hashimoto, 2014-09-05 10:00:00]

Patchset #1 (id:20001) has been deleted

[hashimoto, 2014-09-05 10:08:31]

hashimoto@chromium.org changed reviewers:
+ jamescook@chromium.org

[hashimoto, 2014-09-05 10:08:31]

James, could you review this change?

It'll be able to use app.window.create API (calculator works at least) in
app_shell after this patch and other two patches
(https://codereview.chromium.org/546683002/ and
https://codereview.chromium.org/540273002/) get landed.

[James Cook, 2014-09-05 16:26:43]

Looking good, a few comments.

Just to double-check: When you say calculator works, do you mean the calculator
in chrome/common/extensions/docs/examples/apps/calculator/app?  If so, did you
modify it to take out the call to chrome.shell.createWindow?  If so, that's
great!

Can you write a small test that creates an app window? Hopefully it could live
in app_shell_unittests, but if not then it could live in app_shell_browsertests.

Thanks again for doing all this refactoring.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_app_delegate.cc (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.cc:22: NOTIMPLEMENTED();
Thanks for adding these NOTIMPLEMENTEDs. They make it much easier to debug
app_shell issues.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.cc:60: NOTIMPLEMENTED();
This should be hooked up to extensions/shell/browser/media_capture_util.h

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.cc:81: return false;
nit: maybe return true here? app_shell windows are always visible

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_app_delegate.h (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.h:12: class ShellAppDelegate :
public AppDelegate {
Can you add a brief comment about this class?

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.h:17: // AppsClient overrides:
AppDelegate overrides?

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_apps_client.cc (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_apps_client.cc:39:
native_app_window->GetNativeWindow());
Do you need to call Show() on the Aura window?  The code in
ShellDesktopController::CreateAppWindow() does that.

Also, I think you should break ShellDesktopController::CreateAppWindow() into
two functions, CreateAppWindow() and AddAppWindow() to make it more clear that
there are multiple places in the code that add windows.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_apps_client.h (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_apps_client.h:8: #include "base/basictypes.h"
nit: macros.h and compiler_specific.h, since you aren't actually using integer
types.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_apps_client.h:13: class ShellAppsClient : public
AppsClient {
nit: brief class comment?

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_apps_client.h:30: virtual bool
IsCurrentChannelOlderThanDev() OVERRIDE;
Not for this CL, but this seems like an odd method to have on this interface. I
would expect it to be something like "IsCurrentChannelOlderThan(const
std::string& channel)". I know the current channel constants exist only in
src/chrome, but I would be OK with introducing well known strings for "canary",
"dev", "beta", "stable" for these sorts of interfaces. Or we could push the
concept of channel into src/components.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_browser_main_parts.cc:113: apps_client_.reset(new
ShellAppsClient());
Nice that the order of initialization matches the header.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window.h (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window.h:13: class
ShellNativeAppWindow : public NativeAppWindow {
nit: small class comment?

[hashimoto, 2014-09-05 17:27:11]

Patchset #2 (id:60001) has been deleted

[hashimoto, 2014-09-05 17:37:10]

On 2014/09/05 16:26:43, James Cook wrote:
> Looking good, a few comments.
> 
> Just to double-check: When you say calculator works, do you mean the
calculator
> in chrome/common/extensions/docs/examples/apps/calculator/app?  If so, did you
Yes.
> modify it to take out the call to chrome.shell.createWindow?  If so, that's
Yes, I locally changed controller.js to force it to use
chrome.app.window.create.
> great!
> 
> Can you write a small test that creates an app window? Hopefully it could live
> in app_shell_unittests, but if not then it could live in
app_shell_browsertests.
Currently I cannot because app.window.create doesn't work yet without the two
CLs mentioned above landed.
I'll add tests once things get ready.
> 
> Thanks again for doing all this refactoring.
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> File extensions/shell/browser/shell_app_delegate.cc (right):
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_app_delegate.cc:22: NOTIMPLEMENTED();
> Thanks for adding these NOTIMPLEMENTEDs. They make it much easier to debug
> app_shell issues.
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_app_delegate.cc:60: NOTIMPLEMENTED();
> This should be hooked up to extensions/shell/browser/media_capture_util.h
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_app_delegate.cc:81: return false;
> nit: maybe return true here? app_shell windows are always visible
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> File extensions/shell/browser/shell_app_delegate.h (right):
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_app_delegate.h:12: class ShellAppDelegate :
> public AppDelegate {
> Can you add a brief comment about this class?
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_app_delegate.h:17: // AppsClient overrides:
> AppDelegate overrides?
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> File extensions/shell/browser/shell_apps_client.cc (right):
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_apps_client.cc:39:
> native_app_window->GetNativeWindow());
> Do you need to call Show() on the Aura window?  The code in
> ShellDesktopController::CreateAppWindow() does that.
> 
> Also, I think you should break ShellDesktopController::CreateAppWindow() into
> two functions, CreateAppWindow() and AddAppWindow() to make it more clear that
> there are multiple places in the code that add windows.
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> File extensions/shell/browser/shell_apps_client.h (right):
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_apps_client.h:8: #include "base/basictypes.h"
> nit: macros.h and compiler_specific.h, since you aren't actually using integer
> types.
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_apps_client.h:13: class ShellAppsClient :
public
> AppsClient {
> nit: brief class comment?
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_apps_client.h:30: virtual bool
> IsCurrentChannelOlderThanDev() OVERRIDE;
> Not for this CL, but this seems like an odd method to have on this interface.
I
> would expect it to be something like "IsCurrentChannelOlderThan(const
> std::string& channel)". I know the current channel constants exist only in
> src/chrome, but I would be OK with introducing well known strings for
"canary",
> "dev", "beta", "stable" for these sorts of interfaces. Or we could push the
> concept of channel into src/components.
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> File extensions/shell/browser/shell_browser_main_parts.cc (right):
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_browser_main_parts.cc:113:
apps_client_.reset(new
> ShellAppsClient());
> Nice that the order of initialization matches the header.
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> File extensions/shell/browser/shell_native_app_window.h (right):
> 
>
https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_native_app_window.h:13: class
> ShellNativeAppWindow : public NativeAppWindow {
> nit: small class comment?

[hashimoto, 2014-09-05 17:37:17]

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_app_delegate.cc (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.cc:22: NOTIMPLEMENTED();
On 2014/09/05 16:26:42, James Cook wrote:
> Thanks for adding these NOTIMPLEMENTEDs. They make it much easier to debug
> app_shell issues.
I left many methods unimplemented to start with the minimum set of
functionality.
Hope these NOTIMPLEMENTEDs get removed soon.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.cc:60: NOTIMPLEMENTED();
On 2014/09/05 16:26:42, James Cook wrote:
> This should be hooked up to extensions/shell/browser/media_capture_util.h

Done.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.cc:81: return false;
On 2014/09/05 16:26:42, James Cook wrote:
> nit: maybe return true here? app_shell windows are always visible

Done.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_app_delegate.h (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.h:12: class ShellAppDelegate :
public AppDelegate {
On 2014/09/05 16:26:42, James Cook wrote:
> Can you add a brief comment about this class?

Done.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.h:17: // AppsClient overrides:
On 2014/09/05 16:26:42, James Cook wrote:
> AppDelegate overrides?

Oops, done.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_apps_client.cc (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_apps_client.cc:39:
native_app_window->GetNativeWindow());
On 2014/09/05 16:26:42, James Cook wrote:
> Do you need to call Show() on the Aura window?  The code in
> ShellDesktopController::CreateAppWindow() does that.
API implementation (extensions/browser/api/app_window/app_window_api.cc) is
responsible to call Show() instead.
> 
> Also, I think you should break ShellDesktopController::CreateAppWindow() into
> two functions, CreateAppWindow() and AddAppWindow() to make it more clear that
> there are multiple places in the code that add windows.
Added AddAppWindow(), but couldn't remove the Add() part from CreateAppWindow()
because athena's implementation uses
athena::ActivityManager::Get()->AddActivity() to add the ShellAppWindow to the
hierarchy.
To get rid of Add() part from CreateAppWindow(), athena's implementation
requires AddAppWindow() to take ShellAppWindow*.
But we want to use AddAppWindow() also to add AppWindow so this doesn't work
without massively changing the code of athena or app_shell.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_apps_client.h (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_apps_client.h:8: #include "base/basictypes.h"
On 2014/09/05 16:26:42, James Cook wrote:
> nit: macros.h and compiler_specific.h, since you aren't actually using integer
> types.

Done.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_apps_client.h:13: class ShellAppsClient : public
AppsClient {
On 2014/09/05 16:26:42, James Cook wrote:
> nit: brief class comment?

Done.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_apps_client.h:30: virtual bool
IsCurrentChannelOlderThanDev() OVERRIDE;
On 2014/09/05 16:26:43, James Cook wrote:
> Not for this CL, but this seems like an odd method to have on this interface.
I
> would expect it to be something like "IsCurrentChannelOlderThan(const
> std::string& channel)". I know the current channel constants exist only in
> src/chrome, but I would be OK with introducing well known strings for
"canary",
> "dev", "beta", "stable" for these sorts of interfaces. Or we could push the
> concept of channel into src/components.

Makes sense.
Filed crbug.com/411352

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_browser_main_parts.cc:113: apps_client_.reset(new
ShellAppsClient());
On 2014/09/05 16:26:43, James Cook wrote:
> Nice that the order of initialization matches the header.

Thanks,
Things are initialized in the same order as Chrome initializes them.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window.h (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window.h:13: class
ShellNativeAppWindow : public NativeAppWindow {
On 2014/09/05 16:26:43, James Cook wrote:
> nit: small class comment?

Done.

[James Cook, 2014-09-05 17:41:51]

LGTM!

Please include me on the CL for the test.

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_apps_client.cc (right):

https://codereview.chromium.org/547593002/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_apps_client.cc:39:
native_app_window->GetNativeWindow());
On 2014/09/05 17:37:17, hashimoto wrote:
> On 2014/09/05 16:26:42, James Cook wrote:
> > Do you need to call Show() on the Aura window?  The code in
> > ShellDesktopController::CreateAppWindow() does that.
> API implementation (extensions/browser/api/app_window/app_window_api.cc) is
> responsible to call Show() instead.
> > 
> > Also, I think you should break ShellDesktopController::CreateAppWindow()
into
> > two functions, CreateAppWindow() and AddAppWindow() to make it more clear
that
> > there are multiple places in the code that add windows.
> Added AddAppWindow(), but couldn't remove the Add() part from
CreateAppWindow()
> because athena's implementation uses
> athena::ActivityManager::Get()->AddActivity() to add the ShellAppWindow to the
> hierarchy.
> To get rid of Add() part from CreateAppWindow(), athena's implementation
> requires AddAppWindow() to take ShellAppWindow*.
> But we want to use AddAppWindow() also to add AppWindow so this doesn't work
> without massively changing the code of athena or app_shell.

OK, that makes sense.

[hashimoto, 2014-09-05 17:51:44]

hashimoto@chromium.org changed reviewers:
+ oshima@chromium.org

[hashimoto, 2014-09-05 17:51:44]

oshima@, could you take a look at this change as an owner of athena_main.cc?

[oshima, 2014-09-05 18:06:08]

https://codereview.chromium.org/547593002/diff/70001/extensions/shell/browser...
File extensions/shell/browser/desktop_controller.h (right):

https://codereview.chromium.org/547593002/diff/70001/extensions/shell/browser...
extensions/shell/browser/desktop_controller.h:45: virtual void
AddAppWindow(aura::Window* window) = 0;
What I had in my mind was that CreateAppWindow will be gone and replaced with
AppsClient, and athena_main can use it's own impl, instead of using
DesktopController. 

Is there an issue with this approach?

[hashimoto, 2014-09-05 18:24:58]

On 2014/09/05 18:06:08, oshima wrote:
>
https://codereview.chromium.org/547593002/diff/70001/extensions/shell/browser...
> File extensions/shell/browser/desktop_controller.h (right):
> 
>
https://codereview.chromium.org/547593002/diff/70001/extensions/shell/browser...
> extensions/shell/browser/desktop_controller.h:45: virtual void
> AddAppWindow(aura::Window* window) = 0;
> What I had in my mind was that CreateAppWindow will be gone and replaced with
> AppsClient, and athena_main can use it's own impl, instead of using
> DesktopController. 
> 
> Is there an issue with this approach?
We'll remove CreateAppWindow() once app.window.create gets ready.
This empty implementation is tentatively needed to keep things working while we
transition from ShellAppWindow to AppWindow.

[oshima, 2014-09-05 18:26:27]

ok, lgtm then

[hashimoto, 2014-09-05 18:27:05]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-09-05 18:28:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/547593002/80001

[commit-bot: I haz the power, 2014-09-05 19:19:12]

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

[commit-bot: I haz the power, 2014-09-05 19:19:13]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[hashimoto, 2014-09-06 02:14:32]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-09-06 02:15:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/547593002/80001

[commit-bot: I haz the power, 2014-09-06 08:18:12]

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

[commit-bot: I haz the power, 2014-09-06 08:18:13]

Exceeded time limit waiting for builds to trigger.

[hashimoto, 2014-09-07 14:23:59]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-09-07 14:26:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/547593002/80001

[commit-bot: I haz the power, 2014-09-07 20:50:56]

Committed patchset #3 (id:80001) as fd2e3d7d0f5bab28d23ed651d30f126d1ba08e50

[commit-bot: I haz the power, 2014-09-10 03:44:21]

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