Show "Update Chrome OS" in the wrench menu, when the update image is ready.

Show "Update Chrome OS" in the wrench menu, when the update image is ready.

Along the way, replace "About Chrome" with "About Chrome OS" in the wrench menu.

TEST=Connect to a dev server with a newer version, and check that the menu item appears, and the dialog works.

BUG=chromium-os:6526
BUG=chromium-os:7847
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=63491

[satorux1, 2010-10-19 04:47:21]

Ping in case you missed this one. This is an R9 P1 issue per Kan.

[zel, 2010-10-19 15:16:47]

http://codereview.chromium.org/3858002/diff/1/2
File chrome/browser/views/frame/browser_view.cc (left):

http://codereview.chromium.org/3858002/diff/1/2#oldcode1071
chrome/browser/views/frame/browser_view.cc:1071: #if defined(OS_WIN)
I am not sure that we want to display the dialog here. From my memory, I think
we were planning to use a notification instead. Please ping Kan about this one.

[zel, 2010-10-19 15:38:11]

http://codereview.chromium.org/3858002/diff/1/3
File chrome/browser/views/update_recommended_message_box.cc (right):

http://codereview.chromium.org/3858002/diff/1/3#newcode40
chrome/browser/views/update_recommended_message_box.cc:40:
chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
I think we were supposed to restart the machine not just bounce the session
(unless StopSession() does the full system restart). Please ping derat@ to ask
him how he performed restart for power button (there is  a method in libcros for
that I believe).

[satorux1, 2010-10-20 00:44:24]

http://codereview.chromium.org/3858002/diff/1/3
File chrome/browser/views/update_recommended_message_box.cc (right):

http://codereview.chromium.org/3858002/diff/1/3#newcode40
chrome/browser/views/update_recommended_message_box.cc:40:
chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
On 2010/10/19 15:38:11, zel wrote:
> I think we were supposed to restart the machine not just bounce the session
> (unless StopSession() does the full system restart). Please ping derat@ to ask
> him how he performed restart for power button (there is  a method in libcros
for
> that I believe).

Good catch! I think you are right. Daniel, do you know how to reboot the device
using libcros?

[Daniel Erat, 2010-10-20 00:57:39]

On 2010/10/20 00:44:24, satorux1 wrote:
> http://codereview.chromium.org/3858002/diff/1/3
> File chrome/browser/views/update_recommended_message_box.cc (right):
> 
> http://codereview.chromium.org/3858002/diff/1/3#newcode40
> chrome/browser/views/update_recommended_message_box.cc:40:
> chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
> On 2010/10/19 15:38:11, zel wrote:
> > I think we were supposed to restart the machine not just bounce the session
> > (unless StopSession() does the full system restart). Please ping derat@ to
ask
> > him how he performed restart for power button (there is  a method in libcros
> for
> > that I believe).
> 
> Good catch! I think you are right. Daniel, do you know how to reboot the
device
> using libcros?

No, my change didn't involve libcros (other than adding some enum values to a
header in cros.git that's shared between powerd and the window manager).

I initiated shutdown by calling Daemon::StartCleanShutdown() in powerd.cc (in
the power_manager.git repo).  It looks like you can achieve the same thing by
sending kRequestShutdownSignal to kPowerManagerInterface over D-Bus.  You could
try asking bleung if he knows of an easy way to do this from Chrome.

[kanliu_google.com, 2010-10-20 00:59:10]

+benson

On Tue, Oct 19, 2010 at 5:57 PM, <derat@chromium.org> wrote:

