Add device status reports to policy requests.

Add device status reports to policy requests.

BUG=chromium-os:22035
TEST=


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

[Patrick Dubroy, 2011-11-25 13:47:17]

Hi Mattias,

Can you take a look at this? I still want to add at least a basic unit test, and
probably add a bit more info to the device status reporter, but let me know if
you think the architecture is ok.

[Mattias Nissler (ping if slow), 2011-11-25 14:15:19]

Architecture looks good. Unit tests for the status collector would indeed be
good to have :)

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/browser_p...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.cc:11: #include
"chrome/browser/browser_process.h"
needed?

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

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.cc:277: policy_request,
device_status, this);
One thing: We should reset our status counters after this request has succeeded.
Actually, we should probably reset right away in order to not lose state info
generated while the request is in flight...

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

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.h:14: namespace
enterprise_management { class DeviceStatusReportRequest; }
needed?

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

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_data_store.cc:36:
device_status_reporter_(g_browser_process->local_state()) {}
Please pass in the reference to local state so easier mocking in tests.

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

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_data_store.h:97: DeviceStatusReporter*
device_status_reporter();
Since this is not actively reporting stuff, maybe rename to
DeviceStatusCollector?

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/device_st...
File chrome/browser/policy/device_status_reporter.cc (right):

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_reporter.cc:20: const unsigned int
kIdleStateThresholdSeconds = 300;
no indentation

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_reporter.cc:86:
active_periods->Append(Value::CreateIntegerValue(end_offset));
we should probably limit the maximum number of entries we're willing to put into
the list.

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_reporter.cc:107:
MessageLoop::current()->PostDelayedTask(
You might want to use base/timer.h instead, which gives you periodic callbacks
without having to worry about MessageLoops and stuff.

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_reporter.cc:128: // Something is amiss --
move to the next pair.
If this happens, we might as well clear the list to get to a known good state?

[Patrick Dubroy, 2011-11-29 18:01:46]

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/browser_p...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.cc:11: #include
"chrome/browser/browser_process.h"
On 2011/11/25 14:15:19, Mattias Nissler wrote:
> needed?

Nope. Done.

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

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.cc:277: policy_request,
device_status, this);
On 2011/11/25 14:15:19, Mattias Nissler wrote:
> One thing: We should reset our status counters after this request has
succeeded.
> Actually, we should probably reset right away in order to not lose state info
> generated while the request is in flight...

Yeah, I was sure whether I should reset the data as soon as its collected, or
once the request succeeds. In either case, we could lose data.

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

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.h:14: namespace
enterprise_management { class DeviceStatusReportRequest; }
On 2011/11/25 14:15:19, Mattias Nissler wrote:
> needed?

Nope. Done.

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

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_data_store.cc:36:
device_status_reporter_(g_browser_process->local_state()) {}
On 2011/11/25 14:15:19, Mattias Nissler wrote:
> Please pass in the reference to local state so easier mocking in tests.

Decided to leave this as-is.

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

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_data_store.h:97: DeviceStatusReporter*
device_status_reporter();
On 2011/11/25 14:15:19, Mattias Nissler wrote:
> Since this is not actively reporting stuff, maybe rename to
> DeviceStatusCollector?

Done.

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/device_st...
File chrome/browser/policy/device_status_reporter.cc (right):

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_reporter.cc:20: const unsigned int
kIdleStateThresholdSeconds = 300;
On 2011/11/25 14:15:19, Mattias Nissler wrote:
> no indentation

Done.

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_reporter.cc:107:
MessageLoop::current()->PostDelayedTask(
On 2011/11/25 14:15:19, Mattias Nissler wrote:
> You might want to use base/timer.h instead, which gives you periodic callbacks
> without having to worry about MessageLoops and stuff.

Done.

http://codereview.chromium.org/8702009/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_reporter.cc:128: // Something is amiss --
move to the next pair.
On 2011/11/25 14:15:19, Mattias Nissler wrote:
> If this happens, we might as well clear the list to get to a known good state?

Done.

[Mattias Nissler (ping if slow), 2011-11-30 12:44:29]

The unit tests are good! In general looks good, just some minor things.

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_backend_impl.h (right):

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend_impl.h:78: const
em::DeviceStatusReportRequest& device_status,
Maybe we want to pass this as a pointer, so we can pass NULL if we don't want to
upload status data? That'd at least be helpful for the tests you had to modify
:)

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:75: // it's reasonsable to stop
recording the status.
What if somebody is on vacation for 2 months and doesn't use their device? Or,
in education, the device isn't used during the summer months?

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:78: out_val =
(int)offset_millis;
space after )

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:171: // periodically, the times
will overflow.
It seems likes maintaining the baseline time makes this all a bit complex. Can't
we rather just store the times as strings? That's what PrefService::SetInt64
does.

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_status_collector.h (right):

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.h:23: // Collects and reports the
status of an enterprised-managed ChromeOS device.
doesn't actually do the reporting.

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.h:34: virtual unsigned int
max_stored_active_periods() const;
would a local variable and a setter that tests can use also do? That'd be less
intrusive. Memory-wise, it's a vtable entry vs. an additional member, so no
difference since we have only one instance.

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector_unittest.cc:66: }
// namespace

