Makes BrowserStatusMonitor remove observers in destructor

Makes BrowserStatusMonitor remove observers in destructor

I'm seeing a crash where we have a dangling TabStripModelObserver,
this is one possible place. At the same time I'm making the class only
look for HOST_DESKTOP_TYPE_ASH browsers.

BUG=286162
TEST=none
R=skuhne@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222124

[Mr4D (OOO till 08-26), 2013-09-09 18:48:45]

lgtm with two nits

https://codereview.chromium.org/23455037/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23455037/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:152: return;
Maybe an early exit if !browser since it gets checked twice here?

https://codereview.chromium.org/23455037/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:203: if (browser &&
If browser is null it would crash later on as well. I would guess that a DCHECK
instead might be a good idea?

[sky, 2013-09-09 19:29:28]

https://codereview.chromium.org/23455037/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23455037/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:152: return;
On 2013/09/09 18:48:45, Mr4D wrote:
> Maybe an early exit if !browser since it gets checked twice here?

I thought we still wanted to invoke UpdateAppAndBrowserState in this case. Maybe
I'm wrong?

https://codereview.chromium.org/23455037/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:203: if (browser &&
On 2013/09/09 18:48:45, Mr4D wrote:
> If browser is null it would crash later on as well. I would guess that a
DCHECK
> instead might be a good idea?

Good point. Added DCHECK.

[Mr4D (OOO till 08-26), 2013-09-09 19:38:28]

https://codereview.chromium.org/23455037/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23455037/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:152: return;
I've meant early exit, but calling UpdateApp.. .

So, yes we want to call. It only felt odd that we are testing twice for browser
and then an additional condition. So testing once and then adding the closure
for the two tests does of course do the same thing, but I got several times the
request to early out, so I suggested the same :).

[sky, 2013-09-09 19:39:54]

https://codereview.chromium.org/23455037/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23455037/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:152: return;
On 2013/09/09 19:38:28, Mr4D wrote:
> I've meant early exit, but calling UpdateApp.. .
> 
> So, yes we want to call. It only felt odd that we are testing twice for
browser
> and then an additional condition. So testing once and then adding the closure
> for the two tests does of course do the same thing, but I got several times
the
> request to early out, so I suggested the same :).

I'm going to leave it. If you feel strongly I'll update.

[commit-bot: I haz the power, 2013-09-09 19:40:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/23455037/8001

[commit-bot: I haz the power, 2013-09-09 23:17:55]

Change committed as 222124