> On 2010/10/20 00:44:24, satorux1 wrote:
>
>> http://codereview.chromium.org/3858002/diff/1/3
>> File chrome/browser/views/update_recommended_message_box.cc (right):
>>
>
>  http://codereview.chromium.org/3858002/diff/1/3#newcode40
>> chrome/browser/views/update_recommended_message_box.cc:40:
>> chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
>> On 2010/10/19 15:38:11, zel wrote:
>> > I think we were supposed to restart the machine not just bounce the
>> session
>> > (unless StopSession() does the full system restart). Please ping derat@to
>>
> ask
>
>> > him how he performed restart for power button (there is  a method in
>> libcros
>> for
>> > that I believe).
>>
>
>  Good catch! I think you are right. Daniel, do you know how to reboot the
>>
> device
>
>> using libcros?
>>
>
> No, my change didn't involve libcros (other than adding some enum values to
> a
> header in cros.git that's shared between powerd and the window manager).
>
> I initiated shutdown by calling Daemon::StartCleanShutdown() in powerd.cc
> (in
> the power_manager.git repo).  It looks like you can achieve the same thing
> by
> sending kRequestShutdownSignal to kPowerManagerInterface over D-Bus.  You
> could
> try asking bleung if he knows of an easy way to do this from Chrome.
>
>
> http://codereview.chromium.org/3858002/show
>

[satorux1, 2010-10-20 01:42:21]

On 2010/10/20 00:57:39, Daniel Erat wrote:
> On 2010/10/20 00:44:24, satorux1 wrote:
> > http://codereview.chromium.org/3858002/diff/1/3
> > File chrome/browser/views/update_recommended_message_box.cc (right):
> > 
> > http://codereview.chromium.org/3858002/diff/1/3#newcode40
> > chrome/browser/views/update_recommended_message_box.cc:40:
> > chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
> > On 2010/10/19 15:38:11, zel wrote:
> > > I think we were supposed to restart the machine not just bounce the
session
> > > (unless StopSession() does the full system restart). Please ping derat@ to
> ask
> > > him how he performed restart for power button (there is  a method in
libcros
> > for
> > > that I believe).
> > 
> > Good catch! I think you are right. Daniel, do you know how to reboot the
> device
> > using libcros?
> 
> No, my change didn't involve libcros (other than adding some enum values to a
> header in cros.git that's shared between powerd and the window manager).
> 
> I initiated shutdown by calling Daemon::StartCleanShutdown() in powerd.cc (in
> the power_manager.git repo).  It looks like you can achieve the same thing by
> sending kRequestShutdownSignal to kPowerManagerInterface over D-Bus.  You
could
> try asking bleung if he knows of an easy way to do this from Chrome.

Thanks. If it's just sending a signal over D-Bus, it should be pretty easy to
add a function in chromeos_power.cc in libcros. I'll take a look.

[satorux1, 2010-10-20 06:03:41]

On 2010/10/20 01:42:21, satorux1 wrote:
> On 2010/10/20 00:57:39, Daniel Erat wrote:
> > On 2010/10/20 00:44:24, satorux1 wrote:
> > > http://codereview.chromium.org/3858002/diff/1/3
> > > File chrome/browser/views/update_recommended_message_box.cc (right):
> > > 
> > > http://codereview.chromium.org/3858002/diff/1/3#newcode40
> > > chrome/browser/views/update_recommended_message_box.cc:40:
> > > chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
> > > On 2010/10/19 15:38:11, zel wrote:
> > > > I think we were supposed to restart the machine not just bounce the
> session
> > > > (unless StopSession() does the full system restart). Please ping derat@
to
> > ask
> > > > him how he performed restart for power button (there is  a method in
> libcros
> > > for
> > > > that I believe).
> > > 
> > > Good catch! I think you are right. Daniel, do you know how to reboot the
> > device
> > > using libcros?
> > 
> > No, my change didn't involve libcros (other than adding some enum values to
a
> > header in cros.git that's shared between powerd and the window manager).
> > 
> > I initiated shutdown by calling Daemon::StartCleanShutdown() in powerd.cc
(in
> > the power_manager.git repo).  It looks like you can achieve the same thing
by
> > sending kRequestShutdownSignal to kPowerManagerInterface over D-Bus.  You
> could
> > try asking bleung if he knows of an easy way to do this from Chrome.
> 
> Thanks. If it's just sending a signal over D-Bus, it should be pretty easy to
> add a function in chromeos_power.cc in libcros. I'll take a look.

I've added a function in libcros to request shutdown, but I realized that for
this feature, we need "restart" rather than "shutdown". I think we'll need to
implement "restart" feature in power_manager...

