Delete "shutdown without closing browsers" path

Delete "shutdown without closing browsers"

Removing unused ShellDelegate::Shutdown()

Added new PreShutdown to safely cleanup DisplayObserver.

Chrome no longer access X during shutdown (except for deleting X window, which is safe), so normal SessionEnd
path should work.

There is another issue when this happens during login screen, and I'll file a separate bug for it.

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

R=abodenha@chromium.org, thestig@chromium.org

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

[oshima, 2014-03-22 17:39:07]

https://codereview.chromium.org/205963005/diff/50001/ash/shell_delegate.h
File ash/shell_delegate.h (left):

https://codereview.chromium.org/205963005/diff/50001/ash/shell_delegate.h#old...
ash/shell_delegate.h:80: virtual void Shutdown() = 0;
This method was obsolete.

[Albert Bodenhamer, 2014-03-23 01:38:35]

A lot of this CL is replacing Shutdown with PreShutdown, but that isn't
mentioned in the CL description.

https://codereview.chromium.org/205963005/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/205963005/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:998: NOTREACHED();
It doesn't make sense to have a NOTREACHED() followed by the cleanup code.

[oshima, 2014-03-23 04:07:35]

It wasn't replace. I added comment but sorry if it wasn't clear.

I simply removed obsolete Shtudown() method (it's not used), and added new
method PreShutdown to cleanup observer. I updated the description.

https://codereview.chromium.org/205963005/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/205963005/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:998: NOTREACHED();
On 2014/03/23 01:38:36, Albert Bodenhamer wrote:
> It doesn't make sense to have a NOTREACHED() followed by the cleanup code.

This method should not be called in normal path, and NOTREACHED was always valid
when "ShuttingDownWithoutClosingBrowesrs()" is false,
as you can see.

Since I'm removing the special path, NOTREACHED should remain and
the following code is a fallback path for release build.

[Albert Bodenhamer, 2014-03-24 02:22:41]

lgtm

https://codereview.chromium.org/205963005/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/205963005/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:998: NOTREACHED();
On 2014/03/23 04:07:35, oshima wrote:
> On 2014/03/23 01:38:36, Albert Bodenhamer wrote:
> > It doesn't make sense to have a NOTREACHED() followed by the cleanup code.
> 
> This method should not be called in normal path, and NOTREACHED was always
valid
> when "ShuttingDownWithoutClosingBrowesrs()" is false,
> as you can see.
> 
> Since I'm removing the special path, NOTREACHED should remain and
> the following code is a fallback path for release build.
> 
Isn't NOTREACHED basically a DCHECK? I thought the normal rule was not to handle
DCHECK failure cases. Either calling this code is an error (NOTREACHED) or we
should treat this as a rare, but valid, cleanup case.

I'm not going to hold up the CL any more over it. Just want to understand the
motivation.

[Albert Bodenhamer, 2014-03-24 02:22:50]

The CQ bit was checked by abodenha@chromium.org

[commit-bot: I haz the power, 2014-03-24 02:22:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/205963005/50001

[oshima, 2014-03-24 02:30:05]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-03-24 02:31:50]

On 2014/03/24 02:22:41, Albert Bodenhamer wrote:
> lgtm
> 
>
https://codereview.chromium.org/205963005/diff/50001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):
> 
>
https://codereview.chromium.org/205963005/diff/50001/chrome/browser/ui/views/...
> chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:998: NOTREACHED();
> On 2014/03/23 04:07:35, oshima wrote:
> > On 2014/03/23 01:38:36, Albert Bodenhamer wrote:
> > > It doesn't make sense to have a NOTREACHED() followed by the cleanup code.
> > 
> > This method should not be called in normal path, and NOTREACHED was always
> valid
> > when "ShuttingDownWithoutClosingBrowesrs()" is false,
> > as you can see.
> > 
> > Since I'm removing the special path, NOTREACHED should remain and
> > the following code is a fallback path for release build.
> > 
> Isn't NOTREACHED basically a DCHECK? I thought the normal rule was not to
handle
> DCHECK failure cases. Either calling this code is an error (NOTREACHED) or we
> should treat this as a rare, but valid, cleanup case.
> 
> I'm not going to hold up the CL any more over it. Just want to understand the
> motivation.

NOTREACHED is DCHECK in debug build but LOG(ERROR) in release build.
I understand it's a big confusing, but I didn't want to change that behavior.

I need LGTMs from other owners and will land this on monday.

[oshima, 2014-03-24 02:45:56]

On 2014/03/24 02:31:50, oshima wrote:
> On 2014/03/24 02:22:41, Albert Bodenhamer wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/205963005/diff/50001/chrome/browser/ui/views/...
> > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/205963005/diff/50001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:998: NOTREACHED();
> > On 2014/03/23 04:07:35, oshima wrote:
> > > On 2014/03/23 01:38:36, Albert Bodenhamer wrote:
> > > > It doesn't make sense to have a NOTREACHED() followed by the cleanup
code.
> > > 
> > > This method should not be called in normal path, and NOTREACHED was always
> > valid
> > > when "ShuttingDownWithoutClosingBrowesrs()" is false,
> > > as you can see.
> > > 
> > > Since I'm removing the special path, NOTREACHED should remain and
> > > the following code is a fallback path for release build.
> > > 
> > Isn't NOTREACHED basically a DCHECK? I thought the normal rule was not to
> handle
> > DCHECK failure cases. Either calling this code is an error (NOTREACHED) or
we
> > should treat this as a rare, but valid, cleanup case.
> > 
> > I'm not going to hold up the CL any more over it. Just want to understand
the
> > motivation.
> 
> NOTREACHED is DCHECK in debug build but LOG(ERROR) in release build.
> I understand it's a big confusing, but I didn't want to change that behavior.

I meant s/big/bit :)

> 
> I need LGTMs from other owners and will land this on monday.

[oshima, 2014-03-24 05:38:51]

thestig -> chrome/browser OWNER

[Lei Zhang, 2014-03-24 20:13:14]

lgtm stamp

You can probably remove some chrome/browser/browser_shutdown.h #includes

[oshima, 2014-03-24 20:53:32]

On 2014/03/24 20:13:14, Lei Zhang wrote:
> lgtm stamp
> 
> You can probably remove some chrome/browser/browser_shutdown.h #includes

done.

[oshima, 2014-03-24 20:54:01]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-24 20:54:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/205963005/60001

[commit-bot: I haz the power, 2014-03-25 00:49:36]

Change committed as 259061

[michaeln, 2014-03-25 01:38:52]

A revert of this CL has been created in
https://codereview.chromium.org/210673002/ by michaeln@chromium.org.

The reason for reverting is: Didn't compile on Linux GTK builder.

http://build.chromium.org/p/chromium.linux/builders/Linux%20GTK%20Builder/bui...

../../chrome/browser/ui/gtk/bookmarks/bookmark_bar_gtk.cc: In member function
'virtual void BookmarkBarGtk::BookmarkModelBeingDeleted(BookmarkModel*)':
../../chrome/browser/ui/gtk/bookmarks/bookmark_bar_gtk.cc:896:8:error:
'ShuttingDownWithoutClosingBrowsers' is not a member of 'browser_shutdown'.

[oshima, 2014-03-25 14:02:37]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-25 14:02:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/205963005/130001

[oshima, 2014-03-25 15:29:20]

Committed patchset #3 manually as r259203 (presubmit successful).