ash: Reorder shutdown of ShelfLayoutManager and StatusAreaWidget

ash: Reorder shutdown of ShelfLayoutManager and StatusAreaWidget

Status area widget teardown can cause other widgets to close (e.g. bubbles)
which can cause synthetic events (e.g. ET_MOUSE_EXITED) to be generated.

In a future CL I need to make ShelfLayoutManager::AutoHideEventFilter handle
ET_MOUSE_EXITED, so I need to shutdown the ShelfLayoutManager earlier during
Shell teardown.

BUG=613212
TEST=existing ash_unittests, specifically ShelfLayoutManagerTest.*

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

[James Cook, 2016-05-24 04:32:12]

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

[James Cook, 2016-05-24 04:32:12]

oshima, please take a look. The shelf teardown code is frighteningly complex,
but I think this is safe. I've got ASAN and MSAN try bots running too.

I did this in a separate CL to make it easier to identify in case I introduce a
problem, rather than mixing it with the dependent mash CL.

[oshima, 2016-05-24 20:49:10]

https://codereview.chromium.org/2000393003/diff/20001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2000393003/diff/20001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:266: in_shutdown_ = true;
did you actually need this? resetting/removing observer seems to work just fine.

https://codereview.chromium.org/2000393003/diff/20001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/2000393003/diff/20001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:758: shelf_layout_manager_->PrepareForShutdown();
can you also add comment why we shouldn't remove the shelf layout manager's
observer here?

[James Cook, 2016-05-24 21:18:14]

oshima, please take another look

https://codereview.chromium.org/2000393003/diff/20001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2000393003/diff/20001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:266: in_shutdown_ = true;
On 2016/05/24 20:49:10, oshima wrote:
> did you actually need this? resetting/removing observer seems to work just
fine.

I will need the in_shutdown_ variable in a future CL, but I don't need the if().
I've removed both and I will add back the variable in the next CL.

https://codereview.chromium.org/2000393003/diff/20001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/2000393003/diff/20001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:758: shelf_layout_manager_->PrepareForShutdown();
On 2016/05/24 20:49:10, oshima wrote:
> can you also add comment why we shouldn't remove the shelf layout manager's
> observer here?

Good catch! Actually, if we reset the shelf_layout_manager_ pointer here we
don't need to be a ShelfLayoutManagerObserver at all!

[James Cook, 2016-05-25 00:42:09]

Actually, please wait on review. I'm seeing browser_tests crashes on the bots,
so I may have to put back the observer I removed. <sigh>

[James Cook, 2016-05-25 16:10:40]

oshima, please take another look.

I had to go back to having ShelfWidget be an observer of ShelfLayoutManager. It
turns out the app list widget still accesses the ShelfLayoutManager during
shutdown in browser_tests (and probably in production code). While I could fix
that one widget, and the tests would pass, I'm afraid there might be other
widgets with the same problem that don't have good test coverage.

I expanded the comment and renamed WillDeleteShelf() to
WillDeleteShelfLayoutManager() to make it more clear.

Shutdown is hard. :-(

[oshima, 2016-05-25 19:13:48]

lgtm

thank you for trying and looking into this.

[James Cook, 2016-05-25 19:35:37]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-05-25 19:35:37]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2000393003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-25 19:35:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000393003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000393003/100001

[commit-bot: I haz the power, 2016-05-25 21:22:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-25 21:23:31]

Description was changed from

==========
ash: Reorder shutdown of ShelfLayoutManager and StatusAreaWidget

Status area widget teardown can cause other widgets to close (e.g. bubbles)
which can cause synthetic events (e.g. ET_MOUSE_EXITED) to be generated.

In a future CL I need to make ShelfLayoutManager::AutoHideEventFilter handle
ET_MOUSE_EXITED, so I need to shutdown the ShelfLayoutManager earlier during
Shell teardown.

BUG=613212
TEST=existing ash_unittests, specifically ShelfLayoutManagerTest.*
==========

to

==========
ash: Reorder shutdown of ShelfLayoutManager and StatusAreaWidget

Status area widget teardown can cause other widgets to close (e.g. bubbles)
which can cause synthetic events (e.g. ET_MOUSE_EXITED) to be generated.

In a future CL I need to make ShelfLayoutManager::AutoHideEventFilter handle
ET_MOUSE_EXITED, so I need to shutdown the ShelfLayoutManager earlier during
Shell teardown.

BUG=613212
TEST=existing ash_unittests, specifically ShelfLayoutManagerTest.*

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

[commit-bot: I haz the power, 2016-05-25 21:23:32]

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