[kanliu_google.com, 2010-10-20 06:07:52]

adding benson back in the thread, i thought we already had something like
this? how are we doing restarts during the OOBE? is that different because
it's outside of chrome?

On Tue, Oct 19, 2010 at 11:03 PM, <satorux@chromium.org> wrote:

> On 2010/10/20 01:42:21, satorux1 wrote:
>
>> On 2010/10/20 00:57:39, Daniel Erat wrote:
>> > On 2010/10/20 00:44:24, satorux1 wrote:
>> > > http://codereview.chromium.org/3858002/diff/1/3
>> > > File chrome/browser/views/update_recommended_message_box.cc (right):
>> > >
>> > > http://codereview.chromium.org/3858002/diff/1/3#newcode40
>> > > chrome/browser/views/update_recommended_message_box.cc:40:
>> > > chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
>> > > On 2010/10/19 15:38:11, zel wrote:
>> > > > I think we were supposed to restart the machine not just bounce the
>> session
>> > > > (unless StopSession() does the full system restart). Please ping
>> derat@
>>
> to
>
>> > ask
>> > > > him how he performed restart for power button (there is  a method in
>> libcros
>> > > for
>> > > > that I believe).
>> > >
>> > > Good catch! I think you are right. Daniel, do you know how to reboot
>> the
>> > device
>> > > using libcros?
>> >
>> > No, my change didn't involve libcros (other than adding some enum values
>> to
>>
> a
>
>> > header in cros.git that's shared between powerd and the window manager).
>> >
>> > I initiated shutdown by calling Daemon::StartCleanShutdown() in
>> powerd.cc
>>
> (in
>
>> > the power_manager.git repo).  It looks like you can achieve the same
>> thing
>>
> by
>
>> > sending kRequestShutdownSignal to kPowerManagerInterface over D-Bus.
>>  You
>> could
>> > try asking bleung if he knows of an easy way to do this from Chrome.
>>
>
>  Thanks. If it's just sending a signal over D-Bus, it should be pretty easy
>> to
>> add a function in chromeos_power.cc in libcros. I'll take a look.
>>
>
> I've added a function in libcros to request shutdown, but I realized that
> for
> this feature, we need "restart" rather than "shutdown". I think we'll need
> to
> implement "restart" feature in power_manager...
>
>
>
> http://codereview.chromium.org/3858002/show
>

[satorux1, 2010-10-20 06:24:34]

Kan, this is a great point. I found this code in the login screen code:

  CrosLibrary::Get()->GetUpdateLibrary()->RebootAfterUpdate();

This will initiate a reboot by Reboot() in utils.cc in update_engine:

bool Reboot() {
  vector<string> command;
  command.push_back("/sbin/shutdown");
  command.push_back("-r");
  command.push_back("now");
  int rc = 0;
  Subprocess::SynchronousExec(command, &rc);
  TEST_AND_RETURN_FALSE(rc == 0);
  return true;
}

However, I'm not sure if this is safe to do while running Chrome. If it's OK, we
can use this in the confirmation dialog. I was thinking about doing

Satoru

