Send policy blobs to session_manager

Send policy blobs to session_manager

And also read policy back from session_manager into CloudPolicyCache, but there are no consumers of those values yet.


BUG=chromium-os:11258
TEST=UserPolicyCacheTest.*; DevicePolicyCacheTest.*

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

[Jakob Kummerow, 2011-03-23 18:20:12]

Chris: Please review the changes to signed_settings_helper.{h,cc}.

Mattias: Please review the other changes. They're less intimidating than they
look at first sight:

CloudPolicyCache has been renamed to CloudPolicyCacheBase. Some parts of it have
been split out into UserPolicyCache and DevicePolicyCache. 
cloud_policy_cache_unittest.cc has had its second half deleted (obsolete), the
still relevant first half has been moved to user_policy_cache_unittest. 
device_policy_cache_unittest is new.
The other changes are minimal and straightforward.

[Chris Masone, 2011-03-23 22:54:18]

SignedSettingsHelper stuff LGTM

On 2011/03/23 18:20:12, jkummerow wrote:
> Chris: Please review the changes to signed_settings_helper.{h,cc}.
> 
> Mattias: Please review the other changes. They're less intimidating than they
> look at first sight:
> 
> CloudPolicyCache has been renamed to CloudPolicyCacheBase. Some parts of it
have
> been split out into UserPolicyCache and DevicePolicyCache. 
> cloud_policy_cache_unittest.cc has had its second half deleted (obsolete), the
> still relevant first half has been moved to user_policy_cache_unittest. 
> device_policy_cache_unittest is new.
> The other changes are minimal and straightforward.

[Chris Masone, 2011-03-23 23:03:53]

