Observe chrome::NOTIFICATION_APP_TERMINATING in Chrome's AppListViewDelegate

Observe chrome::NOTIFICATION_APP_TERMINATING in Chrome's AppListViewDelegate

This allows custom launcher pages to be torn down in response to this
notification in the same way that regular packaged app windows are.
Currently these WebContents are torn down later, by
Widget::CloseAllSecondaryWidgets(), but this relies on the WebContents
lifetime being tied to a Widget.

Observing NOTIFICATION_APP_TERMINATING also allows us to explore
decoupling the AppListViewDelegate from the AppListView lifetime, and
remove some special code on Mac to deal with updating the profile
switcher (since the AppListViewDelegate is owned by a leaky singleton
there).

BUG=403647
Committed: https://crrev.com/ffbd8b951add65d1d610b25df7611861fd133cb2
Cr-Commit-Position: refs/heads/master@{#295194}

[tapted, 2014-09-10 06:18:29]

tapted@chromium.org changed reviewers:
+ mgiuca@chromium.org

[tapted, 2014-09-10 06:52:14]

Hi Matt - please take a look. For a bit more context, this is split off from the
bigger patch at crrev/508813002

[Matt Giuca, 2014-09-11 03:50:37]

lgtm

https://codereview.chromium.org/559823002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/559823002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:205: if (profile_ ==
new_profile)
Are these profile checks (this one, and the next two) related to the main change
here (notification observing and proper shutdown)? They seem unrelated.

Do they change any behaviour (like fixing any crashes)? If so, I would put them
up in a separate CL. If not, then it's fine.

https://codereview.chromium.org/559823002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:654: // singleton like
this, so will get offended without the following.
Cute :) But can you be clear instead of saying "get offended".

suggest: "SigninManagerFactory is not a leaky singleton (unlike this class), and
its destructor will check that it has no remaining observers."

[tapted, 2014-09-11 04:57:45]

https://codereview.chromium.org/559823002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/559823002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:205: if (profile_ ==
new_profile)
On 2014/09/11 03:50:37, Matt Giuca wrote:
> Are these profile checks (this one, and the next two) related to the main
change
> here (notification observing and proper shutdown)? They seem unrelated.
> 
> Do they change any behaviour (like fixing any crashes)? If so, I would put
them
> up in a separate CL. If not, then it's fine.

They're needed, but only due to this change. Previously SetProfile(NULL) was
only called from the destructor, so nothing bad could result. It's still called
from there on platforms where it is not leaky whenever the window is closed (and
it may or may not already be NULL), and things like profile destruction and
widget teardown happen after SetProfile(NULL) is [now] called in Observe().

https://codereview.chromium.org/559823002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:654: // singleton like
this, so will get offended without the following.
On 2014/09/11 03:50:37, Matt Giuca wrote:
> Cute :) But can you be clear instead of saying "get offended".
> 
> suggest: "SigninManagerFactory is not a leaky singleton (unlike this class),
and
> its destructor will check that it has no remaining observers."

Done.

[tapted, 2014-09-11 04:58:31]

tapted@chromium.org changed reviewers:
+ rogerta@chromium.org

[tapted, 2014-09-11 04:58:31]

+rogerta please review for OWNERS in chrome/browser/signin

[Matt Giuca, 2014-09-11 08:17:58]

https://codereview.chromium.org/559823002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/559823002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:205: if (profile_ ==
new_profile)
On 2014/09/11 04:57:45, tapted wrote:
> On 2014/09/11 03:50:37, Matt Giuca wrote:
> > Are these profile checks (this one, and the next two) related to the main
> change
> > here (notification observing and proper shutdown)? They seem unrelated.
> > 
> > Do they change any behaviour (like fixing any crashes)? If so, I would put
> them
> > up in a separate CL. If not, then it's fine.
> 
> They're needed, but only due to this change. Previously SetProfile(NULL) was
> only called from the destructor, so nothing bad could result. It's still
called
> from there on platforms where it is not leaky whenever the window is closed
(and
> it may or may not already be NULL), and things like profile destruction and
> widget teardown happen after SetProfile(NULL) is [now] called in Observe().

Acknowledged.

[tapted, 2014-09-11 23:30:43]

tapted@chromium.org changed reviewers:
+ tim@chromium.org
- rogerta@chromium.org

[tapted, 2014-09-11 23:30:44]

+tim - could you please take a look for OWNERS in chrome/browser/signin

(roger is OOO for a few more days)

[tapted, 2014-09-15 22:19:55]

rogerta (since you're back :), or timsteele: ping?

[Roger Tawa OOO till Jul 10th, 2014-09-16 23:04:23]

rogerta@chromium.org changed reviewers:
+ rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-09-16 23:04:24]

lgtm for chrome/browser/signin

[tapted, 2014-09-16 23:05:58]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-09-16 23:07:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/559823002/60001

[commit-bot: I haz the power, 2014-09-17 00:49:00]

Committed patchset #4 (id:60001) as d234f94573ad6d39ef0aecb92c6387b32422ba0c

[commit-bot: I haz the power, 2014-09-17 00:50:09]

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