On 2010/10/20 06:07:52, kanliu_google.com wrote:
> adding benson back in the thread, i thought we already had something like
> this? how are we doing restarts during the OOBE? is that different because
> it's outside of chrome?
> 
> On Tue, Oct 19, 2010 at 11:03 PM, <mailto:satorux@chromium.org> wrote:
> 
> > On 2010/10/20 01:42:21, satorux1 wrote:
> >
> >> On 2010/10/20 00:57:39, Daniel Erat wrote:
> >> > On 2010/10/20 00:44:24, satorux1 wrote:
> >> > > http://codereview.chromium.org/3858002/diff/1/3
> >> > > File chrome/browser/views/update_recommended_message_box.cc (right):
> >> > >
> >> > > http://codereview.chromium.org/3858002/diff/1/3#newcode40
> >> > > chrome/browser/views/update_recommended_message_box.cc:40:
> >> > > chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
> >> > > On 2010/10/19 15:38:11, zel wrote:
> >> > > > I think we were supposed to restart the machine not just bounce the
> >> session
> >> > > > (unless StopSession() does the full system restart). Please ping
> >> derat@
> >>
> > to
> >
> >> > ask
> >> > > > him how he performed restart for power button (there is  a method in
> >> libcros
> >> > > for
> >> > > > that I believe).
> >> > >
> >> > > Good catch! I think you are right. Daniel, do you know how to reboot
> >> the
> >> > device
> >> > > using libcros?
> >> >
> >> > No, my change didn't involve libcros (other than adding some enum values
> >> to
> >>
> > a
> >
> >> > header in cros.git that's shared between powerd and the window manager).
> >> >
> >> > I initiated shutdown by calling Daemon::StartCleanShutdown() in
> >> powerd.cc
> >>
> > (in
> >
> >> > the power_manager.git repo).  It looks like you can achieve the same
> >> thing
> >>
> > by
> >
> >> > sending kRequestShutdownSignal to kPowerManagerInterface over D-Bus.
> >>  You
> >> could
> >> > try asking bleung if he knows of an easy way to do this from Chrome.
> >>
> >
> >  Thanks. If it's just sending a signal over D-Bus, it should be pretty easy
> >> to
> >> add a function in chromeos_power.cc in libcros. I'll take a look.
> >>
> >
> > I've added a function in libcros to request shutdown, but I realized that
> > for
> > this feature, we need "restart" rather than "shutdown". I think we'll need
> > to
> > implement "restart" feature in power_manager...
> >
> >
> >
> > http://codereview.chromium.org/3858002/show
> >
>

[satorux1, 2010-10-20 06:26:34]

Added bleung to the reviewer list.

On 2010/10/20 06:24:34, satorux1 wrote:
> Kan, this is a great point. I found this code in the login screen code:
> 
>   CrosLibrary::Get()->GetUpdateLibrary()->RebootAfterUpdate();
> 
> This will initiate a reboot by Reboot() in utils.cc in update_engine:
> 
> bool Reboot() {
>   vector<string> command;
>   command.push_back("/sbin/shutdown");
>   command.push_back("-r");
>   command.push_back("now");
>   int rc = 0;
>   Subprocess::SynchronousExec(command, &rc);
>   TEST_AND_RETURN_FALSE(rc == 0);
>   return true;
> }
> 
> However, I'm not sure if this is safe to do while running Chrome. If it's OK,
we
> can use this in the confirmation dialog. I was thinking about doing
> 
> Satoru
> 
> On 2010/10/20 06:07:52, http://kanliu_google.com wrote:
> > adding benson back in the thread, i thought we already had something like
> > this? how are we doing restarts during the OOBE? is that different because
> > it's outside of chrome?
> > 
> > On Tue, Oct 19, 2010 at 11:03 PM, <mailto:satorux@chromium.org> wrote:
> > 
> > > On 2010/10/20 01:42:21, satorux1 wrote:
> > >
> > >> On 2010/10/20 00:57:39, Daniel Erat wrote:
> > >> > On 2010/10/20 00:44:24, satorux1 wrote:
> > >> > > http://codereview.chromium.org/3858002/diff/1/3
> > >> > > File chrome/browser/views/update_recommended_message_box.cc (right):
> > >> > >
> > >> > > http://codereview.chromium.org/3858002/diff/1/3#newcode40
> > >> > > chrome/browser/views/update_recommended_message_box.cc:40:
> > >> > > chromeos::CrosLibrary::Get()->GetLoginLibrary()->StopSession("");
> > >> > > On 2010/10/19 15:38:11, zel wrote:
> > >> > > > I think we were supposed to restart the machine not just bounce the
> > >> session
> > >> > > > (unless StopSession() does the full system restart). Please ping
> > >> derat@
> > >>
> > > to
> > >
> > >> > ask
> > >> > > > him how he performed restart for power button (there is  a method
in
> > >> libcros
> > >> > > for
> > >> > > > that I believe).
> > >> > >
> > >> > > Good catch! I think you are right. Daniel, do you know how to reboot
> > >> the
> > >> > device
> > >> > > using libcros?
> > >> >
> > >> > No, my change didn't involve libcros (other than adding some enum
values
> > >> to
> > >>
> > > a
> > >
> > >> > header in cros.git that's shared between powerd and the window
manager).
> > >> >
> > >> > I initiated shutdown by calling Daemon::StartCleanShutdown() in
> > >> powerd.cc
> > >>
> > > (in
> > >
> > >> > the power_manager.git repo).  It looks like you can achieve the same
> > >> thing
> > >>
> > > by
> > >
> > >> > sending kRequestShutdownSignal to kPowerManagerInterface over D-Bus.
> > >>  You
> > >> could
> > >> > try asking bleung if he knows of an easy way to do this from Chrome.
> > >>
> > >
> > >  Thanks. If it's just sending a signal over D-Bus, it should be pretty
easy
> > >> to
> > >> add a function in chromeos_power.cc in libcros. I'll take a look.
> > >>
> > >
> > > I've added a function in libcros to request shutdown, but I realized that
> > > for
> > > this feature, we need "restart" rather than "shutdown". I think we'll need
> > > to
> > > implement "restart" feature in power_manager...
> > >
> > >
> > >
> > > http://codereview.chromium.org/3858002/show
> > >
> >

