mash: Move ownership of ShelfDelegate to WmShell

mash: Move ownership of ShelfDelegate to WmShell

This removes some ash::Shell access in the shelf, which we need to eliminate
to port to mus.

* Move ShelfDelegate to //ash/common
* Move ownership to WmShell
* Move ShelfDelegate creation slightly earlier by doing it on Shelf creation,
rather than lazily on access. It can't happen in WmShell::Initialize() because
Chrome's implementation depends on the user being logged in.

TODO: Fix ash::ShelfWindowWatcher ownership and creation time.

BUG=629244
TEST=ash_unittests, chrome unit_tests and browser_tests
TBR=rockot@chromium.org

Committed: https://crrev.com/ff5220bf49573c720e26bfa025011fa198038f27
Cr-Commit-Position: refs/heads/master@{#407497}

[James Cook, 2016-07-22 21:27:08]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 21:27:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-07-22 21:41:38]

Description was changed from

==========
mash: Move ownership of ShelfDelegate to WmShell

move ownership to WmShell


hackery in shell, move ShelfDelegate to //ash/common

BUG=
==========

to

==========
mash: Move ownership of ShelfDelegate to WmShell

This removes some ash::Shell access in the shelf, which we need to eliminate
to port to mus.

* Move ShelfDelegate to //ash/common
* Move ownership to WmShell
* Move ShelfDelegate creation slightly earlier by doing it on Shelf creation,
rather than lazily on access. It can't happen in WmShell::Initialize() because
Chrome's implementation depends on the user being logged in.

TODO: Fix ash::ShelfWindowWatcher ownership and creation time.

BUG=629244
TEST=ash_unittests, chrome unit_tests and browser_tests
==========

[James Cook, 2016-07-22 21:42:37]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 21:43:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 22:44:15]

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

[commit-bot: I haz the power, 2016-07-22 22:44:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2016-07-22 23:13:14]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 23:13:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-07-22 23:37:00]

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

[James Cook, 2016-07-22 23:37:01]

msw, please take a look at everything

stevenjb, please take a look at chrome/browser/chromeos

Thanks.

https://codereview.chromium.org/2177663002/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
(right):

https://codereview.chromium.org/2177663002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc:121:
ash::Shell::GetInstance()->CreateShelf();
I went with manually creating the shelf, rather than calling something upstream
like ash::Shell::OnLoginUserProfilePrepared(), because it's the only thing this
test needs and we do something similar in other test code:
https://cs-staging.chromium.org/chromium/src/ash/test/test_session_state_dele...

[commit-bot: I haz the power, 2016-07-22 23:42:24]

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

[commit-bot: I haz the power, 2016-07-22 23:42:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2016-07-22 23:59:03]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 23:59:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2016-07-23 00:02:37]

Sorry I keep asking 'why'...

https://codereview.chromium.org/2177663002/diff/40001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2177663002/diff/40001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:206: void WmShell::CreateShelfDelegate() {
Could we just do this once in the ctor?

https://codereview.chromium.org/2177663002/diff/40001/ash/metrics/user_metric...
File ash/metrics/user_metrics_recorder.cc (right):

https://codereview.chromium.org/2177663002/diff/40001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.cc:160: ShelfDelegate* shelf_delegate =
WmShell::Get()->shelf_delegate();
Couldn't this now be null if no shelves have called CreateShelfDelegate? The old
accessor would lazily create the shelf delegate if needed. ditto elsewhere

https://codereview.chromium.org/2177663002/diff/40001/ash/shelf/shelf.cc
File ash/shelf/shelf.cc (right):

https://codereview.chromium.org/2177663002/diff/40001/ash/shelf/shelf.cc#newc...
ash/shelf/shelf.cc:45: : delegate_(shelf_delegate),
optional nit: remove this argument and member (and test api setters), and just
get the delegate from the shell in the few places its actually needed below?

https://codereview.chromium.org/2177663002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2177663002/diff/40001/ash/shell.cc#newcode375
ash/shell.cc:375: // Must occur after SessionStateDelegate creation and user
login.
Why? Move these checks into CreateShelfDelegate; closer to the need?

https://codereview.chromium.org/2177663002/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
(right):

https://codereview.chromium.org/2177663002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc:121:
ash::Shell::GetInstance()->CreateShelf();
On 2016/07/22 23:37:00, James Cook wrote:
> I went with manually creating the shelf, rather than calling something
upstream
> like ash::Shell::OnLoginUserProfilePrepared(), because it's the only thing
this
> test needs and we do something similar in other test code:
>
https://cs-staging.chromium.org/chromium/src/ash/test/test_session_state_dele...