[Patrick Dubroy, 2011-12-06 14:30:36]

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_backend_impl.h (right):

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend_impl.h:78: const
em::DeviceStatusReportRequest& device_status,
On 2011/11/30 12:44:29, Mattias Nissler wrote:
> Maybe we want to pass this as a pointer, so we can pass NULL if we don't want
to
> upload status data? That'd at least be helpful for the tests you had to modify
> :)

Done.

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:75: // it's reasonsable to stop
recording the status.
On 2011/11/30 12:44:29, Mattias Nissler wrote:
> What if somebody is on vacation for 2 months and doesn't use their device? Or,
> in education, the device isn't used during the summer months?

This doesn't matter anymore -- the method has been removed.

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:78: out_val =
(int)offset_millis;
On 2011/11/30 12:44:29, Mattias Nissler wrote:
> space after )

Done.

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:171: // periodically, the times
will overflow.
On 2011/11/30 12:44:29, Mattias Nissler wrote:
> It seems likes maintaining the baseline time makes this all a bit complex.
Can't
> we rather just store the times as strings? That's what PrefService::SetInt64
> does.

You're right. It's simpler to store as strings. Done.

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_status_collector.h (right):

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.h:23: // Collects and reports the
status of an enterprised-managed ChromeOS device.
On 2011/11/30 12:44:29, Mattias Nissler wrote:
> doesn't actually do the reporting.

Done.

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.h:34: virtual unsigned int
max_stored_active_periods() const;
On 2011/11/30 12:44:29, Mattias Nissler wrote:
> would a local variable and a setter that tests can use also do? That'd be less
> intrusive. Memory-wise, it's a vtable entry vs. an additional member, so no
> difference since we have only one instance.

Good idea -- done.

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/8702009/diff/5001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector_unittest.cc:66: }
On 2011/11/30 12:44:29, Mattias Nissler wrote:
> // namespace

Done.

[Mattias Nissler (ping if slow), 2011-12-06 15:20:17]

LGTM with nits.

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
chrome/browser/policy/device_management_backend_impl.cc:420:
request_wrapper.mutable_device_status_report_request()->CopyFrom(
You should check whether device_status is NULL here and if so, ignore it.

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
chrome/browser/policy/device_status_collector.cc:54: if (MessageLoop::current())
{
That's a poor way of detecting whether this is a unit test :) Can't you just add
a message loop to the unit test? The task won't run, but as far as I understand,
that should be OK for the tests.

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
chrome/browser/policy/device_status_collector_unittest.cc:225: }
// namespace policy

[Patrick Dubroy, 2011-12-06 16:41:57]

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
chrome/browser/policy/device_management_backend_impl.cc:420:
request_wrapper.mutable_device_status_report_request()->CopyFrom(
On 2011/12/06 15:20:18, Mattias Nissler wrote:
> You should check whether device_status is NULL here and if so, ignore it.

Whoops :-) Done.

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
chrome/browser/policy/device_status_collector.cc:54: if (MessageLoop::current())
{
On 2011/12/06 15:20:18, Mattias Nissler wrote:
> That's a poor way of detecting whether this is a unit test :) Can't you just
add
> a message loop to the unit test? The task won't run, but as far as I
understand,
> that should be OK for the tests.

Done.

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/8702009/diff/12009/chrome/browser/policy/devic...
chrome/browser/policy/device_status_collector_unittest.cc:225: }
On 2011/12/06 15:20:18, Mattias Nissler wrote:
> // namespace policy

Done.

[commit-bot: I haz the power, 2011-12-08 09:24:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/8702009/20001

[commit-bot: I haz the power, 2011-12-08 09:50:26]

Try job failure for 8702009-20001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Patrick Dubroy, 2011-12-08 14:15:43]

Mattias, can you take another quick look at this?

[Mattias Nissler (ping if slow), 2011-12-08 14:23:36]

LGTM

[commit-bot: I haz the power, 2011-12-09 10:14:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/8702009/23012

[commit-bot: I haz the power, 2011-12-09 11:40:52]

Change committed as 113791