Aggregate device activity, and report per-day activity in device status reports.

Aggregate device activity, and report per-day activity in device status reports.

BUG=chromium-os:26372
TEST=Manual.


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

[Patrick Dubroy, 2012-02-14 21:34:03]

Bin & Zhen:

Here's my CL to aggregate device usage data on the client side. I still want to
add a few tests, but should be able to land this tomorrow or the next day.

I named the new message "ActiveTimePeriod" rather than "TimePeriodUsageDuration"
-- does that sound ok to you? Otherwise, I think it's just as we discussed.

Also, I'm resetting the usage counters whenever that data is uploaded -- is that
what you'd like? I.e., if we successfully upload device status every 3 hours,
then the maximum value for the current day's usage is 3 hours, and the data for
a particular day would be added up on the server side. Let me know if you'd like
me to do it differently.

[zhen_google.com, 2012-02-14 22:30:12]

Looks all right.

Could user move clock further back than 2 days?
const unsigned int kMaxStoredFutureActivityDays = 2;

thanks
zhen

On Tue, Feb 14, 2012 at 1:34 PM,  <dubroy@chromium.org> wrote:
> Reviewers: ,
>
> Message:
> Bin & Zhen:
>
> Here's my CL to aggregate device usage data on the client side. I still want
> to
> add a few tests, but should be able to land this tomorrow or the next day.
>
> I named the new message "ActiveTimePeriod" rather than
> "TimePeriodUsageDuration"
> -- does that sound ok to you? Otherwise, I think it's just as we discussed.
>
> Also, I'm resetting the usage counters whenever that data is uploaded -- is
> that
> what you'd like? I.e., if we successfully upload device status every 3
> hours,
> then the maximum value for the current day's usage is 3 hours, and the data
> for
> a particular day would be added up on the server side. Let me know if you'd
> like
> me to do it differently.
>
> Description:
> Aggregate device activity, and report per-day activity in device status
> reports.
>
> BUG=chromium-os:26372
> TEST=Manual.
>
>
> Please review this at http://codereview.chromium.org/9348105/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/chromeos/device_settings_provider.cc
>  M chrome/browser/policy/device_status_collector.h
>  M chrome/browser/policy/device_status_collector.cc
>  M chrome/browser/policy/device_status_collector_unittest.cc
>  M chrome/browser/policy/proto/device_management_backend.proto
>
>

[Patrick Dubroy, 2012-02-15 13:48:33]

Mattias, can you please review?

[Mattias Nissler (ping if slow), 2012-02-15 14:56:21]

