Chris Masone has uploaded a new change for review.
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
[login_manager] Make hang-detection flag optionally take an interval
By allowing this flag to optionally take a value, the interval
can be shortened for testing, or set to the default, while
still allowing the feature to be disabled completely.
BUG=chromium-os:35729
TEST=unit
Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
---
M liveness_checker_impl.cc
M liveness_checker_impl.h
M liveness_checker_impl_unittest.cc
M session_manager_main.cc
M session_manager_service.cc
M session_manager_service.h
6 files changed, 42 insertions(+), 21 deletions(-)
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
Patch Set 1: (1 inline comment)
....................................................
File liveness_checker_impl.cc
Line 37: enable_aborting_(interval_seconds >= 0),
this feels too tricky. i think it'd be better to keep separate interval and "enabled" settings.
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
Patch Set 1: (1 inline comment)
....................................................
File liveness_checker_impl.cc
Line 37: enable_aborting_(interval_seconds >= 0),
Done
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
Patch Set 2: (4 inline comments)
....................................................
File liveness_checker_impl.cc
Line 38: kLivenessCheckIntervalSeconds),
i think it'd also be cleaner to get rid of the "negative means use some default" behavior and just make the caller pass in whatever interval they want. (otherwise, you end up with e.g. session_manager_main.cc claiming that the default is 60, when the actual constant lives here -- it'd be easy for the usage message to get out of sync.)
....................................................
File liveness_checker_impl_unittest.cc
Line 108: false, /* Disable aborting */
nit: i think that // is usually used for end-of-line comments; c-style comments are used for e.g.
false /* disable aborting */,
....................................................
File session_manager_main.cc
Line 135: hang_detection_interval = -1;
you're assigning -1 to an unsigned int here
....................................................
File session_manager_service.h
Line 74: uint hang_detection_interval,
document that a negative interval disables hang detection; also make this be an int instead of an unsigned int.
it'd be good to put a "_sec" prefix on the name so that callers aren't left wondering if it's ms, seconds, etc. base::TimeDelta would be best, although that makes it tougher to make this double as a enable/disable setting -- you could use null/0 for that, i suppose.
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
Patch Set 2: (4 inline comments)
....................................................
File liveness_checker_impl.cc
Line 38: kLivenessCheckIntervalSeconds),
it's all gone now, the -1 stuff
....................................................
File session_manager_main.cc
Line 135: hang_detection_interval = -1;
hurr.
The intent was to go back to uints all over and get rid of the -1 stuff, but I only got halfway there. Shouldn't send code at the end of the day :-/
....................................................
File session_manager_service.h
Line 74: uint hang_detection_interval,
making it a timedelta, and adding explicit bool
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File session_manager_main.cc
Line 138: if (base::StringToUint(flag, &from_flag)) {
nit: can you just do this, or are you worried about StringToUint changing the passed-in dest even on parse failure?
if (!base::StringToUint(flag, &hang_detection_interval))
DLOG(WARNING) << "etc.";
(if you're worried about this, it'd still be a bit shorter to reassign the default to hang_detection_interval in the failure case)
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................