[Benson Leung, 2010-10-20 06:42:48]

Adding a restart should be simple. shutdown -r now is indeed the correct thing
to do to restart the system, but it sidesteps the wait on cromo disconnect for
carrier compliance. We wait for cromo to clean up before we issue shutdown or
suspend.

[satorux1, 2010-10-20 06:57:25]

Benson,

Does that mean that we should NOT use this function while running Chrome, as it
sidesteps the cromo disconnection?

If we should reboot the system more gracefully, we'd probably need to do
something similar to what we do for graceful shutdown (send shutdown request to
power manager, and invokes /etc/init/clean-shutdown.conf). Maybe we should
create /etc/init/clean-reboot.conf? We have /etc/init/reboot.conf but this
doesn't seem to handle cromo disconnection.



On 2010/10/20 06:42:48, Benson Leung wrote:
> Adding a restart should be simple. shutdown -r now is indeed the correct thing
> to do to restart the system, but it sidesteps the wait on cromo disconnect for
> carrier compliance. We wait for cromo to clean up before we issue shutdown or
> suspend.

[Benson Leung, 2010-10-20 07:42:05]

We would probably not have to create two different confs for shutdown and
reboot. It could all be handled in powerd with a different command run at the
end, that's all.

It's good practice to have a reboot follow the same path as shutdown, but I'm
not 100% sure it is a requirement like for shutdown or suspend, but I will ask
our connectivity team.

The changes on my side are minimal, and should take less than a day. Let me know
if it is required.

On 2010/10/20 06:57:25, satorux1 wrote:
> Benson,
> 
> Does that mean that we should NOT use this function while running Chrome, as
it
> sidesteps the cromo disconnection?
> 
> If we should reboot the system more gracefully, we'd probably need to do
> something similar to what we do for graceful shutdown (send shutdown request
to
> power manager, and invokes /etc/init/clean-shutdown.conf). Maybe we should
> create /etc/init/clean-reboot.conf? We have /etc/init/reboot.conf but this
> doesn't seem to handle cromo disconnection.
> 
> 
> 
> On 2010/10/20 06:42:48, Benson Leung wrote:
> > Adding a restart should be simple. shutdown -r now is indeed the correct
thing
> > to do to restart the system, but it sidesteps the wait on cromo disconnect
for
> > carrier compliance. We wait for cromo to clean up before we issue shutdown
or
> > suspend.

[satorux1, 2010-10-21 00:12:39]

Benson,

I think it's nice to have a graceful reboot feature that follows the same path
as graceful shutdown, in the power manager. I'd rather use the graceful reboot
feature, rather than 
CrosLibrary::Get()->GetUpdateLibrary()->RebootAfterUpdate() that instantly runs
"shutdown -r now". Could you implement it in the power manager? The change seems
to be simple, so I could give it a shot if you are busy.