Would WmShell::SetShelfDelegateForTesting suffice instead? Do it in SetUp?

[James Cook, 2016-07-23 00:35:16]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-23 00:35:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-07-23 00:38:03]

msw, please take another look.

Given that you can't actually create the ShelfDelegate at an arbitrary time, do
you think it would be clearer to go back to lazy initialization, or do you think
it's clearer to explicitly initialize it and have the getter potentially be null
sometimes?

(With lazy init the code will still crash if you try to call GetShelfDelegate
before login, just in the construction of Chrome's delegate impl, not with a
nullptr deref at the call site.)

https://codereview.chromium.org/2177663002/diff/40001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2177663002/diff/40001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:206: void WmShell::CreateShelfDelegate() {
On 2016/07/23 00:02:36, msw wrote:
> Could we just do this once in the ctor?

No. It must be done after login. Moved the DCHECKS from ash::Shell to here to
make that more clear.

https://codereview.chromium.org/2177663002/diff/40001/ash/metrics/user_metric...
File ash/metrics/user_metrics_recorder.cc (right):

https://codereview.chromium.org/2177663002/diff/40001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.cc:160: ShelfDelegate* shelf_delegate =
WmShell::Get()->shelf_delegate();
On 2016/07/23 00:02:36, msw wrote:
> Couldn't this now be null if no shelves have called CreateShelfDelegate? The
old
> accessor would lazily create the shelf delegate if needed. ditto elsewhere

Added DCHECKs.

I thought the lazy construction before was confusing. I could not tell by
reading the code when the object was created. If you think it's better to lazily
create the delegate then I'm happy to move it into the getter.

https://codereview.chromium.org/2177663002/diff/40001/ash/shelf/shelf.cc
File ash/shelf/shelf.cc (right):

https://codereview.chromium.org/2177663002/diff/40001/ash/shelf/shelf.cc#newc...
ash/shelf/shelf.cc:45: : delegate_(shelf_delegate),
On 2016/07/23 00:02:36, msw wrote:
> optional nit: remove this argument and member (and test api setters), and just
> get the delegate from the shell in the few places its actually needed below?

Ordinarily I prefer dependency injection, but given that tests have to do extra
work to reset the pointer, switched to WmShell access.

I didn't change ShelfView, though.

https://codereview.chromium.org/2177663002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2177663002/diff/40001/ash/shell.cc#newcode375
ash/shell.cc:375: // Must occur after SessionStateDelegate creation and user
login.
On 2016/07/23 00:02:37, msw wrote:
> Why? Move these checks into CreateShelfDelegate; closer to the need?

Done and commented more on why.

https://codereview.chromium.org/2177663002/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
(right):

https://codereview.chromium.org/2177663002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc:121:
ash::Shell::GetInstance()->CreateShelf();
On 2016/07/23 00:02:37, msw wrote:
> On 2016/07/22 23:37:00, James Cook wrote:
> > I went with manually creating the shelf, rather than calling something
> upstream
> > like ash::Shell::OnLoginUserProfilePrepared(), because it's the only thing
> this
> > test needs and we do something similar in other test code:
> >
>
https://cs-staging.chromium.org/chromium/src/ash/test/test_session_state_dele...
> 
> Would WmShell::SetShelfDelegateForTesting suffice instead? Do it in SetUp?

Done.

[msw, 2016-07-23 00:54:09]

I guess explicit init and potential for null is clearer; but it'd be nicest if
we could just create it in WmShell's ctor and know it's never null. lgtm

Dependency injection might be nicer; I don't have a strong opinion.

[commit-bot: I haz the power, 2016-07-23 01:08:33]

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

[commit-bot: I haz the power, 2016-07-23 01:08:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2016-07-23 17:24:01]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-23 17:24:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-23 18:05:51]

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

[commit-bot: I haz the power, 2016-07-23 18:05:52]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-07-25 15:59:19]

lgtm

[James Cook, 2016-07-25 16:20:17]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-07-25 16:20:17]

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/2177663002/#ps120001
(title: "fix wallpaper tests again")

[commit-bot: I haz the power, 2016-07-25 16:20:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 16:26:28]

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

[commit-bot: I haz the power, 2016-07-25 16:26:30]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[James Cook, 2016-07-25 16:45:27]

