Update RestoreOnStartupPolicyHandler to translate to the correct preferences

Update RestoreOnStartupPolicyHandler to translate to the correct preferences

BUG=124027
TEST=pyauto:policy.PolicyTest.testStartupOptionsHomepage


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

[Tyler Breisacher (Chromium), 2012-04-18 22:00:06]

As suggested by mnissler in https://chromiumcodereview.appspot.com/10049005/ --
I think if we commit this CL first then we can simplify the other one and commit
it second, without breaking anything or adding a lot of unnecessary complexity.

[Nirnimesh, 2012-04-18 23:11:31]

chrome/test/functional LGTM

[Mattias Nissler (ping if slow), 2012-04-19 13:15:55]

Looks mostly good. One request: Please update the policy definition in
chrome/app/policy_templates.json to reflect the changes to the valid values of
the RestoreOnStartup policy.

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:980:
SimplePolicyHandler::ApplyPolicySettings(policies, prefs);
This would be just prefs->SetValue() if you don't derive from
SimplePolicyHandler, which I think would be easier to follow.

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:992: // enforcing what the
homepage should be. Don't set any prefs.
So the policy doesn't have the homepage_is_new_tab_page setting. In that case,
shouldn't we fall back to setting the default (which would be that the homepage
is the new tab page IIRC)?

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:999: DCHECK(result);
This DCHECK is inappropriate. The Value you get here comes directly from the
policy provider, which cannot enforce proper Value types in all cases. Thus, you
should handle this situation gracefully, probably by just ignoring the value.

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:1027: 
Ideally, you should add a check here that marks the key::kRestoreOnStartup
policy value deprecated. There already is IDS_POLICY_DEPRECATED, but that is for
the case when the entire policy is deprecated, so you might want to add another
constant IDS_POLICY_VALUE_DEPRECATED for the case that the policy is still
appropriate, but the specific value is deprecated.

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler.h (right):

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.h:319: PrefValueMap* prefs)
OVERRIDE;
Now that you also override ApplyPolicySettings, I'd prefer if you derived this
class from TypeCheckingPolicyHandler (which will make it clearer to the reader
what prefs this actually manipulates).

[Tyler Breisacher (Chromium), 2012-04-19 19:07:56]

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:980:
SimplePolicyHandler::ApplyPolicySettings(policies, prefs);
On 2012/04/19 13:15:55, Mattias Nissler wrote:
> This would be just prefs->SetValue() if you don't derive from
> SimplePolicyHandler, which I think would be easier to follow.

Done.

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:992: // enforcing what the
homepage should be. Don't set any prefs.
On 2012/04/19 13:15:55, Mattias Nissler wrote:
> So the policy doesn't have the homepage_is_new_tab_page setting. In that case,
> shouldn't we fall back to setting the default (which would be that the
homepage
> is the new tab page IIRC)?

In Chrome 18, if a policy has RestoreOnStartup=0 and doesn't have
HomePageIsNewTab set at all, the user would still be free to choose their
homepage. Then presumably the admin setting the policy would notice that and
explicitly set a HomePageIsNewTab value (and probably a HomepageLocation value
too) so this would be a non-issue for most people. But in case they didn't, I
think we probably want the policy to have the same effect it had in the previous
version.

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:999: DCHECK(result);
On 2012/04/19 13:15:55, Mattias Nissler wrote:
> This DCHECK is inappropriate. The Value you get here comes directly from the
> policy provider, which cannot enforce proper Value types in all cases. Thus,
you
> should handle this situation gracefully, probably by just ignoring the value.

Done. (Also did the same thing above for homepage_is_new_tab_page_value)

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:1027: 
On 2012/04/19 13:15:55, Mattias Nissler wrote:
> Ideally, you should add a check here that marks the key::kRestoreOnStartup
> policy value deprecated. There already is IDS_POLICY_DEPRECATED, but that is
for
> the case when the entire policy is deprecated, so you might want to add
another
> constant IDS_POLICY_VALUE_DEPRECATED for the case that the policy is still
> appropriate, but the specific value is deprecated.