Besides, the existing function UpdateLibrary::RebootAfterUpdate() can be only
used after update is complete. I guess there will be cases where we want to
reboot in other situations, like rebooting from the screen locker, etc.


On 2010/10/20 07:42:05, Benson Leung wrote:
> We would probably not have to create two different confs for shutdown and
> reboot. It could all be handled in powerd with a different command run at the
> end, that's all.
> 
> It's good practice to have a reboot follow the same path as shutdown, but I'm
> not 100% sure it is a requirement like for shutdown or suspend, but I will ask
> our connectivity team.
> 
> The changes on my side are minimal, and should take less than a day. Let me
know
> if it is required.
> 
> On 2010/10/20 06:57:25, satorux1 wrote:
> > Benson,
> > 
> > Does that mean that we should NOT use this function while running Chrome, as
> it
> > sidesteps the cromo disconnection?
> > 
> > If we should reboot the system more gracefully, we'd probably need to do
> > something similar to what we do for graceful shutdown (send shutdown request
> to
> > power manager, and invokes /etc/init/clean-shutdown.conf). Maybe we should
> > create /etc/init/clean-reboot.conf? We have /etc/init/reboot.conf but this
> > doesn't seem to handle cromo disconnection.
> > 
> > 
> > 
> > On 2010/10/20 06:42:48, Benson Leung wrote:
> > > Adding a restart should be simple. shutdown -r now is indeed the correct
> thing
> > > to do to restart the system, but it sidesteps the wait on cromo disconnect
> for
> > > carrier compliance. We wait for cromo to clean up before we issue shutdown
> or
> > > suspend.

[Benson Leung, 2010-10-21 00:19:19]

On 2010/10/21 00:12:39, satorux1 wrote:
> Benson,
> 
> I think it's nice to have a graceful reboot feature that follows the same path
> as graceful shutdown, in the power manager. I'd rather use the graceful reboot
> feature, rather than 
> CrosLibrary::Get()->GetUpdateLibrary()->RebootAfterUpdate() that instantly
runs
> "shutdown -r now". Could you implement it in the power manager? The change
seems
> to be simple, so I could give it a shot if you are busy.
> 
> 
> Besides, the existing function UpdateLibrary::RebootAfterUpdate() can be only
> used after update is complete. I guess there will be cases where we want to
> reboot in other situations, like rebooting from the screen locker, etc.
> 
> 
> On 2010/10/20 07:42:05, Benson Leung wrote:
> > We would probably not have to create two different confs for shutdown and
> > reboot. It could all be handled in powerd with a different command run at
the
> > end, that's all.
> > 
> > It's good practice to have a reboot follow the same path as shutdown, but
I'm
> > not 100% sure it is a requirement like for shutdown or suspend, but I will
ask
> > our connectivity team.
> > 
> > The changes on my side are minimal, and should take less than a day. Let me
> know
> > if it is required.
> > 
> > On 2010/10/20 06:57:25, satorux1 wrote:
> > > Benson,
> > > 
> > > Does that mean that we should NOT use this function while running Chrome,
as
> > it
> > > sidesteps the cromo disconnection?
> > > 
> > > If we should reboot the system more gracefully, we'd probably need to do
> > > something similar to what we do for graceful shutdown (send shutdown
request
> > to
> > > power manager, and invokes /etc/init/clean-shutdown.conf). Maybe we should
> > > create /etc/init/clean-reboot.conf? We have /etc/init/reboot.conf but this
> > > doesn't seem to handle cromo disconnection.
> > > 
> > > 
> > > 
> > > On 2010/10/20 06:42:48, Benson Leung wrote:
> > > > Adding a restart should be simple. shutdown -r now is indeed the correct
> > thing
> > > > to do to restart the system, but it sidesteps the wait on cromo
disconnect
> > for
> > > > carrier compliance. We wait for cromo to clean up before we issue
shutdown
> > or
> > > > suspend.

I can take care of this. I'll send you the changelist when I'm ready. Do you
have an issue that I can use to track this?

[satorux1, 2010-10-21 00:37:20]

Thanks. Filed a bug: http://crosbug.com/7995