Mostly good, but a number of nits to fix.

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/chrom...
File chrome/browser/chromeos/device_settings_provider.cc (right):

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/chrom...
chrome/browser/chromeos/device_settings_provider.cc:435: // to notify the user
has been implemented (http://crosbug.com/26252).
Umm... I guess you want to change this :)

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
File chrome/browser/policy/device_status_collector.cc (right):

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
chrome/browser/policy/device_status_collector.cc:39: DictionaryValue*
activity_times, Time day_midnight, TimeDelta activity) {
If you break the declaration, you need to put all parameters on a separate line.

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
chrome/browser/policy/device_status_collector.cc:42: (day_midnight -
Time::UnixEpoch()).InMilliseconds();
indentation

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
chrome/browser/policy/device_status_collector.cc:49: 
remove extra blank line

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
chrome/browser/policy/device_status_collector.cc:146: Time epoch =
Time::UnixEpoch();
declare const.

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
chrome/browser/policy/device_status_collector.cc:153: if
(base::StringToInt64(*it, &timestamp)) {
We should probably also remove it if we cannot parse the entry.

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
chrome/browser/policy/device_status_collector.cc:215: int64 milliseconds_per_day
= TimeDelta::FromDays(1).InMilliseconds();
declare const

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
File chrome/browser/policy/device_status_collector.h (right):

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
chrome/browser/policy/device_status_collector.h:64: // Cleans up the locally
stored activity periods if there are too many.
s/if there are too many/that fall outside of the activity collection window/ or
something alike.

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
chrome/browser/policy/device_status_collector_unittest.cc:332: EXPECT_EQ(2,
status_.active_period_size());
We should check that we get correct times for both of the days.

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
File chrome/browser/policy/proto/device_management_backend.proto (right):

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
chrome/browser/policy/proto/device_management_backend.proto:262: // No longer
used -- use daily_activity instead.
I guess you mean active_period?

https://chromiumcodereview.appspot.com/9348105/diff/3001/chrome/browser/polic...
chrome/browser/policy/proto/device_management_backend.proto:268: repeated
ActiveTimePeriod active_period = 6;
Could use a comment saying that this is a map of days to aggregated active
periods.

[Patrick Dubroy, 2012-02-15 15:37:24]

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:435: // to notify the user
has been implemented (http://crosbug.com/26252).
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> Umm... I guess you want to change this :)

N/A.

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

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:39: DictionaryValue*
activity_times, Time day_midnight, TimeDelta activity) {
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> If you break the declaration, you need to put all parameters on a separate
line.

Done.

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:42: (day_midnight -
Time::UnixEpoch()).InMilliseconds();
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:49: 
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> remove extra blank line

Done.

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:146: Time epoch =
Time::UnixEpoch();
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> declare const.

Done.

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:153: if
(base::StringToInt64(*it, &timestamp)) {
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> We should probably also remove it if we cannot parse the entry.

Done.

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:215: int64 milliseconds_per_day
= TimeDelta::FromDays(1).InMilliseconds();
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> declare const

Done.

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

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.h:64: // Cleans up the locally
stored activity periods if there are too many.
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> s/if there are too many/that fall outside of the activity collection window/
or
> something alike.

Done.

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

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector_unittest.cc:332: EXPECT_EQ(2,
status_.active_period_size());
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> We should check that we get correct times for both of the days.

Done.

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/proto/...
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_backend.proto:262: // No longer
used -- use daily_activity instead.
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> I guess you mean active_period?

Done.

http://codereview.chromium.org/9348105/diff/3001/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_backend.proto:268: repeated
ActiveTimePeriod active_period = 6;
On 2012/02/15 14:56:21, Mattias Nissler wrote:
> Could use a comment saying that this is a map of days to aggregated active
> periods.

Done.

[Mattias Nissler (ping if slow), 2012-02-15 16:14:47]

more nits

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

http://codereview.chromium.org/9348105/diff/3004/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:35: const char* const
kPrefDeviceActivityTimes = "device_status.activity_times";
This should go into pref_names.h/cc

http://codereview.chromium.org/9348105/diff/3004/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:38: void AddDeviceActivity(
nit: I think you can remove the line break after the opening parenthesis.

http://codereview.chromium.org/9348105/diff/3004/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:229:
period->set_end_timestamp(start_timestamp + milliseconds_per_day);
Technically, this is incorrect for days with leap seconds. I'm not sure whether
our Time class handles that correctly, but to give it a chance, we could convert
|start_timestamp| back to a base::Time object, explode it, increment the day,
and the implode and convert back to a timestamp.

[Patrick Dubroy, 2012-02-16 09:56:30]

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

http://codereview.chromium.org/9348105/diff/3004/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:35: const char* const
kPrefDeviceActivityTimes = "device_status.activity_times";
On 2012/02/15 16:14:47, Mattias Nissler wrote:
> This should go into pref_names.h/cc

Done.

http://codereview.chromium.org/9348105/diff/3004/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:38: void AddDeviceActivity(
On 2012/02/15 16:14:47, Mattias Nissler wrote:
> nit: I think you can remove the line break after the opening parenthesis.

Done.

http://codereview.chromium.org/9348105/diff/3004/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:229:
period->set_end_timestamp(start_timestamp + milliseconds_per_day);
On 2012/02/15 16:14:47, Mattias Nissler wrote:
> Technically, this is incorrect for days with leap seconds. I'm not sure
whether
> our Time class handles that correctly, but to give it a chance, we could
convert
> |start_timestamp| back to a base::Time object, explode it, increment the day,
> and the implode and convert back to a timestamp.

It's actually correct, because when a leap second occurs, two consecutive
seconds have the same timestamp.

[Mattias Nissler (ping if slow), 2012-02-16 10:06:43]

LGTM!

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

http://codereview.chromium.org/9348105/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_status_collector.cc:104:
prefs::kDeviceActivityTimes, new DictionaryValue);
style nit: I think you could reformat the argument list so 
prefs::kDeviceActivityTimes goes onto the line 103 and then put a line break for
the second argument.

[commit-bot: I haz the power, 2012-02-16 14:40:22]

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

[commit-bot: I haz the power, 2012-02-16 16:33:41]

Change committed as 122299