Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9423043:
Add the protobufs for the set of session timeout policies planned for the kiosk mode. (Closed)
|
Created:
8 years, 10 months ago by pastarmovj Modified:
8 years, 9 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd the protobufs for the set of session timeout policies planned for the kiosk mode.
BUG=chromium-os:26250
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125159
Patch Set 1 #
Total comments: 6
Patch Set 2 : Removed unused fields as per Rahul's comment and cleaned up comments. #Patch Set 3 : Updated the protobuf according to the discussion and added client side binding. #
Total comments: 20
Patch Set 4 : Addressed comments. #
Total comments: 22
Patch Set 5 : Addressed comments and did some more cleanup. #
Messages
Total messages: 15 (0 generated)
Tis is a CL that adds the proposed set of timeout policies to the device settings protobuf. @Mattias, Bin: Please give me your opinion if you find the protobuf structure fine. If so I will commit this and proceed with augmenting the client site protobuf parser to support those policies and expose them through the CrosSettings interface. @Rahul: FYI.
As discussed offline, can we just land this with the code that is actually going to use it? I don't this to have the same fate as CameraEnabledProto. http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... File chrome/browser/policy/proto/chrome_device_policy.proto (right): http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... chrome/browser/policy/proto/chrome_device_policy.proto:105: // If omitted or set to 0. No idle timeout will be performed. Please fix comment grammar, here and below.
http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... File chrome/browser/policy/proto/chrome_device_policy.proto (right): http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... chrome/browser/policy/proto/chrome_device_policy.proto:117: optional int64 idle_logout_warning_duration = 4; What's the difference between this and the forced_logout_warning_duration? Isn't forced logout something that will only happen on idle timeout anyway?
Thanks for looking into this CL guys. I updated it as per your comments. I'll still hold off committing this until Rahul's CLs are fully committed. http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... File chrome/browser/policy/proto/chrome_device_policy.proto (right): http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... chrome/browser/policy/proto/chrome_device_policy.proto:105: // If omitted or set to 0. No idle timeout will be performed. On 2012/02/21 10:41:38, Mattias Nissler wrote: > Please fix comment grammar, here and below. Done. http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... chrome/browser/policy/proto/chrome_device_policy.proto:117: optional int64 idle_logout_warning_duration = 4; On 2012/02/21 18:15:36, Rahul Chaturvedi wrote: > What's the difference between this and the forced_logout_warning_duration? Isn't > forced logout something that will only happen on idle timeout anyway? I thought somewhere along the course of discussion there was the idea to also log out unconditionally after some time. But if this is not implemented yet we can as well remove those fields until it is really needed.
On 2012/02/22 14:22:26, pastarmovj wrote: > Thanks for looking into this CL guys. I updated it as per your comments. > > I'll still hold off committing this until Rahul's CLs are fully committed. Cool, let's update the CL then to wire it up with Rahul's code. Note that you should also add parsing code for the new policies in device_policy_cache.cc > > http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... > File chrome/browser/policy/proto/chrome_device_policy.proto (right): > > http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... > chrome/browser/policy/proto/chrome_device_policy.proto:105: // If omitted or set > to 0. No idle timeout will be performed. > On 2012/02/21 10:41:38, Mattias Nissler wrote: > > Please fix comment grammar, here and below. > > Done. > > http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... > chrome/browser/policy/proto/chrome_device_policy.proto:117: optional int64 > idle_logout_warning_duration = 4; > On 2012/02/21 18:15:36, Rahul Chaturvedi wrote: > > What's the difference between this and the forced_logout_warning_duration? > Isn't > > forced logout something that will only happen on idle timeout anyway? > > I thought somewhere along the course of discussion there was the idea to also > log out unconditionally after some time. But if this is not implemented yet we > can as well remove those fields until it is really needed.
http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... File chrome/browser/policy/proto/chrome_device_policy.proto (right): http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... chrome/browser/policy/proto/chrome_device_policy.proto:117: optional int64 idle_logout_warning_duration = 4; On 2012/02/22 14:22:27, pastarmovj wrote: > On 2012/02/21 18:15:36, Rahul Chaturvedi wrote: > > What's the difference between this and the forced_logout_warning_duration? > Isn't > > forced logout something that will only happen on idle timeout anyway? > > I thought somewhere along the course of discussion there was the idea to also > log out unconditionally after some time. But if this is not implemented yet we > can as well remove those fields until it is really needed. Ah I see what you mean. No, we don't plan on having a forced logout in terms of absolute time at the moment. Though let me check with Josh if that is something we might need.
http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... File chrome/browser/policy/proto/chrome_device_policy.proto (right): http://codereview.chromium.org/9423043/diff/1/chrome/browser/policy/proto/chr... chrome/browser/policy/proto/chrome_device_policy.proto:117: optional int64 idle_logout_warning_duration = 4; On 2012/02/22 17:39:07, Rahul Chaturvedi wrote: > On 2012/02/22 14:22:27, pastarmovj wrote: > > On 2012/02/21 18:15:36, Rahul Chaturvedi wrote: > > > What's the difference between this and the forced_logout_warning_duration? > > Isn't > > > forced logout something that will only happen on idle timeout anyway? > > > > I thought somewhere along the course of discussion there was the idea to also > > log out unconditionally after some time. But if this is not implemented yet we > > can as well remove those fields until it is really needed. > > Ah I see what you mean. No, we don't plan on having a forced logout in terms of > absolute time at the moment. Though let me check with Josh if that is something > we might need. Spoke with Josh; the forced_logout_warning_duration and forced_logout_timeout are not needed at this point. These are both tentative policies that have issues that need to be worked out with before they should be implemented. The idle_logout_timeout and idle_logout_warning_duration are the only two needed here.
Updated this CL according to the discussion and added client side accessors for those policies.
On 2012/02/24 15:08:20, pastarmovj wrote: > Updated this CL according to the discussion and added client side accessors for > those policies. Hey Julian, if this is fixed, we should just commit this, yeah?
http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/cro... File chrome/browser/chromeos/cros_settings_names.cc (right): http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros_settings_names.cc:55: // Parts of the ScreenSaver proto. Define the extension ID of the screen saver s/Define/Defines/ http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros_settings_names.cc:55: // Parts of the ScreenSaver proto. Define the extension ID of the screen saver s/Parts/Values from/ http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros_settings_names.cc:60: // Parts of the ForcedLogoutTimeouts proto. Define the timeouts before a user s/Parts/Values from/ http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros_settings_names.cc:60: // Parts of the ForcedLogoutTimeouts proto. Define the timeouts before a user s/Define/Defines/ http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/dev... File chrome/browser/chromeos/device_settings_provider.cc (right): http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:282: // intended to be customizable by the user yet: The yet is not accurate. Put it in parens at least. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:346: // -------------------- Instead of doing this, how about actually breaking the code up into functions? Even better, maybe break it into logical groups instead of by type? http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... File chrome/browser/policy/proto/chrome_device_policy.proto (right): http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:116: // If this field is omitted or set to 0, no idle timeout will be performed. "no idle timeout will be performed": Not clear what this means? I guess we don't do idle logouts in that case? http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:120: // This field is only used if |idle_logout_timeout| is specified. not quite accurate, should be if |idle_logut_timeout| > 0. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:125: // Specifies the extension ID which is to be used as a screen saver. We should point out that this screen saver applies to pre-login only. Somebody who doesn't have context will believe this is the session screensaver. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:127: // Specified in milliseconds the timeout before the screen saver is activated grammar http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:128: // on the login screen if no user activity present. Only respected if the oh, so here it says login screen. Can we move that to a comment on the message?
Adding Joao as replacement for Mattias. @Joao: Can you please take a look. Mattias already went over it so it should be quite polished already. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/cro... File chrome/browser/chromeos/cros_settings_names.cc (right): http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros_settings_names.cc:55: // Parts of the ScreenSaver proto. Define the extension ID of the screen saver On 2012/03/01 19:58:10, Mattias Nissler wrote: > s/Define/Defines/ Done. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros_settings_names.cc:60: // Parts of the ForcedLogoutTimeouts proto. Define the timeouts before a user On 2012/03/01 19:58:10, Mattias Nissler wrote: > s/Define/Defines/ Done. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/dev... File chrome/browser/chromeos/device_settings_provider.cc (right): http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:282: // intended to be customizable by the user yet: On 2012/03/01 19:58:10, Mattias Nissler wrote: > The yet is not accurate. Put it in parens at least. Done. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:346: // -------------------- On 2012/03/01 19:58:10, Mattias Nissler wrote: > Instead of doing this, how about actually breaking the code up into functions? > Even better, maybe break it into logical groups instead of by type? Done. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... File chrome/browser/policy/proto/chrome_device_policy.proto (right): http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:116: // If this field is omitted or set to 0, no idle timeout will be performed. On 2012/03/01 19:58:10, Mattias Nissler wrote: > "no idle timeout will be performed": Not clear what this means? I guess we don't > do idle logouts in that case? Done. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:120: // This field is only used if |idle_logout_timeout| is specified. On 2012/03/01 19:58:10, Mattias Nissler wrote: > not quite accurate, should be if |idle_logut_timeout| > 0. Done. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:125: // Specifies the extension ID which is to be used as a screen saver. On 2012/03/01 19:58:10, Mattias Nissler wrote: > We should point out that this screen saver applies to pre-login only. Somebody > who doesn't have context will believe this is the session screensaver. Done. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:127: // Specified in milliseconds the timeout before the screen saver is activated On 2012/03/01 19:58:10, Mattias Nissler wrote: > grammar Done. http://codereview.chromium.org/9423043/diff/11001/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:128: // on the login screen if no user activity present. Only respected if the On 2012/03/01 19:58:10, Mattias Nissler wrote: > oh, so here it says login screen. Can we move that to a comment on the message? Done.
lgtm, just a couple of style nits. Try running this through linux_chromeos_clang too before landing. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... File chrome/browser/chromeos/device_settings_provider.cc (right): http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:393: policy.screen_saver().screen_saver_timeout()); Nit: indent http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:410: if (policy.has_screen_saver() && Nit: put both screen_saver policies together http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:462: new_values_cache->SetBoolean(kReportDeviceVersionInfo, Nit: move the first arg to a newline too, like in other places in this file http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:468: new_values_cache->SetBoolean(kReportDeviceBootMode, Nit: same here http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:479: policy.metrics_enabled().metrics_enabled()); Nit: indent http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:490: policy.release_channel().release_channel()); Nit: indent http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... File chrome/browser/chromeos/device_settings_provider.h (right): http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.h:19: #include "chrome/browser/policy/proto/chrome_device_policy.pb.h" Nit: alphabetical order http://codereview.chromium.org/9423043/diff/17002/chrome/browser/policy/proto... File chrome/browser/policy/proto/chrome_device_policy.proto (right): http://codereview.chromium.org/9423043/diff/17002/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:113: // All timeouts are specified in milliseconds. These policies are only valid in Kiosk mode too, right? If so, mention it in this comment. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:117: optional int64 idle_logout_timeout = 1; Nit: newline after this line. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:128: optional string screen_saver_extension_id = 1; Nit: newline after this line. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:151: optional ScreenSaverProto screen_saver = 16; Suggestion: call this login_screen_saver instead?
Fixed all nits. Clang bot is happy so I'll click the commit checkbox now. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... File chrome/browser/chromeos/device_settings_provider.cc (right): http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:393: policy.screen_saver().screen_saver_timeout()); On 2012/03/05 17:40:24, Joao da Silva wrote: > Nit: indent Done. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:410: if (policy.has_screen_saver() && On 2012/03/05 17:40:24, Joao da Silva wrote: > Nit: put both screen_saver policies together Done. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:462: new_values_cache->SetBoolean(kReportDeviceVersionInfo, On 2012/03/05 17:40:24, Joao da Silva wrote: > Nit: move the first arg to a newline too, like in other places in this file Done. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:468: new_values_cache->SetBoolean(kReportDeviceBootMode, On 2012/03/05 17:40:24, Joao da Silva wrote: > Nit: same here Done. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:479: policy.metrics_enabled().metrics_enabled()); On 2012/03/05 17:40:24, Joao da Silva wrote: > Nit: indent Done. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.cc:490: policy.release_channel().release_channel()); On 2012/03/05 17:40:24, Joao da Silva wrote: > Nit: indent Done. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... File chrome/browser/chromeos/device_settings_provider.h (right): http://codereview.chromium.org/9423043/diff/17002/chrome/browser/chromeos/dev... chrome/browser/chromeos/device_settings_provider.h:19: #include "chrome/browser/policy/proto/chrome_device_policy.pb.h" On 2012/03/05 17:40:24, Joao da Silva wrote: > Nit: alphabetical order Done. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/policy/proto... File chrome/browser/policy/proto/chrome_device_policy.proto (right): http://codereview.chromium.org/9423043/diff/17002/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:113: // All timeouts are specified in milliseconds. On 2012/03/05 17:40:24, Joao da Silva wrote: > These policies are only valid in Kiosk mode too, right? If so, mention it in > this comment. I think we discussed that those policies might be used for normal logins too at some point so I'd rather keep this description more general. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:117: optional int64 idle_logout_timeout = 1; On 2012/03/05 17:40:24, Joao da Silva wrote: > Nit: newline after this line. Done. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:128: optional string screen_saver_extension_id = 1; On 2012/03/05 17:40:24, Joao da Silva wrote: > Nit: newline after this line. Done. http://codereview.chromium.org/9423043/diff/17002/chrome/browser/policy/proto... chrome/browser/policy/proto/chrome_device_policy.proto:151: optional ScreenSaverProto screen_saver = 16; On 2012/03/05 17:40:24, Joao da Silva wrote: > Suggestion: call this login_screen_saver instead? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/9423043/25001
Change committed as 125159 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