Description was changed from

==========
mash: Move ownership of ShelfDelegate to WmShell

This removes some ash::Shell access in the shelf, which we need to eliminate
to port to mus.

* Move ShelfDelegate to //ash/common
* Move ownership to WmShell
* Move ShelfDelegate creation slightly earlier by doing it on Shelf creation,
rather than lazily on access. It can't happen in WmShell::Initialize() because
Chrome's implementation depends on the user being logged in.

TODO: Fix ash::ShelfWindowWatcher ownership and creation time.

BUG=629244
TEST=ash_unittests, chrome unit_tests and browser_tests
==========

to

==========
mash: Move ownership of ShelfDelegate to WmShell

This removes some ash::Shell access in the shelf, which we need to eliminate
to port to mus.

* Move ShelfDelegate to //ash/common
* Move ownership to WmShell
* Move ShelfDelegate creation slightly earlier by doing it on Shelf creation,
rather than lazily on access. It can't happen in WmShell::Initialize() because
Chrome's implementation depends on the user being logged in.

TODO: Fix ash::ShelfWindowWatcher ownership and creation time.

BUG=629244
TEST=ash_unittests, chrome unit_tests and browser_tests
TBR=rockot@chromium.org
==========

[James Cook, 2016-07-25 16:45:27]

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

[James Cook, 2016-07-25 16:45:50]

TBR rockot for mechanical change to
chrome/browser/extensions/bookmark_app_helper.cc

[James Cook, 2016-07-25 16:45:59]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-07-25 16:46:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 17:08:11]

Description was changed from

==========
mash: Move ownership of ShelfDelegate to WmShell

This removes some ash::Shell access in the shelf, which we need to eliminate
to port to mus.

* Move ShelfDelegate to //ash/common
* Move ownership to WmShell
* Move ShelfDelegate creation slightly earlier by doing it on Shelf creation,
rather than lazily on access. It can't happen in WmShell::Initialize() because
Chrome's implementation depends on the user being logged in.

TODO: Fix ash::ShelfWindowWatcher ownership and creation time.

BUG=629244
TEST=ash_unittests, chrome unit_tests and browser_tests
TBR=rockot@chromium.org
==========

to

==========
mash: Move ownership of ShelfDelegate to WmShell

This removes some ash::Shell access in the shelf, which we need to eliminate
to port to mus.

* Move ShelfDelegate to //ash/common
* Move ownership to WmShell
* Move ShelfDelegate creation slightly earlier by doing it on Shelf creation,
rather than lazily on access. It can't happen in WmShell::Initialize() because
Chrome's implementation depends on the user being logged in.

TODO: Fix ash::ShelfWindowWatcher ownership and creation time.

BUG=629244
TEST=ash_unittests, chrome unit_tests and browser_tests
TBR=rockot@chromium.org
==========

[commit-bot: I haz the power, 2016-07-25 17:08:12]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-25 17:09:50]

Description was changed from

==========
mash: Move ownership of ShelfDelegate to WmShell

This removes some ash::Shell access in the shelf, which we need to eliminate
to port to mus.

* Move ShelfDelegate to //ash/common
* Move ownership to WmShell
* Move ShelfDelegate creation slightly earlier by doing it on Shelf creation,
rather than lazily on access. It can't happen in WmShell::Initialize() because
Chrome's implementation depends on the user being logged in.

TODO: Fix ash::ShelfWindowWatcher ownership and creation time.

BUG=629244
TEST=ash_unittests, chrome unit_tests and browser_tests
TBR=rockot@chromium.org
==========

to

==========
mash: Move ownership of ShelfDelegate to WmShell

This removes some ash::Shell access in the shelf, which we need to eliminate
to port to mus.

* Move ShelfDelegate to //ash/common
* Move ownership to WmShell
* Move ShelfDelegate creation slightly earlier by doing it on Shelf creation,
rather than lazily on access. It can't happen in WmShell::Initialize() because
Chrome's implementation depends on the user being logged in.

TODO: Fix ash::ShelfWindowWatcher ownership and creation time.

BUG=629244
TEST=ash_unittests, chrome unit_tests and browser_tests
TBR=rockot@chromium.org

Committed: https://crrev.com/ff5220bf49573c720e26bfa025011fa198038f27
Cr-Commit-Position: refs/heads/master@{#407497}
==========

[commit-bot: I haz the power, 2016-07-25 17:09:52]

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