mash: Move SystemTrayDelegate ownership to WmShell

mash: Move SystemTrayDelegate ownership to WmShell

This makes it possible to use inside //ash/common.

(SystemTrayDelegate is a dependency of TrayUpdate, which I'm in the process
of moving to //ash/common.)

* Make WmShell non-pure-virtual
* Fix now-unneeded includes of shell.h
* Fix some unnecessary ash:: qualifiers along the way

BUG=615155
TEST=existing ash_unittests, chrome unit_tests and browser_tests
TBR=sky@chromium.org for mechanical changes to //chrome/browser

Committed: https://crrev.com/443e42d71ea1b9836c434c661cb2cd2517d413d4
Cr-Commit-Position: refs/heads/master@{#399277}

[James Cook, 2016-06-10 17:56:54]

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

[James Cook, 2016-06-10 17:56:55]

msw, please take a look.

sky@ and I agreed yesterday that we're OK with WmShell not being pure-virtual.
He's going to put more stuff there in a CL he's working on; I just happened to
beat him to it.

(He had considered doing something similar in the past, see
https://codereview.chromium.org/2041423002/diff/80001/ash/common/wm_shell.h -- I
was against it then, but he convinced me we should switch over. Using
wm_shell_common_ is awkward and we're going to remove it.)

[msw, 2016-06-10 18:48:21]

lgtm with nits

https://codereview.chromium.org/2058173002/diff/1/ash/content/display/screen_...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2058173002/diff/1/ash/content/display/screen_...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:420:
test::TestSystemTrayDelegate* tray_delegate =
nit: use AshTestBase::GetSystemTrayDelegate

https://codereview.chromium.org/2058173002/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/shell.cc#newcode1083
ash/shell.cc:1083: // Initialize system tray delegate before initializing
StatusAreaWidget.
I don't think these StatusAreaWidget comments are even still relevant; consider
removing them and calling Initialize immediately after SetSystemTrayDelegate? We
could even have WmShell::SetSystemTrayDelegate call Initialize and Shutdown...

https://codereview.chromium.org/2058173002/diff/1/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2058173002/diff/1/ash/shell.h#newcode151
ash/shell.h:151: class SystemTrayDelegate;
nit: remove

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/bluetoo...
File ash/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/bluetoo...
ash/system/chromeos/bluetooth/tray_bluetooth.cc:294: if
(!ash::WmShell::Get()->system_tray_delegate()->ShouldShowSettings()) {
nit: remove curlies

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/setting...
File ash/system/chromeos/settings/tray_settings.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/setting...
ash/system/chromeos/settings/tray_settings.cc:76: bool user_adding_running =
Shell::GetInstance()
nit: |adding_user| or |in_secondary_login| or inline?

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/setting...
ash/system/chromeos/settings/tray_settings.cc:81: login_status_ ==
LoginStatus::LOCKED || user_adding_running)
nit: curlies

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/supervi...
File ash/system/chromeos/supervised/tray_supervised_user_unittest.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/supervi...
ash/system/chromeos/supervised/tray_supervised_user_unittest.cc:66:
test::TestSystemTrayDelegate* delegate =
nit: use AshTestBase::GetSystemTrayDelegate

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/tray_di...
File ash/system/chromeos/tray_display_unittest.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/tray_di...
ash/system/chromeos/tray_display_unittest.cc:447: test::TestSystemTrayDelegate*
tray_delegate =
nit: AshTestBase::GetSystemTrayDelegate here and twice more below.

https://codereview.chromium.org/2058173002/diff/1/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/system/web_notification...
ash/system/web_notification/web_notification_tray_unittest.cc:289:
test::TestSystemTrayDelegate* tray_delegate =
nit: use AshTestBase::GetSystemTrayDelegate

https://codereview.chromium.org/2058173002/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/device_system_use_24hour_clock_browsertest.cc
(right):

https://codereview.chromium.org/2058173002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_system_use_24hour_clock_browsertest.cc:71:
return tray_delegate->GetShouldUse24HourClockForTesting();
aside: calls into g_browser_process under the hood... yuck!

https://codereview.chromium.org/2058173002/diff/1/chrome/browser/chromeos/pow...
File chrome/browser/chromeos/power/power_button_observer.cc (right):

https://codereview.chromium.org/2058173002/diff/1/chrome/browser/chromeos/pow...
chrome/browser/chromeos/power/power_button_observer.cc:28: if
(!ash::WmShell::Get()->system_tray_delegate())
optional nit: flip conditional and swap returns

[James Cook, 2016-06-10 20:20:33]

Thanks for the quick review.

https://codereview.chromium.org/2058173002/diff/1/ash/content/display/screen_...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2058173002/diff/1/ash/content/display/screen_...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:420:
test::TestSystemTrayDelegate* tray_delegate =
On 2016/06/10 18:48:20, msw wrote:
> nit: use AshTestBase::GetSystemTrayDelegate

Done.

https://codereview.chromium.org/2058173002/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/shell.cc#newcode1083
ash/shell.cc:1083: // Initialize system tray delegate before initializing
StatusAreaWidget.
On 2016/06/10 18:48:20, msw wrote:
> I don't think these StatusAreaWidget comments are even still relevant;
consider
> removing them and calling Initialize immediately after SetSystemTrayDelegate?
We
> could even have WmShell::SetSystemTrayDelegate call Initialize and Shutdown...

I removed the comments and moved initialization/shutdown into
WmShell::SetSystemTrayDelegate.

https://codereview.chromium.org/2058173002/diff/1/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2058173002/diff/1/ash/shell.h#newcode151
ash/shell.h:151: class SystemTrayDelegate;
On 2016/06/10 18:48:20, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/bluetoo...
File ash/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/bluetoo...
ash/system/chromeos/bluetooth/tray_bluetooth.cc:294: if
(!ash::WmShell::Get()->system_tray_delegate()->ShouldShowSettings()) {
On 2016/06/10 18:48:20, msw wrote:
> nit: remove curlies

Done.

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/setting...
File ash/system/chromeos/settings/tray_settings.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/setting...
ash/system/chromeos/settings/tray_settings.cc:76: bool user_adding_running =
Shell::GetInstance()
On 2016/06/10 18:48:21, msw wrote:
> nit: |adding_user| or |in_secondary_login| or inline?

I went with adding_user, since that's what's actually happening. ("secondary
login" just sounds weird)

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/setting...
ash/system/chromeos/settings/tray_settings.cc:81: login_status_ ==
LoginStatus::LOCKED || user_adding_running)
On 2016/06/10 18:48:21, msw wrote:
> nit: curlies

Done.

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/supervi...
File ash/system/chromeos/supervised/tray_supervised_user_unittest.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/supervi...
ash/system/chromeos/supervised/tray_supervised_user_unittest.cc:66:
test::TestSystemTrayDelegate* delegate =
On 2016/06/10 18:48:21, msw wrote:
> nit: use AshTestBase::GetSystemTrayDelegate

Done. (I'm glad you noticed these!)

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/tray_di...
File ash/system/chromeos/tray_display_unittest.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/system/chromeos/tray_di...
ash/system/chromeos/tray_display_unittest.cc:447: test::TestSystemTrayDelegate*
tray_delegate =
On 2016/06/10 18:48:21, msw wrote:
> nit: AshTestBase::GetSystemTrayDelegate here and twice more below.

Done.

https://codereview.chromium.org/2058173002/diff/1/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/2058173002/diff/1/ash/system/web_notification...
ash/system/web_notification/web_notification_tray_unittest.cc:289:
test::TestSystemTrayDelegate* tray_delegate =
On 2016/06/10 18:48:21, msw wrote:
> nit: use AshTestBase::GetSystemTrayDelegate

Done.

https://codereview.chromium.org/2058173002/diff/1/chrome/browser/chromeos/pow...
File chrome/browser/chromeos/power/power_button_observer.cc (right):

https://codereview.chromium.org/2058173002/diff/1/chrome/browser/chromeos/pow...
chrome/browser/chromeos/power/power_button_observer.cc:28: if
(!ash::WmShell::Get()->system_tray_delegate())
On 2016/06/10 18:48:21, msw wrote:
> optional nit: flip conditional and swap returns

Done.

[James Cook, 2016-06-10 20:21:23]

Description was changed from

==========
mash: Move SystemTrayDelegate ownership to WmShell

This makes it possible to use inside //ash/common.

(SystemTrayDelegate is a dependency of TrayUpdate, which I'm in the process
of moving to //ash/common.)

* Make WmShell non-pure-virtual
* Fix now-unneeded includes of shell.h
* Fix some unnecessary ash:: qualifiers along the way

BUG=615155
TEST=existing ash_unittests, chrome unit_tests and browser_tests
==========

to

==========
mash: Move SystemTrayDelegate ownership to WmShell

This makes it possible to use inside //ash/common.

(SystemTrayDelegate is a dependency of TrayUpdate, which I'm in the process
of moving to //ash/common.)

* Make WmShell non-pure-virtual
* Fix now-unneeded includes of shell.h
* Fix some unnecessary ash:: qualifiers along the way

BUG=615155
TEST=existing ash_unittests, chrome unit_tests and browser_tests
TBR=sky@chromium.org for mechanical changes to //chrome/browser
==========

[James Cook, 2016-06-10 20:21:30]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-06-10 20:21:30]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2058173002/#ps20001
(title: "review feedback")

[commit-bot: I haz the power, 2016-06-10 20:21:52]

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

[msw, 2016-06-10 20:48:10]

still lgtm

https://codereview.chromium.org/2058173002/diff/20001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2058173002/diff/20001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:24: class Shell;
nit: needed?

https://codereview.chromium.org/2058173002/diff/20001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2058173002/diff/20001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:11:
#include "ash/common/wm_shell.h"
nit: remove

[commit-bot: I haz the power, 2016-06-10 21:45:01]

Description was changed from

==========
mash: Move SystemTrayDelegate ownership to WmShell

This makes it possible to use inside //ash/common.

(SystemTrayDelegate is a dependency of TrayUpdate, which I'm in the process
of moving to //ash/common.)

* Make WmShell non-pure-virtual
* Fix now-unneeded includes of shell.h
* Fix some unnecessary ash:: qualifiers along the way

BUG=615155
TEST=existing ash_unittests, chrome unit_tests and browser_tests
TBR=sky@chromium.org for mechanical changes to //chrome/browser
==========

to

==========
mash: Move SystemTrayDelegate ownership to WmShell

This makes it possible to use inside //ash/common.

(SystemTrayDelegate is a dependency of TrayUpdate, which I'm in the process
of moving to //ash/common.)

* Make WmShell non-pure-virtual
* Fix now-unneeded includes of shell.h
* Fix some unnecessary ash:: qualifiers along the way

BUG=615155
TEST=existing ash_unittests, chrome unit_tests and browser_tests
TBR=sky@chromium.org for mechanical changes to //chrome/browser
==========

[commit-bot: I haz the power, 2016-06-10 21:45:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-10 21:45:21]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 21:46:22]

Description was changed from

==========
mash: Move SystemTrayDelegate ownership to WmShell

This makes it possible to use inside //ash/common.

(SystemTrayDelegate is a dependency of TrayUpdate, which I'm in the process
of moving to //ash/common.)

* Make WmShell non-pure-virtual
* Fix now-unneeded includes of shell.h
* Fix some unnecessary ash:: qualifiers along the way

BUG=615155
TEST=existing ash_unittests, chrome unit_tests and browser_tests
TBR=sky@chromium.org for mechanical changes to //chrome/browser
==========

to

==========
mash: Move SystemTrayDelegate ownership to WmShell

This makes it possible to use inside //ash/common.

(SystemTrayDelegate is a dependency of TrayUpdate, which I'm in the process
of moving to //ash/common.)

* Make WmShell non-pure-virtual
* Fix now-unneeded includes of shell.h
* Fix some unnecessary ash:: qualifiers along the way

BUG=615155
TEST=existing ash_unittests, chrome unit_tests and browser_tests
TBR=sky@chromium.org for mechanical changes to //chrome/browser

Committed: https://crrev.com/443e42d71ea1b9836c434c661cb2cd2517d413d4
Cr-Commit-Position: refs/heads/master@{#399277}
==========

[commit-bot: I haz the power, 2016-06-10 21:46:24]

Patchset 2 (id:??) landed as
https://crrev.com/443e42d71ea1b9836c434c661cb2cd2517d413d4
Cr-Commit-Position: refs/heads/master@{#399277}