Fixed continue window bugs and added 60s auto-shutdown timeout.

Fixed continue window bugs and added 60s auto-shutdown timeout.

BUG=92415
TEST=Manual

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

[Jamie, 2011-08-24 01:06:54]

This CL implements a 60s timer on the continue window, after which the
connection is automatically closed. It also fixes a couple of bugs that had
crept in. PTAL.

[Sergey Ulanov, 2011-08-24 01:56:45]

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:19: static const int
kContinueWindowShowTimeoutMs = 10  * 1000;
Revert this change

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:23: static const int
kContinueWindowHideTimeoutMs = 5 * 1000;
Should be 60 * 1000 instead of 5 seconds

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:230: continue_timer_state_ = start ?
SHOW_DIALOG : INACTIVE;
What if this function is called with when continue_timer_state == HIDE_DIALOG?
Should we really reset it to SHOW_DIALOG?

I think it's better to add continue_timer_start_ = SHOW_DIALOG in the if block
above and add else block to handle other cases

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:255: host_->Shutdown(NULL);
I don't like the idea of calling Shutdown() from here directly, but it's Ok to
have it here for now since UI code calls Shutdown() anyway.

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.h (right):

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.h:108: enum {INACTIVE, SHOW_DIALOG,
HIDE_DIALOG} continue_timer_state_;
This is not very readable. Please define type ContinueTimerState and use it
here. Also, add description for each state.

[Jamie, 2011-08-24 17:42:02]

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:19: static const int
kContinueWindowShowTimeoutMs = 10  * 1000;
On 2011/08/24 01:56:45, sergeyu wrote:
> Revert this change

Done.

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:23: static const int
kContinueWindowHideTimeoutMs = 5 * 1000;
On 2011/08/24 01:56:45, sergeyu wrote:
> Should be 60 * 1000 instead of 5 seconds

Done.

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:230: continue_timer_state_ = start ?
SHOW_DIALOG : INACTIVE;
On 2011/08/24 01:56:45, sergeyu wrote:
> What if this function is called with when continue_timer_state == HIDE_DIALOG?
> Should we really reset it to SHOW_DIALOG?
> 
> I think it's better to add continue_timer_start_ = SHOW_DIALOG in the if block
> above and add else block to handle other cases

Done.

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.h (right):

http://codereview.chromium.org/7714026/diff/2001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.h:108: enum {INACTIVE, SHOW_DIALOG,
HIDE_DIALOG} continue_timer_state_;
On 2011/08/24 01:56:45, sergeyu wrote:
> This is not very readable. Please define type ContinueTimerState and use it
> here. Also, add description for each state.

Done. I've also renamed HIDE_DIALOG to SHUTDOWN_HOST, since that's the primary
action taken on timer expiry.

[Sergey Ulanov, 2011-08-24 18:38:36]

LGTM if the comments are fixed.

http://codereview.chromium.org/7714026/diff/3004/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/7714026/diff/3004/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:240: if (continue_timer_state_ == INACTIVE
||
The comment above this line may be confusing now. Please move the new check
below. Even better would be to use switch statement for |continue_timer_state_|

http://codereview.chromium.org/7714026/diff/3004/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.h (right):

http://codereview.chromium.org/7714026/diff/3004/remoting/host/desktop_enviro...
remoting/host/desktop_environment.h:108: enum ContinueTimerState {
Type definition should precede methods declaration:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

[Jamie, 2011-08-25 00:16:39]

http://codereview.chromium.org/7714026/diff/3004/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/7714026/diff/3004/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:240: if (continue_timer_state_ == INACTIVE
||
On 2011/08/24 18:38:37, sergeyu wrote:
> The comment above this line may be confusing now. Please move the new check
> below. Even better would be to use switch statement for
|continue_timer_state_|

Done.

http://codereview.chromium.org/7714026/diff/3004/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.h (right):

http://codereview.chromium.org/7714026/diff/3004/remoting/host/desktop_enviro...
remoting/host/desktop_environment.h:108: enum ContinueTimerState {
On 2011/08/24 18:38:37, sergeyu wrote:
> Type definition should precede methods declaration:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.