app_shell: Shut down in response to power button.

app_shell: Shut down in response to power button.

Also fix a dependency issue in
chromeos_test_support_without_gmock, make
ExtensionsTestSuite call
GLSurface::InitializeOneOffForTests(), and avoid
double-initializating Aura in app_shell.

BUG=405967
Committed: https://crrev.com/5a4560307a4250de157ac7af8954f4b059ed9fc2
Cr-Commit-Position: refs/heads/master@{#291607}

[Daniel Erat, 2014-08-22 20:41:35]

james: please review everything
yoz: extensions/test
dana: ui/compositor
oshima: fyi for aura; let me know if you have suggestions

https://codereview.chromium.org/468393005/diff/40001/chromeos/dbus/fake_power...
File chromeos/dbus/fake_power_manager_client.cc (right):

https://codereview.chromium.org/468393005/diff/40001/chromeos/dbus/fake_power...
chromeos/dbus/fake_power_manager_client.cc:34: bool
FakePowerManagerClient::HasObserver(Observer* observer) {
sorry for all the churn here; misordered .cc files are a pet peeve. the only
changes that i made to this class are related to num_request_shutdown_calls_.

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

https://codereview.chromium.org/468393005/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller.cc:283: void
ShellDesktopController::CreateRootWindow() {
just moving this now that it's private

[danakj, 2014-08-22 20:47:28]

LGTM

[oshima, 2014-08-22 21:15:33]

one q:

https://codereview.chromium.org/468393005/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_desktop_controller.cc (left):

https://codereview.chromium.org/468393005/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller.cc:166:
aura::Env::CreateInstance(true);
where Create/Delete Instance are called now?

[Daniel Erat, 2014-08-22 21:26:13]

https://codereview.chromium.org/468393005/diff/40001/extensions/shell/browser...
File extensions/shell/browser/shell_desktop_controller.cc (left):

https://codereview.chromium.org/468393005/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller.cc:166:
aura::Env::CreateInstance(true);
On 2014/08/22 21:15:33, oshima wrote:
> where Create/Delete Instance are called now?

content::BrowserMainLoop::InitializeToolkit() calls CreateInstance(). i'm less
sure where DeleteInstance() is supposed to be called -- i see it in
content::Shell::PlatformExit() and chrome's BrowserProcessImpl::StartTearDown(),
for example. maybe that means that app_shell should call it explicitly from
ShellBrowserMainParts, although having setup and destruction in different
locations is weird.

[oshima, 2014-08-22 21:35:17]

On 2014/08/22 21:26:13, Daniel Erat wrote:
>
https://codereview.chromium.org/468393005/diff/40001/extensions/shell/browser...
> File extensions/shell/browser/shell_desktop_controller.cc (left):
> 
>
https://codereview.chromium.org/468393005/diff/40001/extensions/shell/browser...
> extensions/shell/browser/shell_desktop_controller.cc:166:
> aura::Env::CreateInstance(true);
> On 2014/08/22 21:15:33, oshima wrote:
> > where Create/Delete Instance are called now?
> 
> content::BrowserMainLoop::InitializeToolkit() calls CreateInstance(). i'm less
> sure where DeleteInstance() is supposed to be called -- i see it in
> content::Shell::PlatformExit() and chrome's
BrowserProcessImpl::StartTearDown(),
> for example. maybe that means that app_shell should call it explicitly from
> ShellBrowserMainParts, although having setup and destruction in different
> locations is weird.

Maybe here for chrome

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

ash has its own, which may be no-op when run in chrome. It'd be nice to have it
in the same place for content shell,
but that's probably outside of the scope of this CL. I'm not sure if you need to
do that in app_shell though.

[James Cook, 2014-08-23 00:55:01]

LGTM with nits

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

https://codereview.chromium.org/468393005/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller_unittest.cc:7: #include
"base/macros.h"
Nice IWYU

https://codereview.chromium.org/468393005/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller_unittest.cc:55:
chromeos::FakePowerManagerClient* power_manager_client_;  // Weak pointer.
nit: "Weak pointer" -> "Not owned", because we have a real WeakPtr class and
this isn't one.

https://codereview.chromium.org/468393005/diff/40001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller_unittest.cc:63:
TEST_F(ShellDesktopControllerTest, PowerButton) {
nit: Comment above the test about what it intends to test.

https://codereview.chromium.org/468393005/diff/40001/ui/compositor/test/in_pr...
File ui/compositor/test/in_process_context_factory.cc (right):

https://codereview.chromium.org/468393005/diff/40001/ui/compositor/test/in_pr...
ui/compositor/test/in_process_context_factory.cc:23: #if defined(UNIT_TEST)
I'm not sure this works. In particular, I think only *_unittest.cc files (and
maybe some test support files?) are compiled with this flag. Otherwise the
entire project would have to be rebuilt to build the unit_tests binary.

"git grep UNIT_TEST" shows it only used in headers, which could be compiled
differently for the unit test binary due to inlining.

[Yoyo Zhou, 2014-08-23 01:18:03]

extensions/test LGTM

https://chromiumcodereview.appspot.com/468393005/diff/40001/extensions/test/e...
File extensions/test/extensions_unittests_main.cc (right):

https://chromiumcodereview.appspot.com/468393005/diff/40001/extensions/test/e...
extensions/test/extensions_unittests_main.cc:68:
gfx::GLSurface::InitializeOneOffForTests();
Hah, James commented in my earlier CL
(https://chromiumcodereview.appspot.com/461273003/) that we shouldn't need this
for all tests, so ideally it should be defined only for those that need it. But
I remember there are likely to be startup ordering issues that prevent that.

[Daniel Erat, 2014-08-23 02:21:31]

https://chromiumcodereview.appspot.com/468393005/diff/40001/extensions/shell/...
File extensions/shell/browser/shell_desktop_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/468393005/diff/40001/extensions/shell/...
extensions/shell/browser/shell_desktop_controller_unittest.cc:55:
chromeos::FakePowerManagerClient* power_manager_client_;  // Weak pointer.
On 2014/08/23 00:55:00, James Cook wrote:
> nit: "Weak pointer" -> "Not owned", because we have a real WeakPtr class and
> this isn't one.

i can never remember if you're the reviewer who tells me to use "Weak pointer"
or the one who tells me to use "Not owned". now i know :-P

https://chromiumcodereview.appspot.com/468393005/diff/40001/extensions/shell/...
extensions/shell/browser/shell_desktop_controller_unittest.cc:63:
TEST_F(ShellDesktopControllerTest, PowerButton) {
On 2014/08/23 00:55:00, James Cook wrote:
> nit: Comment above the test about what it intends to test.

Done.

https://chromiumcodereview.appspot.com/468393005/diff/40001/ui/compositor/tes...
File ui/compositor/test/in_process_context_factory.cc (right):

https://chromiumcodereview.appspot.com/468393005/diff/40001/ui/compositor/tes...
ui/compositor/test/in_process_context_factory.cc:23: #if defined(UNIT_TEST)
On 2014/08/23 00:55:00, James Cook wrote:
> I'm not sure this works. In particular, I think only *_unittest.cc files (and
> maybe some test support files?) are compiled with this flag. Otherwise the
> entire project would have to be rebuilt to build the unit_tests binary.
> 
> "git grep UNIT_TEST" shows it only used in headers, which could be compiled
> differently for the unit test binary due to inlining.

what you write makes sense. it worked when i tested it, but probably i just
happened to build the test first. i've changed this back to a DCHECK and tweaked
the message.

[Daniel Erat, 2014-08-23 02:38:20]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-08-23 04:56:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/derat@chromium.org/468393005/60001

[commit-bot: I haz the power, 2014-08-23 11:23:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-23 13:17:37]

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

[commit-bot: I haz the power, 2014-08-23 13:17:38]

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

[Daniel Erat, 2014-08-23 14:24:43]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-08-23 14:26:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/derat@chromium.org/468393005/60001

[commit-bot: I haz the power, 2014-08-23 22:18:16]

Committed patchset #4 (60001) as 84e6ea79d2e51795da8e9e1396b6e0af754ad4c1

[commit-bot: I haz the power, 2014-09-10 02:32:02]

Patchset 4 (id:??) landed as
https://crrev.com/5a4560307a4250de157ac7af8954f4b059ed9fc2
Cr-Commit-Position: refs/heads/master@{#291607}