http://codereview.chromium.org/6705031/diff/1/chrome/browser/chromeos/login/s...
File chrome/browser/chromeos/login/signed_settings_helper.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/chromeos/login/s...
chrome/browser/chromeos/login/signed_settings_helper.cc:243: virtual void
OnSettingsCompleted(SignedSettings::ReturnCode code,
On SettingsOpCompleted, no?

http://codereview.chromium.org/6705031/diff/1/chrome/browser/chromeos/login/s...
chrome/browser/chromeos/login/signed_settings_helper.cc:271: virtual void
OnSettingsCompleted(SignedSettings::ReturnCode code,
Again

[Mattias Nissler (ping if slow), 2011-03-24 18:55:57]

Looks pretty good already, but a few issues to resolve.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/chromeos/login/s...
File chrome/browser/chromeos/login/signed_settings_helper.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/chromeos/login/s...
chrome/browser/chromeos/login/signed_settings_helper.cc:243: virtual void
OnSettingsCompleted(SignedSettings::ReturnCode code,
On 2011/03/23 23:03:53, Chris Masone wrote:
> On SettingsOpCompleted, no?

Maybe this could use an OVERRIDE declaration then ;)

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_cache_base.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.cc:95: last_policy_refresh_time_ =
timestamp;
This does have the same bogus-timestamp bug we've just fixed for M11, right?

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:48: virtual void SetUnmanaged()
= 0;
Why is the setter virtual and getter not? Should we maybe move the is_unmanaged_
into the subclasses?

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:80: PolicyMap
recommended_policy_;
Does this need to be protected?

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:85: bool
initialization_complete_;
Same question here?

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:91: base::Time
last_policy_refresh_time_;
And here?

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:94:
ObserverList<ConfigurationPolicyProvider::Observer, true> observer_list_;
And here?

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:24: void
DecodeIntegerValue(google::protobuf::int64 value);
We shouldn't make use of this here, since this class has nothing to do with the
generated code.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:65: // TODO(jkummerow): error
handling?
Maybe make this comment a bit more elaborate, something like:

We cannot do anything about this, but we may want to notify the user that
something is wrong.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:79: // TODO(jkummerow): error
handling?
Same here.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:109: // TODO(jkummerow): define
kPolicyDevicePolicyRefreshRate.
So why not do it?

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:114: if
(policy.has_user_whitelist()) {
I guess we don't need the user whitelist in prefs at this point, but I guess it
doesn't hurt either. Anyway, let's make a decision before committing this.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:126: // TODO(jkummerow): remove
this when |mandatory->Set()| is enabled.
If list is empty, you still need to delete!

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
File chrome/browser/policy/device_policy_cache.h (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.h:30: virtual void Load();
Consider adding OVERRIDE declarations.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
File chrome/browser/policy/device_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache_unittest.cc:112:
EXPECT_GT(mandatory_policy(*cache_.get()).size(), 0U);
can just write *cache (also below)

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache_unittest.cc:145: // In particular,
want to test decoding a whitelist with 2+ elements.
So define the constants and write the test case?

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_cache.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.cc:97: PolicyMap mandatory_policy;
As discussed, we probably want to call SetPolicyInternal here. Gotta sort out
timestamp handling though...

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.cc:138: is_unmanaged_ = true;
This could probably also be moved to the base class, since it'll become relevant
for device policy sooner or later.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_cache.h (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.h:6: #define
CHROME_BROWSER_POLICY_USER_POLICY_CACHE_H_
#pragma once (also check other files please)

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.h:21: virtual void Load();
Consider adding OVERRIDE declarations

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.h:28: 
// CloudPolicyCacheBase implementation:

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.h:31: PolicyMap* recommended);
OVERRIDE?

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Any serious changes in here? Skipped for now.

http://codereview.chromium.org/6705031/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/common/chrome_switches.c...
chrome/common/chrome_switches.cc:449: // Enables device policy support on
ChromeOS.
Should probably have an #if defined(OS_CHROMEOS) then

[Jakob Kummerow, 2011-03-28 13:53:53]

Thanks for the comments.

Chris: This CL is now based on the updated SignedSettings method signatures.
Please have another look at SignedSettingsHelper (the changes are
straightforward, though).

http://codereview.chromium.org/6705031/diff/1/chrome/browser/chromeos/login/s...
File chrome/browser/chromeos/login/signed_settings_helper.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/chromeos/login/s...
chrome/browser/chromeos/login/signed_settings_helper.cc:243: virtual void
OnSettingsCompleted(SignedSettings::ReturnCode code,
On 2011/03/23 23:03:53, Chris Masone wrote:
> On SettingsOpCompleted, no?

Most definitely. Good catch. Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/chromeos/login/s...
chrome/browser/chromeos/login/signed_settings_helper.cc:243: virtual void
OnSettingsCompleted(SignedSettings::ReturnCode code,
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> On 2011/03/23 23:03:53, Chris Masone wrote:
> > On SettingsOpCompleted, no?
> 
> Maybe this could use an OVERRIDE declaration then ;)

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/chromeos/login/s...
chrome/browser/chromeos/login/signed_settings_helper.cc:271: virtual void
OnSettingsCompleted(SignedSettings::ReturnCode code,
On 2011/03/23 23:03:53, Chris Masone wrote:
> Again

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_cache_base.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.cc:95: last_policy_refresh_time_ =
timestamp;
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> This does have the same bogus-timestamp bug we've just fixed for M11, right?

In this patch set: yes; it's already fixed in my local checkout. IOW, done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:48: virtual void SetUnmanaged()
= 0;
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> Why is the setter virtual and getter not? Should we maybe move the
is_unmanaged_
> into the subclasses?

Because what the setter needs to do depends on the subclass, whereas the
getter's behavior does not. I don't see an easy way around it. I managed to make
|is_unmanaged_| private, though.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:80: PolicyMap
recommended_policy_;
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> Does this need to be protected?

Done -- managed to make it private.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:85: bool
initialization_complete_;
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> Same question here?

UserPolicyCache needs to read it. I made the member private (style guide says
so) and provided a protected getter.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:91: base::Time
last_policy_refresh_time_;
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> And here?

Currently, both when this field is set and to which value is dependent on the
subclass. I made the member private and provided a protected setter.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:94:
ObserverList<ConfigurationPolicyProvider::Observer, true> observer_list_;
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> And here?

Done -- managed to make it private.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:24: void
DecodeIntegerValue(google::protobuf::int64 value);
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> We shouldn't make use of this here, since this class has nothing to do with
the
> generated code.

Done (has become a moot point since we've scratched decoding completely for
now).

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:65: // TODO(jkummerow): error
handling?
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> Maybe make this comment a bit more elaborate, something like:
> 
> We cannot do anything about this, but we may want to notify the user that
> something is wrong.

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:79: // TODO(jkummerow): error
handling?
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> Same here.

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:109: // TODO(jkummerow): define
kPolicyDevicePolicyRefreshRate.
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> So why not do it?

As discussed, let's remove all of this protobuf-decoding code for now, and add
it back once we know we need it.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:114: if
(policy.has_user_whitelist()) {
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> I guess we don't need the user whitelist in prefs at this point, but I guess
it
> doesn't hurt either. Anyway, let's make a decision before committing this.

As discussed, let's remove all of this protobuf-decoding code for now, and add
it back once we know we need it.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.cc:126: // TODO(jkummerow): remove
this when |mandatory->Set()| is enabled.
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> If list is empty, you still need to delete!

Good point. Solved by using a scoped_ptr.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
File chrome/browser/policy/device_policy_cache.h (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache.h:30: virtual void Load();
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> Consider adding OVERRIDE declarations.

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
File chrome/browser/policy/device_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache_unittest.cc:112:
EXPECT_GT(mandatory_policy(*cache_.get()).size(), 0U);
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> can just write *cache (also below)

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/device_po...
chrome/browser/policy/device_policy_cache_unittest.cc:145: // In particular,
want to test decoding a whitelist with 2+ elements.
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> So define the constants and write the test case?

As discussed, punted to a later CL.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_cache.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.cc:97: PolicyMap mandatory_policy;
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> As discussed, we probably want to call SetPolicyInternal here. Gotta sort out
> timestamp handling though...

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.cc:138: is_unmanaged_ = true;
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> This could probably also be moved to the base class, since it'll become
relevant
> for device policy sooner or later.

Done. Sort of. I've extracted most of the method body into a new method
SetUnmanagedInternal() in the base class.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_cache.h (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.h:6: #define
CHROME_BROWSER_POLICY_USER_POLICY_CACHE_H_
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> #pragma once (also check other files please)

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.h:21: virtual void Load();
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> Consider adding OVERRIDE declarations

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.h:28: 
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> // CloudPolicyCacheBase implementation:

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.h:31: PolicyMap* recommended);
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> OVERRIDE?

Done.

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> Any serious changes in here? Skipped for now.

• s/CloudPolicyCache/UserPolicyCache/g
• s/LoadFromFile()/Load()/g
• removed the second half of the original file (containing tests for old-style,
GenericNameValue based policy)

No other changes. So indeed no need for a thorough review.

http://codereview.chromium.org/6705031/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/6705031/diff/1/chrome/common/chrome_switches.c...
chrome/common/chrome_switches.cc:449: // Enables device policy support on
ChromeOS.
On 2011/03/24 18:55:57, Mattias Nissler wrote:
> Should probably have an #if defined(OS_CHROMEOS) then

Done (moved to ChromeOS section of the file).

[Chris Masone, 2011-03-28 20:57:21]

SignedSettingsHelper stuff LGTM

[Mattias Nissler (ping if slow), 2011-03-29 08:58:39]

LGTM with a few nits.

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_base.cc (right):

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.cc:16: // CloudPolicyCache for
hooking up with ConfigurationPolicyPrefStore.
s/CloudPolicyCache/CloudPolicyCacheBase/

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.cc:83: timestamp =
&temp_timestamp;
Why not just DCHECK(timestamp)?

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.h:10: #include
"base/observer_list.h"
alphabetize

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.h:81: const
em::PolicyFetchResponse& policy_response,
Nit: I think this fits the previous line.

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:10: #include
"chrome/browser/policy/proto/device_management_backend.pb.h"
alphabetize.

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.h (right):

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.h:35: //
SignedSettingsHelper::Callback implementation
nit: colon here as well for consistency with line 30.

[Jakob Kummerow, 2011-03-29 09:33:37]

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_base.cc (right):

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.cc:16: // CloudPolicyCache for
hooking up with ConfigurationPolicyPrefStore.
On 2011/03/29 08:58:39, Mattias Nissler wrote:
> s/CloudPolicyCache/CloudPolicyCacheBase/

Done.

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.cc:83: timestamp =
&temp_timestamp;
On 2011/03/29 08:58:39, Mattias Nissler wrote:
> Why not just DCHECK(timestamp)?

Because we need a non-NULL |timestamp| to perform what
|check_for_timestamp_validity| instructs us to do, but I thought it'd be nice if
this implementation detail was hidden from callers. This way, they can pass in
NULL for |timestamp| if they don't care, and this method works correctly for all
possible combinations of inputs.

I can see a less "hacky" way to do it: always use |temp_timestamp| in the call
to DecodePolicyResponse below, and then do:
if (timestamp) { *timestamp = temp_timestamp; }
Would you prefer that?

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.h:10: #include
"base/observer_list.h"
On 2011/03/29 08:58:39, Mattias Nissler wrote:
> alphabetize

Done.

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.h:81: const
em::PolicyFetchResponse& policy_response,
On 2011/03/29 08:58:39, Mattias Nissler wrote:
> Nit: I think this fits the previous line.

Done.

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:10: #include
"chrome/browser/policy/proto/device_management_backend.pb.h"
On 2011/03/29 08:58:39, Mattias Nissler wrote:
> alphabetize.

Done.

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.h (right):

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.h:35: //
SignedSettingsHelper::Callback implementation
On 2011/03/29 08:58:39, Mattias Nissler wrote:
> nit: colon here as well for consistency with line 30.

Done.

[Mattias Nissler (ping if slow), 2011-03-29 10:25:48]

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_base.cc (right):

http://codereview.chromium.org/6705031/diff/10001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.cc:83: timestamp =
&temp_timestamp;
On 2011/03/29 09:33:37, jkummerow wrote:
> On 2011/03/29 08:58:39, Mattias Nissler wrote:
> > Why not just DCHECK(timestamp)?
> 
> Because we need a non-NULL |timestamp| to perform what
> |check_for_timestamp_validity| instructs us to do, but I thought it'd be nice
if
> this implementation detail was hidden from callers. This way, they can pass in
> NULL for |timestamp| if they don't care, and this method works correctly for
all
> possible combinations of inputs.
> 
> I can see a less "hacky" way to do it: always use |temp_timestamp| in the call
> to DecodePolicyResponse below, and then do:
> if (timestamp) { *timestamp = temp_timestamp; }
> Would you prefer that?

This is a nit, so I'd be happy either way.

My point was this: Why support passing NULL pointers at all? Doesn't hurt the
callers to just have a dummy timestamp local they don't look at and makes the
code here much cleaner.