When critical updates have been installed and the user has been idle for quite some time, show a ...

When critical updates have been installed and the user has been idle for quite some time, show a bubble anchored to the wrench menu with a countdown clock for auto-restarting.

BUG=97665
TEST=The testing for this is a bit too long-windy to describe here. Contact me for details.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105298

[Finnur, 2011-10-12 09:48:40]

Ben, you reviewed the upgrade detector a while back, so it makes sense for you
to review this, as it builds on it. Got time? :)

[Ben Goodger (Google), 2011-10-12 15:10:45]

LGTM

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/cri...
File chrome/browser/ui/views/critical_notification_bubble_view.cc (right):

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/cri...
chrome/browser/ui/views/critical_notification_bubble_view.cc:108:
BrowserList::AttemptRestart();
btw, what happens if a tab stops shutdown (with an onbeforeunload etc.)?

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/cri...
chrome/browser/ui/views/critical_notification_bubble_view.cc:188: bool
CriticalNotificationBubbleView::CloseOnEscape() {
general bubble design notes for future reference: CloseOnEscape and FadeInOnShow
should be properties on the bubble, not delegate methods.

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/cri...
File chrome/browser/ui/views/critical_notification_bubble_view.h (right):

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/cri...
chrome/browser/ui/views/critical_notification_bubble_view.h:42: virtual string16
accessible_name() OVERRIDE;
side comment: ugh, virtual method with unix_hacker name.

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:46: #include
"chrome/browser/ui/views/critical_notification_bubble_view.h"
this should go in the !defined(USE_AURA) bit

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:119: #if defined(OS_WIN)
&& !defined(USE_AURA)

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:429: #if defined(OS_WIN)
&& !defined(USE_AURA)

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:725: #if defined(OS_WIN)
&& !defined(USE_AURA)

[Finnur, 2011-10-13 14:34:10]

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/cri...
File chrome/browser/ui/views/critical_notification_bubble_view.cc (right):

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/cri...
chrome/browser/ui/views/critical_notification_bubble_view.cc:108:
BrowserList::AttemptRestart();
What happens? Good question. The "Leave-this-page?" dialog appears. 

If you select Leave, the browser shuts down and is restored, as it would
normally.

If you select Stay, the restart sequence halts, but the critical notification
bubble remains open, allowing you to Reboot directly from the bubble (browser is
restored, everything works normally).

If you choose "Don't reboot" in the bubble, the bubble dismisses and you can
continue to use the browser. Once you decide to shutdown, the flag (to restart
on shutdown) has already been set, so the browser relaunches, restoring your
session. I guess we should clean the pref to restart if the user opts to not
restart... I'll submit a follow-up changelist.

The only other thing that annoys me about this is that by selecting "Leave"
during onbeforeunload the bubble is up with the message "Google Chrome will
restart in 1 seconds", which is now misleading (the counter is frozen because
the clock has been stopped)... :/

I'll submit a followup that changes the headline on 0 seconds to:

You should restart [PRODUCT_NAME] now.


On 2011/10/12 15:10:45, Ben Goodger (Google) wrote:
> btw, what happens if a tab stops shutdown (with an onbeforeunload etc.)?

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/cri...
File chrome/browser/ui/views/critical_notification_bubble_view.h (right):

http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/cri...
chrome/browser/ui/views/critical_notification_bubble_view.h:42: virtual string16
accessible_name() OVERRIDE;
Yeah, don't worry about this. Someone fixed the string16 issue right under my
nose and fixed the unix_hacker style as well.

[Ben Goodger (Google), 2011-10-13 14:59:39]

Thanks for the explanation. Aside from this work, I think it'd be
interesting to have an experimental/about:flags mode that just closes the
tab (i.e. performs the "leave this page" automatically).

My guess is that I have a Chrome tab open somewhere that has something
that'd prevent your feature from working (e.g. freenode IRC, or some other
poorly designed editor that wants to stop me from leaving), so I'd most
likely return to my computer in the morning and find it in this state,
defeating the purpose of the auto-restart.

Given that other modern app development platforms (e.g. iOS, Android) are
forcing developers to deal with their apps being terminated or freeze-dried
at any moment, it seems like it'd be an interesting experiment for us to try
for webapps.

-Ben

On Thu, Oct 13, 2011 at 7:34 AM, <finnur@chromium.org> wrote:

>
> http://codereview.chromium.**org/8142028/diff/15001/chrome/**
>
browser/ui/views/critical_**notification_bubble_view.cc<http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/critical_notification_bubble_view.cc>
> File chrome/browser/ui/views/**critical_notification_bubble_**view.cc
> (right):
>
> http://codereview.chromium.**org/8142028/diff/15001/chrome/**
>
browser/ui/views/critical_**notification_bubble_view.cc#**newcode108<http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/critical_notification_bubble_view.cc#newcode108>
> chrome/browser/ui/views/**critical_notification_bubble_**view.cc:108:
> BrowserList::AttemptRestart();
> What happens? Good question. The "Leave-this-page?" dialog appears.
>
> If you select Leave, the browser shuts down and is restored, as it would
> normally.
>
> If you select Stay, the restart sequence halts, but the critical
> notification bubble remains open, allowing you to Reboot directly from
> the bubble (browser is restored, everything works normally).
>
> If you choose "Don't reboot" in the bubble, the bubble dismisses and you
> can continue to use the browser. Once you decide to shutdown, the flag
> (to restart on shutdown) has already been set, so the browser
> relaunches, restoring your session. I guess we should clean the pref to
> restart if the user opts to not restart... I'll submit a follow-up
> changelist.
>
> The only other thing that annoys me about this is that by selecting
> "Leave" during onbeforeunload the bubble is up with the message "Google
> Chrome will restart in 1 seconds", which is now misleading (the counter
> is frozen because the clock has been stopped)... :/
>
> I'll submit a followup that changes the headline on 0 seconds to:
>
> You should restart [PRODUCT_NAME] now.
>
>
>
> On 2011/10/12 15:10:45, Ben Goodger (Google) wrote:
>
>> btw, what happens if a tab stops shutdown (with an onbeforeunload
>>
> etc.)?
>
> http://codereview.chromium.**org/8142028/diff/15001/chrome/**
>
browser/ui/views/critical_**notification_bubble_view.h<http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/critical_notification_bubble_view.h>
> File chrome/browser/ui/views/**critical_notification_bubble_**view.h
> (right):
>
> http://codereview.chromium.**org/8142028/diff/15001/chrome/**
>
browser/ui/views/critical_**notification_bubble_view.h#**newcode42<http://codereview.chromium.org/8142028/diff/15001/chrome/browser/ui/views/critical_notification_bubble_view.h#newcode42>
> chrome/browser/ui/views/**critical_notification_bubble_**view.h:42:
> virtual
> string16 accessible_name() OVERRIDE;
> Yeah, don't worry about this. Someone fixed the string16 issue right
> under my nose and fixed the unix_hacker style as well.
>
>
http://codereview.chromium.**org/8142028/<http://codereview.chromium.org/8142...
>