Done, but I'm pretty sure we'll want to merge this to the m19 branch. Is it too
late to add new strings?

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler.h (right):

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.h:319: PrefValueMap* prefs)
OVERRIDE;
On 2012/04/19 13:15:55, Mattias Nissler wrote:
> Now that you also override ApplyPolicySettings, I'd prefer if you derived this
> class from TypeCheckingPolicyHandler (which will make it clearer to the reader
> what prefs this actually manipulates).

Done.

http://codereview.chromium.org/10035043/diff/17001/chrome/test/functional/pol...
File chrome/test/functional/policy_test_cases.py (right):

http://codereview.chromium.org/10035043/diff/17001/chrome/test/functional/pol...
chrome/test/functional/policy_test_cases.py:275: 'ReportDeviceLocation': (None,
False, [], ['chromeos']),
Had to add these because the presubmit complains that they exist in
policy_templates.json but not here.

[Mattias Nissler (ping if slow), 2012-04-20 09:20:24]

LGTM (I put another comment requesting something, feel free to ignore if I'm
taking too much of your time. If you decide to go for it, I'd like to take
another look).

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:992: // enforcing what the
homepage should be. Don't set any prefs.
On 2012/04/19 19:07:56, Tyler Breisacher wrote:
> On 2012/04/19 13:15:55, Mattias Nissler wrote:
> > So the policy doesn't have the homepage_is_new_tab_page setting. In that
case,
> > shouldn't we fall back to setting the default (which would be that the
> homepage
> > is the new tab page IIRC)?
> 
> In Chrome 18, if a policy has RestoreOnStartup=0 and doesn't have
> HomePageIsNewTab set at all, the user would still be free to choose their
> homepage. Then presumably the admin setting the policy would notice that and
> explicitly set a HomePageIsNewTab value (and probably a HomepageLocation value
> too) so this would be a non-issue for most people. But in case they didn't, I
> think we probably want the policy to have the same effect it had in the
previous
> version.

You're right.

http://codereview.chromium.org/10035043/diff/4003/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:1027: 
On 2012/04/19 19:07:56, Tyler Breisacher wrote:
> On 2012/04/19 13:15:55, Mattias Nissler wrote:
> > Ideally, you should add a check here that marks the key::kRestoreOnStartup
> > policy value deprecated. There already is IDS_POLICY_DEPRECATED, but that is
> for
> > the case when the entire policy is deprecated, so you might want to add
> another
> > constant IDS_POLICY_VALUE_DEPRECATED for the case that the policy is still
> > appropriate, but the specific value is deprecated.
> 
> Done, but I'm pretty sure we'll want to merge this to the m19 branch. Is it
too
> late to add new strings?

Yes, it is too late. I'm OK with merging an adjusted patch that doesn't have the
additional error reporting.

http://codereview.chromium.org/10035043/diff/17001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/17001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:981: if (!result)
nit: how about dropping the result local and just use the expression above as
condition?

http://codereview.chromium.org/10035043/diff/17001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1007: if (!result)
nit: could drop the result local here as well.

http://codereview.chromium.org/10035043/diff/17001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1042: if (result) {
nit: same thing

http://codereview.chromium.org/10035043/diff/17001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1069: }
Another small request while you're at it: It would be useful to add an else
branch here that checks whether the value is valid at all and report
IDS_POLICY_OUT_OF_RANGE_ERROR.

http://codereview.chromium.org/10035043/diff/17001/chrome/test/functional/pol...
File chrome/test/functional/policy_test_cases.py (right):

http://codereview.chromium.org/10035043/diff/17001/chrome/test/functional/pol...
chrome/test/functional/policy_test_cases.py:275: 'ReportDeviceLocation': (None,
False, [], ['chromeos']),
On 2012/04/19 19:07:56, Tyler Breisacher wrote:
> Had to add these because the presubmit complains that they exist in
> policy_templates.json but not here.

Yes, correct. Thanks!

[Tyler Breisacher (Chromium), 2012-04-20 18:39:43]

http://codereview.chromium.org/10035043/diff/17001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/17001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:981: if (!result)
On 2012/04/20 09:20:24, Mattias Nissler wrote:
> nit: how about dropping the result local and just use the expression above as
> condition?

I think that's more consistent with Chrome code, thanks. Done.

http://codereview.chromium.org/10035043/diff/17001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1007: if (!result)
On 2012/04/20 09:20:24, Mattias Nissler wrote:
> nit: could drop the result local here as well.

Done.

