Add support for chrome.app.window.create() to app_shell

Add support for chrome.app.window.create() to app_shell

* Introduce an AppsClient for app_shell
* Create an AppsClient for tests
* Introduce ShellAppWindowDelegate whose sole purpose is to create a NativeAppWindowViews for windows in app_shell
* Extract Chrome-specific code from apps/app_window.cc

This is part 2 of 2 and depends on https://codereview.chromium.org/171003004/

BUG=343174
TEST=browser_tests PlatformApp*
TBR=sky@chromium.org for chrome/test/base/testing_browser_process.cc

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252820

[James Cook, 2014-02-20 23:01:17]

benwells, PTAL.

[James Cook, 2014-02-21 17:38:01]

miket, would you feel comfortable reviewing this?  benwells LGTM'd part 1 of
this patch set.  I'm blocked on this landing and it's Saturday in his part of
the world.

[miket_OOO, 2014-02-21 18:25:38]

lgtm

https://codereview.chromium.org/166833004/diff/30001/apps/apps_client.h
File apps/apps_client.h (right):

https://codereview.chromium.org/166833004/diff/30001/apps/apps_client.h#newco...
apps/apps_client.h:41: // after a matching number of calls to EndKeepAlive() are
made.
Nit: if the keepalive logic is a counter rather than a boolean, I wonder whether
it'd be clearer to name it accordingly, like "IncrementKeepAliveCount()."
Ideally the documentation would be optional because the method name is so clear.

Again, it's a nit. I'm not sure someone would reasonably expect a single
End...() call to work after two Start() calls, at least not without reading the
documentation. So I can see it going either way.

https://codereview.chromium.org/166833004/diff/30001/apps/shell/browser/shell...
File apps/shell/browser/shell_app_window_delegate.cc (right):

https://codereview.chromium.org/166833004/diff/30001/apps/shell/browser/shell...
apps/shell/browser/shell_app_window_delegate.cc:42: // Opening a new tab/window
is not supported.
Any chance a future developer will be confused by what's going on here? In other
words, should we debug-log what's being described in these comments?

[James Cook, 2014-02-22 00:11:17]

https://codereview.chromium.org/166833004/diff/30001/apps/apps_client.h
File apps/apps_client.h (right):

https://codereview.chromium.org/166833004/diff/30001/apps/apps_client.h#newco...
apps/apps_client.h:41: // after a matching number of calls to EndKeepAlive() are
made.
On 2014/02/21 18:25:38, miket wrote:
> Nit: if the keepalive logic is a counter rather than a boolean, I wonder
whether
> it'd be clearer to name it accordingly, like "IncrementKeepAliveCount()."
> Ideally the documentation would be optional because the method name is so
clear.
> 
> Again, it's a nit. I'm not sure someone would reasonably expect a single
> End...() call to work after two Start() calls, at least not without reading
the
> documentation. So I can see it going either way.

The name is bad, but I prefer to have the client interface method name match the
implementation name in Chrome.  I think we should rename it everywhere.  Filed
crbug.com/345918 to do that in another CL.

https://codereview.chromium.org/166833004/diff/30001/apps/shell/browser/shell...
File apps/shell/browser/shell_app_window_delegate.cc (right):

https://codereview.chromium.org/166833004/diff/30001/apps/shell/browser/shell...
apps/shell/browser/shell_app_window_delegate.cc:42: // Opening a new tab/window
is not supported.
On 2014/02/21 18:25:38, miket wrote:
> Any chance a future developer will be confused by what's going on here? In
other
> words, should we debug-log what's being described in these comments?

Done.

[James Cook, 2014-02-22 14:50:33]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-02-22 14:50:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/166833004/180001

[commit-bot: I haz the power, 2014-02-22 15:57:31]

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

[commit-bot: I haz the power, 2014-02-22 15:57:31]

Retried try job too often on win_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[James Cook, 2014-02-22 18:20:30]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-02-22 18:20:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/166833004/180001

[commit-bot: I haz the power, 2014-02-22 19:54:00]

Change committed as 252820

[Haojian Wu, 2014-02-25 13:48:47]

https://codereview.chromium.org/166833004/diff/180001/apps/shell/browser/shel...
File apps/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/166833004/diff/180001/apps/shell/browser/shel...
apps/shell/browser/shell_browser_main_parts.cc:168: void
ShellBrowserMainParts::OnWindowTreeHostCloseRequested(
James@, could you tell me when will this method get invoked?

I have done some tracks of this method, I don't see any invokes of this method.
 
From the
document:https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/root_window_observer.h&q=nWindowTreeHostCloseRequested&sq=package:chromium&type=cs&l=31
, it says "Invoked when the native windowing system sends us a request to close
our window.". Does this mean when we close the button of app_shell root window,
it will be invoked?

[James Cook, 2014-02-25 17:47:31]

https://codereview.chromium.org/166833004/diff/180001/apps/shell/browser/shel...
File apps/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/166833004/diff/180001/apps/shell/browser/shel...
apps/shell/browser/shell_browser_main_parts.cc:168: void
ShellBrowserMainParts::OnWindowTreeHostCloseRequested(
On 2014/02/25 13:48:48, Haojian Wu wrote:
> James@, could you tell me when will this method get invoked?
> 
> I have done some tracks of this method, I don't see any invokes of this
method.
>  
> From the
>
document:https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/root_window_observer.h&q=nWindowTreeHostCloseRequested&sq=package:chromium&type=cs&l=31
> , it says "Invoked when the native windowing system sends us a request to
close
> our window.". Does this mean when we close the button of app_shell root
window,
> it will be invoked?

I'm not very familiar with that method, but it sounds like it should be called
when the user clicks the close button of the root window.  Someone refactored
this recently -- you might look at the svn blame / git blame for this line and
see what they changed.  Maybe we're not observing the root window correctly.