On 2010/10/21 00:19:19, Benson Leung wrote:
> On 2010/10/21 00:12:39, satorux1 wrote:
> > Benson,
> > 
> > I think it's nice to have a graceful reboot feature that follows the same
path
> > as graceful shutdown, in the power manager. I'd rather use the graceful
reboot
> > feature, rather than 
> > CrosLibrary::Get()->GetUpdateLibrary()->RebootAfterUpdate() that instantly
> runs
> > "shutdown -r now". Could you implement it in the power manager? The change
> seems
> > to be simple, so I could give it a shot if you are busy.
> > 
> > 
> > Besides, the existing function UpdateLibrary::RebootAfterUpdate() can be
only
> > used after update is complete. I guess there will be cases where we want to
> > reboot in other situations, like rebooting from the screen locker, etc.
> > 
> > 
> > On 2010/10/20 07:42:05, Benson Leung wrote:
> > > We would probably not have to create two different confs for shutdown and
> > > reboot. It could all be handled in powerd with a different command run at
> the
> > > end, that's all.
> > > 
> > > It's good practice to have a reboot follow the same path as shutdown, but
> I'm
> > > not 100% sure it is a requirement like for shutdown or suspend, but I will
> ask
> > > our connectivity team.
> > > 
> > > The changes on my side are minimal, and should take less than a day. Let
me
> > know
> > > if it is required.
> > > 
> > > On 2010/10/20 06:57:25, satorux1 wrote:
> > > > Benson,
> > > > 
> > > > Does that mean that we should NOT use this function while running
Chrome,
> as
> > > it
> > > > sidesteps the cromo disconnection?
> > > > 
> > > > If we should reboot the system more gracefully, we'd probably need to do
> > > > something similar to what we do for graceful shutdown (send shutdown
> request
> > > to
> > > > power manager, and invokes /etc/init/clean-shutdown.conf). Maybe we
should
> > > > create /etc/init/clean-reboot.conf? We have /etc/init/reboot.conf but
this
> > > > doesn't seem to handle cromo disconnection.
> > > > 
> > > > 
> > > > 
> > > > On 2010/10/20 06:42:48, Benson Leung wrote:
> > > > > Adding a restart should be simple. shutdown -r now is indeed the
correct
> > > thing
> > > > > to do to restart the system, but it sidesteps the wait on cromo
> disconnect
> > > for
> > > > > carrier compliance. We wait for cromo to clean up before we issue
> shutdown
> > > or
> > > > > suspend.
> 
> I can take care of this. I'll send you the changelist when I'm ready. Do you
> have an issue that I can use to track this?

[satorux1, 2010-10-21 10:38:53]

Please take another look.

Addressed the two issues:

1. We should "restart" rather than "log out"
2. Show "Restart" button in the about page, when appropriate