http://codereview.chromium.org/10035043/diff/17001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1042: if (result) {
On 2012/04/20 09:20:24, Mattias Nissler wrote:
> nit: same thing

Done.

http://codereview.chromium.org/10035043/diff/17001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1069: }
On 2012/04/20 09:20:24, Mattias Nissler wrote:
> Another small request while you're at it: It would be useful to add an else
> branch here that checks whether the value is valid at all and report
> IDS_POLICY_OUT_OF_RANGE_ERROR.

Done.

http://codereview.chromium.org/10035043/diff/29001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/29001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1014: if (!homepage_value
|| !homepage_value->IsType(TYPE_STRING)) {
I noticed that we weren't type-checking this value, since we never extract the
string itself from the Value. (I could extract the string, but I would just have
to wrap it back up into a value again, which is why I did DeepCopy at line 1020
instead.)

I can't decide which one feels clearer, so I'm good with either one:

if (!homepage_value || !homepage_value->IsType(TYPE_STRING)) {
  or
if (!(homepage_value && homepage_value->IsType(TYPE_STRING))) {

http://codereview.chromium.org/10035043/diff/29001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1043: case
SessionStartupPref::kPrefValueLast: {
The braces here are necessary because we're declaring variables inside this
case. The style guide doesn't seem to say where to place the braces, but it
looks like there's plenty of other code doing this:
http://code.google.com/p/chromium/source/search?q=case.*%3A%5Cs%7B

[Mattias Nissler (ping if slow), 2012-04-23 09:40:46]

LGTM with nits (for most of which I leave the decision to you how to handle).

http://codereview.chromium.org/10035043/diff/29001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/29001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1014: if (!homepage_value
|| !homepage_value->IsType(TYPE_STRING)) {
On 2012/04/20 18:39:44, Tyler Breisacher wrote:
> I noticed that we weren't type-checking this value, since we never extract the
> string itself from the Value. (I could extract the string, but I would just
have
> to wrap it back up into a value again, which is why I did DeepCopy at line
1020
> instead.)
> 
> I can't decide which one feels clearer, so I'm good with either one:
> 
> if (!homepage_value || !homepage_value->IsType(TYPE_STRING)) {
>   or
> if (!(homepage_value && homepage_value->IsType(TYPE_STRING))) {

My personal preference is in favor of the former, which you currently have in
the code.

http://codereview.chromium.org/10035043/diff/29001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1070: // No error
nit: missing period.

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:983: if
(restore_on_startup == SessionStartupPref::kPrefValueHomePage) {
If you like, you can remove braces here. I'm fine either way.

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1008: prefs->SetInteger(
nit: could remove the line break after ( and align on (

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1021: prefs->SetInteger(
same here.

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1041: //
TODO(tbreisacher): AddError indicating this value is deprecated.
Ah, so you are planning to merge this CL without changes and then fix the TODO
in a follow-up? If so, fine with me.

[Tyler Breisacher (Chromium), 2012-04-23 17:54:03]

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:983: if
(restore_on_startup == SessionStartupPref::kPrefValueHomePage) {
On 2012/04/23 09:40:46, Mattias Nissler wrote:
> If you like, you can remove braces here. I'm fine either way.

Done.

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1008: prefs->SetInteger(
On 2012/04/23 09:40:46, Mattias Nissler wrote:
> nit: could remove the line break after ( and align on (

Done.

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1021: prefs->SetInteger(
On 2012/04/23 09:40:46, Mattias Nissler wrote:
> same here.

Done.

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:1041: //
TODO(tbreisacher): AddError indicating this value is deprecated.
On 2012/04/23 09:40:46, Mattias Nissler wrote:
> Ah, so you are planning to merge this CL without changes and then fix the TODO
> in a follow-up? If so, fine with me.

Yes.

[csilv, 2012-04-23 17:57:26]

lgtm

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:994: 
nit, kill this blank line

[Tyler Breisacher (Chromium), 2012-04-23 18:00:11]

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/10035043/diff/33001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.cc:994: 
On 2012/04/23 17:57:26, csilv wrote:
> nit, kill this blank line

Done.

[commit-bot: I haz the power, 2012-04-23 18:00:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/10035043/42001

[commit-bot: I haz the power, 2012-04-23 20:10:40]

Change committed as 133504