chromeos: Notify window manager earlier about signout.

chromeos: Notify window manager earlier about signout.

We were previously doing this pretty late in Chrome's
shutdown process, which resulted in the closing of
individual browser windows often being visible onscreen.

This change makes us send the message before closing any
windows (at least in the
no-beforeunload-handlers-or-downloads case) and flushes the
Xlib request queue to make sure that the message won't sit
around until the next time that we process events.

Note that there are still cases where beforeunload handlers
aren't registered but we still won't close the window
without warning and hence still show the flip through the
windows.  See RenderViewHost::sudden_termination_allowed_.

BUG=chromium-os:9915
TEST=opened a bunch of windows and checked that we fade to black and hide the cursor immediately on ctrl-shift-q and wrench->sign out.  also tested that we don't fade out until the dialog is closed with an affirmative response when http://goo.gl/gzMdN is open

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72014

[Chris Masone, 2011-01-14 03:03:50]

Someone recently landed a change that blocks signout if pages (like gmail)
have some modal UI up...I don't know how they did that, but does this change
play nicely with that?  If you have an unsaved draft in gmail, do we wipe
the screen but then block signout and wait for user input that will never
come?

On Thu, Jan 13, 2011 at 6:49 PM, <derat@chromium.org> wrote:

> Reviewers: Chris Masone,
>
> Description:
> chromeos: Notify window manager earlier about signout.
>
> We were previously doing this pretty late in Chrome's
> shutdown process, which resulted in the closing of
> individual browser windows often being visible onscreen.
>
> This change makes use send the message as soon as we've
> recorded the "shutting down" bootstat, and flushes the Xlib
> request queue to make sure that the message won't sit around
> until the next time that we process events.
>
> BUG=chromium-os:9915
> TEST=opened a bunch of windows and checked that we fade to black and hide
> the
> cursor immediately on ctrl-shift-q and wrench->sign out
>
> Please review this at http://codereview.chromium.org/6353001/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/chrome
>
> Affected files:
>  M chrome/browser/browser_shutdown.cc
>  M chrome/browser/chromeos/wm_ipc.cc
>  M chrome/browser/ui/browser.cc
>  M chrome/browser/ui/browser_list.cc
>
>
> Index: chrome/browser/browser_shutdown.cc
> diff --git a/chrome/browser/browser_shutdown.cc
> b/chrome/browser/browser_shutdown.cc
> index
>
72c035023e05ecc72a3ef94977903363a20481c3..43088d5d5320f90e43f009689f33b6f230ea2cb4
> 100644
> --- a/chrome/browser/browser_shutdown.cc
> +++ b/chrome/browser/browser_shutdown.cc
> @@ -48,7 +48,6 @@
>  #include "chrome/browser/chromeos/boot_times_loader.h"
>  #include "chrome/browser/chromeos/cros/cros_library.h"
>  #include "chrome/browser/chromeos/cros/login_library.h"
> -#include "chrome/browser/chromeos/wm_ipc.h"
>  #endif
>
>  using base::Time;
> @@ -252,7 +251,6 @@ void Shutdown() {
>   UnregisterURLRequestChromeJob();
>
>  #if defined(OS_CHROMEOS)
> -  chromeos::WmIpc::instance()->NotifyAboutSignout();
>   if (chromeos::CrosLibrary::Get()->EnsureLoaded()) {
>     chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
>   }
> Index: chrome/browser/chromeos/wm_ipc.cc
> diff --git a/chrome/browser/chromeos/wm_ipc.cc
> b/chrome/browser/chromeos/wm_ipc.cc
> index
>
3e856f34d54fededc2fda6d32817b7ca3a673e5b..e8f8b4eb4908c5225d7f65f6f7cd8e3d27fcadc1
> 100644
> --- a/chrome/browser/chromeos/wm_ipc.cc
> +++ b/chrome/browser/chromeos/wm_ipc.cc
> @@ -166,6 +166,7 @@ void WmIpc::SetLoggedInProperty(bool logged_in) {
>  void WmIpc::NotifyAboutSignout() {
>   Message msg(chromeos::WM_IPC_MESSAGE_WM_NOTIFY_SIGNING_OUT);
>   SendMessage(msg);
> +  XFlush(x11_util::GetXDisplay());
>  }
>
>  WmIpc::WmIpc() {
> Index: chrome/browser/ui/browser.cc
> diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
> index
>
8334180266497a88933aefcad496ca1c56b3972d..2aae24df074a453b92be91f9c2bcc50bb54737d1
> 100644
> --- a/chrome/browser/ui/browser.cc
> +++ b/chrome/browser/ui/browser.cc
> @@ -131,6 +131,7 @@
>  #if defined(OS_CHROMEOS)
>  #include "chrome/browser/chromeos/boot_times_loader.h"
>  #include "chrome/browser/chromeos/options/language_config_view.h"
> +#include "chrome/browser/chromeos/wm_ipc.h"
>  #endif
>
>  using base::TimeDelta;
> @@ -1473,6 +1474,10 @@ void Browser::Exit() {
>   // Write /tmp/uptime-logout-started as well.
>   const char kLogoutStarted[] = "logout-started";
>   chromeos::BootTimesLoader::Get()->RecordCurrentStats(kLogoutStarted);
> +
> +  // Let the window manager know that we're going away before we start
> closing
> +  // windows so it can display a graceful transition to a black screen.
> +  chromeos::WmIpc::instance()->NotifyAboutSignout();
>  #endif
>   BrowserList::Exit();
>  }
> Index: chrome/browser/ui/browser_list.cc
> diff --git a/chrome/browser/ui/browser_list.cc
> b/chrome/browser/ui/browser_list.cc
> index
>
ec5fef1cc27a11fa0ebba44072e4c641a505a5f6..27a599ca557d3a1d95d29b2f8d1c3b861182373e
> 100644
> --- a/chrome/browser/ui/browser_list.cc
> +++ b/chrome/browser/ui/browser_list.cc
> @@ -190,7 +190,6 @@ void BrowserList::NotifyAndTerminate() {
>                                          NotificationService::AllSources(),
>                                          NotificationService::NoDetails());
>  #if defined(OS_CHROMEOS)
> -  chromeos::WmIpc::instance()->NotifyAboutSignout();
>   if (chromeos::CrosLibrary::Get()->EnsureLoaded()) {
>     chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
>     return;
>
>
>

[Daniel Erat, 2011-01-14 04:11:06]

Glad to hear that we finally block on beforeunload.  I was initially
concerned about interaction with that but figured from the
LogoutStarted boot time recording that Browser::Exit() is a point of
no return.  You're right, though; BrowserList::Exit() (which
Browser::Exit calls) explicitly checks for !NeedBeforeUnloadFired()
and !PendingDownloads() before taking a faster path to shutdown
quickly (without closing all windows), so my change is wrong.  I'll
move the message-sending to the fast path.  In the slower path, it
looks like we just close the windows one-by-one (so any of them can
refuse to close if needed) and shut down once they're all gone.  I'll
try to find a good spot in the all-windows-are-gone path to send the
message as well.  Thanks for asking about this.

On Thu, Jan 13, 2011 at 7:03 PM, Chris Masone <cmasone@chromium.org> wrote:
> Someone recently landed a change that blocks signout if pages (like gmail)
> have some modal UI up...I don't know how they did that, but does this change
> play nicely with that?  If you have an unsaved draft in gmail, do we wipe
> the screen but then block signout and wait for user input that will never
> come?
>
> On Thu, Jan 13, 2011 at 6:49 PM, <derat@chromium.org> wrote:
>>
>> Reviewers: Chris Masone,
>>
>> Description:
>> chromeos: Notify window manager earlier about signout.
>>
>> We were previously doing this pretty late in Chrome's
>> shutdown process, which resulted in the closing of
>> individual browser windows often being visible onscreen.
>>
>> This change makes use send the message as soon as we've
>> recorded the "shutting down" bootstat, and flushes the Xlib
>> request queue to make sure that the message won't sit around
>> until the next time that we process events.
>>
>> BUG=chromium-os:9915
>> TEST=opened a bunch of windows and checked that we fade to black and hide
>> the
>> cursor immediately on ctrl-shift-q and wrench->sign out
>>
>> Please review this at http://codereview.chromium.org/6353001/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src/chrome
>>
>> Affected files:
>>  M chrome/browser/browser_shutdown.cc
>>  M chrome/browser/chromeos/wm_ipc.cc
>>  M chrome/browser/ui/browser.cc
>>  M chrome/browser/ui/browser_list.cc
>>
>>
>> Index: chrome/browser/browser_shutdown.cc
>> diff --git a/chrome/browser/browser_shutdown.cc
>> b/chrome/browser/browser_shutdown.cc
>> index
>>
72c035023e05ecc72a3ef94977903363a20481c3..43088d5d5320f90e43f009689f33b6f230ea2cb4
>> 100644
>> --- a/chrome/browser/browser_shutdown.cc
>> +++ b/chrome/browser/browser_shutdown.cc
>> @@ -48,7 +48,6 @@
>>  #include "chrome/browser/chromeos/boot_times_loader.h"
>>  #include "chrome/browser/chromeos/cros/cros_library.h"
>>  #include "chrome/browser/chromeos/cros/login_library.h"
>> -#include "chrome/browser/chromeos/wm_ipc.h"
>>  #endif
>>
>>  using base::Time;
>> @@ -252,7 +251,6 @@ void Shutdown() {
>>   UnregisterURLRequestChromeJob();
>>
>>  #if defined(OS_CHROMEOS)
>> -  chromeos::WmIpc::instance()->NotifyAboutSignout();
>>   if (chromeos::CrosLibrary::Get()->EnsureLoaded()) {
>>     chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
>>   }
>> Index: chrome/browser/chromeos/wm_ipc.cc
>> diff --git a/chrome/browser/chromeos/wm_ipc.cc
>> b/chrome/browser/chromeos/wm_ipc.cc
>> index
>>
3e856f34d54fededc2fda6d32817b7ca3a673e5b..e8f8b4eb4908c5225d7f65f6f7cd8e3d27fcadc1
>> 100644
>> --- a/chrome/browser/chromeos/wm_ipc.cc
>> +++ b/chrome/browser/chromeos/wm_ipc.cc
>> @@ -166,6 +166,7 @@ void WmIpc::SetLoggedInProperty(bool logged_in) {
>>  void WmIpc::NotifyAboutSignout() {
>>   Message msg(chromeos::WM_IPC_MESSAGE_WM_NOTIFY_SIGNING_OUT);
>>   SendMessage(msg);
>> +  XFlush(x11_util::GetXDisplay());
>>  }
>>
>>  WmIpc::WmIpc() {
>> Index: chrome/browser/ui/browser.cc
>> diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
>> index
>>
8334180266497a88933aefcad496ca1c56b3972d..2aae24df074a453b92be91f9c2bcc50bb54737d1
>> 100644
>> --- a/chrome/browser/ui/browser.cc
>> +++ b/chrome/browser/ui/browser.cc
>> @@ -131,6 +131,7 @@
>>  #if defined(OS_CHROMEOS)
>>  #include "chrome/browser/chromeos/boot_times_loader.h"
>>  #include "chrome/browser/chromeos/options/language_config_view.h"
>> +#include "chrome/browser/chromeos/wm_ipc.h"
>>  #endif
>>
>>  using base::TimeDelta;
>> @@ -1473,6 +1474,10 @@ void Browser::Exit() {
>>   // Write /tmp/uptime-logout-started as well.
>>   const char kLogoutStarted[] = "logout-started";
>>   chromeos::BootTimesLoader::Get()->RecordCurrentStats(kLogoutStarted);
>> +
>> +  // Let the window manager know that we're going away before we start
>> closing
>> +  // windows so it can display a graceful transition to a black screen.
>> +  chromeos::WmIpc::instance()->NotifyAboutSignout();
>>  #endif
>>   BrowserList::Exit();
>>  }
>> Index: chrome/browser/ui/browser_list.cc
>> diff --git a/chrome/browser/ui/browser_list.cc
>> b/chrome/browser/ui/browser_list.cc
>> index
>>
ec5fef1cc27a11fa0ebba44072e4c641a505a5f6..27a599ca557d3a1d95d29b2f8d1c3b861182373e
>> 100644
>> --- a/chrome/browser/ui/browser_list.cc
>> +++ b/chrome/browser/ui/browser_list.cc
>> @@ -190,7 +190,6 @@ void BrowserList::NotifyAndTerminate() {
>>
>>  NotificationService::AllSources(),
>>
>>  NotificationService::NoDetails());
>>  #if defined(OS_CHROMEOS)
>> -  chromeos::WmIpc::instance()->NotifyAboutSignout();
>>   if (chromeos::CrosLibrary::Get()->EnsureLoaded()) {
>>     chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
>>     return;
>>
>>
>
>

[Chris Masone, 2011-01-20 16:03:55]

lgtm

[achuithb, 2011-01-20 18:47:09]

lgtm.

On 2011/01/20 16:03:55, Chris Masone wrote:
> lgtm