On 2010/10/21 00:37:20, satorux1 wrote:
> Thanks. Filed a bug: http://crosbug.com/7995
> 
> On 2010/10/21 00:19:19, Benson Leung wrote:
> > On 2010/10/21 00:12:39, satorux1 wrote:
> > > Benson,
> > > 
> > > I think it's nice to have a graceful reboot feature that follows the same
> path
> > > as graceful shutdown, in the power manager. I'd rather use the graceful
> reboot
> > > feature, rather than 
> > > CrosLibrary::Get()->GetUpdateLibrary()->RebootAfterUpdate() that instantly
> > runs
> > > "shutdown -r now". Could you implement it in the power manager? The change
> > seems
> > > to be simple, so I could give it a shot if you are busy.
> > > 
> > > 
> > > Besides, the existing function UpdateLibrary::RebootAfterUpdate() can be
> only
> > > used after update is complete. I guess there will be cases where we want
to
> > > reboot in other situations, like rebooting from the screen locker, etc.
> > > 
> > > 
> > > On 2010/10/20 07:42:05, Benson Leung wrote:
> > > > We would probably not have to create two different confs for shutdown
and
> > > > reboot. It could all be handled in powerd with a different command run
at
> > the
> > > > end, that's all.
> > > > 
> > > > It's good practice to have a reboot follow the same path as shutdown,
but
> > I'm
> > > > not 100% sure it is a requirement like for shutdown or suspend, but I
will
> > ask
> > > > our connectivity team.
> > > > 
> > > > The changes on my side are minimal, and should take less than a day. Let
> me
> > > know
> > > > if it is required.
> > > > 
> > > > On 2010/10/20 06:57:25, satorux1 wrote:
> > > > > Benson,
> > > > > 
> > > > > Does that mean that we should NOT use this function while running
> Chrome,
> > as
> > > > it
> > > > > sidesteps the cromo disconnection?
> > > > > 
> > > > > If we should reboot the system more gracefully, we'd probably need to
do
> > > > > something similar to what we do for graceful shutdown (send shutdown
> > request
> > > > to
> > > > > power manager, and invokes /etc/init/clean-shutdown.conf). Maybe we
> should
> > > > > create /etc/init/clean-reboot.conf? We have /etc/init/reboot.conf but
> this
> > > > > doesn't seem to handle cromo disconnection.
> > > > > 
> > > > > 
> > > > > 
> > > > > On 2010/10/20 06:42:48, Benson Leung wrote:
> > > > > > Adding a restart should be simple. shutdown -r now is indeed the
> correct
> > > > thing
> > > > > > to do to restart the system, but it sidesteps the wait on cromo
> > disconnect
> > > > for
> > > > > > carrier compliance. We wait for cromo to clean up before we issue
> > shutdown
> > > > or
> > > > > > suspend.
> > 
> > I can take care of this. I'll send you the changelist when I'm ready. Do you
> > have an issue that I can use to track this?

[zel, 2010-10-22 01:15:22]

LGTM

Btw, does your dialog still has that blue Win95 look and feel? If so, please
ping xiyuan@ to give you instructions how to change it to match the official
ChromeOS look.

[satorux1, 2010-10-22 01:29:34]

Thanks.

Looking at this change: http://codereview.chromium.org/3538012/diff/26001/27007

It should be as easy as replacing a function call:

-  views::Window* window = views::Window::CreateChromeWindow(
+  views::Window* window = browser::CreateViewsWindow(

I'll give it a try.

On 2010/10/22 01:15:22, zel wrote:
> LGTM
> 
> Btw, does your dialog still has that blue Win95 look and feel? If so, please
> ping xiyuan@ to give you instructions how to change it to match the official
> ChromeOS look.

[satorux1, 2010-10-22 01:42:01]

Done. Worked perfectly.

On 2010/10/22 01:29:34, satorux1 wrote:
> Thanks.
> 
> Looking at this change:
http://codereview.chromium.org/3538012/diff/26001/27007
> 
> It should be as easy as replacing a function call:
> 
> -  views::Window* window = views::Window::CreateChromeWindow(
> +  views::Window* window = browser::CreateViewsWindow(
> 
> I'll give it a try.
> 
> On 2010/10/22 01:15:22, zel wrote:
> > LGTM
> > 
> > Btw, does your dialog still has that blue Win95 look and feel? If so, please
> > ping xiyuan@ to give you instructions how to change it to match the official
> > ChromeOS look.

[zel, 2010-10-22 01:57:46]

Great! Ship it!

On Thu, Oct 21, 2010 at 6:42 PM, <satorux@chromium.org> wrote:

> Done. Worked perfectly.
>
>
> On 2010/10/22 01:29:34, satorux1 wrote:
>
>> Thanks.
>>
>
>  Looking at this change:
>>
> http://codereview.chromium.org/3538012/diff/26001/27007
>
>  It should be as easy as replacing a function call:
>>
>
>  -  views::Window* window = views::Window::CreateChromeWindow(
>> +  views::Window* window = browser::CreateViewsWindow(
>>
>
>  I'll give it a try.
>>
>
>  On 2010/10/22 01:15:22, zel wrote:
>> > LGTM
>> >
>> > Btw, does your dialog still has that blue Win95 look and feel? If so,
>> please
>> > ping xiyuan@ to give you instructions how to change it to match the
>> official
>> > ChromeOS look.
>>
>
>
>
> http://codereview.chromium.org/3858002/show
>