Record policy usage statistics every 24 hours.

Record policy usage statistics every 24 hours.


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

[Mattias Nissler (ping if slow), 2012-09-12 13:06:27]

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/brow...
File chrome/browser/browser_process_impl.h (right):

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/brow...
chrome/browser/browser_process_impl.h:166: // Must be destroyed before
|local_state_| and |policy_statistics_collector_|.
Doesn't make sense.

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/poli...
File chrome/browser/policy/policy_statistics_collector.cc (right):

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/poli...
chrome/browser/policy/policy_statistics_collector.cc:51: "");
use std::string() instead of ""

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/poli...
chrome/browser/policy/policy_statistics_collector.cc:61: if (policy->id >
boundary)
boundary = std::max(policy->id, boundary)?

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/poli...
File chrome/browser/policy/policy_statistics_collector.h (right):

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/poli...
chrome/browser/policy/policy_statistics_collector.h:22: // Manages regular
updates of policy usage statistics.
You should mention UMA somewhere, since this is the buzzword people understand.

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/poli...
chrome/browser/policy/policy_statistics_collector.h:43: const
scoped_refptr<base::TaskRunner> task_runner_;
DISALLOW_COPY_AND_ASSIGN

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/common/pref_...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/common/pref_...
chrome/common/pref_names.cc:2029: 
remove extra blank line

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/tools/build/...
File chrome/tools/build/generate_policy_source.py (right):

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/tools/build/...
chrome/tools/build/generate_policy_source.py:38: 'id name vtype platforms
is_deprecated is_device_policy')
this is nice!

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/tools/build/...
chrome/tools/build/generate_policy_source.py:229: '  {{ key::k{name},
Value::{vtype}, {is_device_policy}, {id} }},\n'.
I think the previous code was easier to read.

[Joao da Silva, 2012-09-12 13:29:38]

Drive-by comments

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/brow...
File chrome/browser/browser_process_impl.h (right):

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/brow...
chrome/browser/browser_process_impl.h:168:
scoped_ptr<policy::PolicyStatisticsCollector> policy_statistics_collector_;
Why doesn't this hang off the BrowserPolicyConnector?

UMA_HISTOGRAM_ENUMERATION can still be called after the MetricsService is shut
down, so policy_statistics_collector_ doesn't really depend on the
metrics_service, as implied on browser_process_impl.cc.

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/poli...
File chrome/browser/policy/policy_statistics_collector.cc (right):

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/poli...
chrome/browser/policy/policy_statistics_collector.cc:9: #include
"base/logging.h"
Used?

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/poli...
chrome/browser/policy/policy_statistics_collector.cc:21: const base::TimeDelta
kStatisticsUpdateRate = base::TimeDelta::FromDays(1);
This requires a static initializer, which are avoided in chromium. Store the
default in an int instead.

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/poli...
chrome/browser/policy/policy_statistics_collector.cc:33: base::TimeDelta delay =
base::Time::Now() - last_update;
|last_update| should be clamped to at most Now(). If for some reason it is much
larger than Now() then the next update will be in the far future (e.g. if the
system time is changed in a broken way).

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/pref...
File chrome/browser/prefs/browser_prefs.cc (right):

https://chromiumcodereview.appspot.com/10916235/diff/3001/chrome/browser/pref...
chrome/browser/prefs/browser_prefs.cc:151:
policy::PolicyStatisticsCollector::RegisterPrefs(local_state);
Should be within the #ifdef(ENABLE_CONFIGURATION_POLICY).

[qfel, 2012-09-18 11:30:56]

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_statistics_collector.cc (right):

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/policy/polic...
chrome/browser/policy/policy_statistics_collector.cc:9: #include
"base/logging.h"
On 2012/09/12 13:29:38, Joao da Silva wrote:
> Used?

Oh, that DCHECK is no longer there.
Done.

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/policy/polic...
chrome/browser/policy/policy_statistics_collector.cc:21: const base::TimeDelta
kStatisticsUpdateRate = base::TimeDelta::FromDays(1);
On 2012/09/12 13:29:38, Joao da Silva wrote:
> This requires a static initializer, which are avoided in chromium. Store the
> default in an int instead.

Done.

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/policy/polic...
chrome/browser/policy/policy_statistics_collector.cc:33: base::TimeDelta delay =
base::Time::Now() - last_update;
On 2012/09/12 13:29:38, Joao da Silva wrote:
> |last_update| should be clamped to at most Now(). If for some reason it is
much
> larger than Now() then the next update will be in the far future (e.g. if the
> system time is changed in a broken way).

Done.

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/policy/polic...
chrome/browser/policy/policy_statistics_collector.cc:51: "");
On 2012/09/12 13:06:27, Mattias Nissler wrote:
> use std::string() instead of ""

Done.

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/policy/polic...
chrome/browser/policy/policy_statistics_collector.cc:61: if (policy->id >
boundary)
On 2012/09/12 13:06:27, Mattias Nissler wrote:
> boundary = std::max(policy->id, boundary)?

Why is it better?

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_statistics_collector.h (right):

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/policy/polic...
chrome/browser/policy/policy_statistics_collector.h:22: // Manages regular
updates of policy usage statistics.
On 2012/09/12 13:06:27, Mattias Nissler wrote:
> You should mention UMA somewhere, since this is the buzzword people
understand.

Done.

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/policy/polic...
chrome/browser/policy/policy_statistics_collector.h:43: const
scoped_refptr<base::TaskRunner> task_runner_;
On 2012/09/12 13:06:27, Mattias Nissler wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/10916235/diff/3001/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:151:
policy::PolicyStatisticsCollector::RegisterPrefs(local_state);
On 2012/09/12 13:29:38, Joao da Silva wrote:
> Should be within the #ifdef(ENABLE_CONFIGURATION_POLICY).

Done.

http://codereview.chromium.org/10916235/diff/3001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10916235/diff/3001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:2029: 
On 2012/09/12 13:06:27, Mattias Nissler wrote:
> remove extra blank line

Done.

http://codereview.chromium.org/10916235/diff/3001/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/10916235/diff/3001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:229: '  {{ key::k{name},
Value::{vtype}, {is_device_policy}, {id} }},\n'.
On 2012/09/12 13:06:27, Mattias Nissler wrote:
> I think the previous code was easier to read.

Done.

http://codereview.chromium.org/10916235/diff/14001/chrome/tools/build/generat...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/10916235/diff/14001/chrome/tools/build/generat...
chrome/tools/build/generate_policy_source.py:139: policies.sort(key=lambda
policy: policy.name)
Could make policy namedtuple have name first, but I see no reason to create a
dependency on tuple elements order here.

[Joao da Silva, 2012-09-18 11:49:34]

lgtm with a couple of nits, please fix them :-)

http://codereview.chromium.org/10916235/diff/14001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_statistics_collector.cc (right):

http://codereview.chromium.org/10916235/diff/14001/chrome/browser/policy/poli...
chrome/browser/policy/policy_statistics_collector.cc:11: #include
"base/logging.h"
nit (again): used? :-)

http://codereview.chromium.org/10916235/diff/14001/chrome/browser/policy/poli...
chrome/browser/policy/policy_statistics_collector.cc:57: std::string());
#include <string>

http://codereview.chromium.org/10916235/diff/14001/chrome/browser/prefs/brows...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/10916235/diff/14001/chrome/browser/prefs/brows...
chrome/browser/prefs/browser_prefs.cc:46: #include
"chrome/browser/policy/url_blacklist_manager.h"
These 3 headers should be within #ifdef ENABLE_CONFIGURATION_POLICY, please fix
that :-)

http://codereview.chromium.org/10916235/diff/14001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10916235/diff/14001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:1335: 'browser/policy/policy_statistics_collector.h',
nit: order (before policy_types.h)

http://codereview.chromium.org/10916235/diff/14001/chrome/tools/build/generat...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/10916235/diff/14001/chrome/tools/build/generat...
chrome/tools/build/generate_policy_source.py:128:
is_device_policy=is_device_policy)
Very cool

[qfel, 2012-09-18 13:50:06]

http://codereview.chromium.org/10916235/diff/14001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_statistics_collector.cc (right):

http://codereview.chromium.org/10916235/diff/14001/chrome/browser/policy/poli...
chrome/browser/policy/policy_statistics_collector.cc:11: #include
"base/logging.h"
On 2012/09/18 11:49:34, Joao da Silva wrote:
> nit (again): used? :-)

Oh, was sure removed it already O_O

http://codereview.chromium.org/10916235/diff/14001/chrome/browser/policy/poli...
chrome/browser/policy/policy_statistics_collector.cc:57: std::string());
On 2012/09/18 11:49:34, Joao da Silva wrote:
> #include <string>

Done.

http://codereview.chromium.org/10916235/diff/14001/chrome/browser/prefs/brows...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/10916235/diff/14001/chrome/browser/prefs/brows...
chrome/browser/prefs/browser_prefs.cc:46: #include
"chrome/browser/policy/url_blacklist_manager.h"
On 2012/09/18 11:49:34, Joao da Silva wrote:
> These 3 headers should be within #ifdef ENABLE_CONFIGURATION_POLICY, please
fix
> that :-)

Done.

http://codereview.chromium.org/10916235/diff/14001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10916235/diff/14001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:1335: 'browser/policy/policy_statistics_collector.h',
On 2012/09/18 11:49:34, Joao da Silva wrote:
> nit: order (before policy_types.h)

I was felt very stupid until I realized it was rebase..

[qfel, 2012-09-20 11:02:56]

thakis: need owner's review for chrome/tools/build and for some mechanical
changes in chrome/

Thanks.

[Nico, 2012-09-20 11:10:15]

chrome/ and tools/ lgtm

[Joao da Silva, 2012-09-20 12:06:20]

lgtm with a couple more nits

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/poli...
File chrome/browser/policy/policy_statistics_collector.cc (right):

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/poli...
chrome/browser/policy/policy_statistics_collector.cc:71: DCHECK(id <=
max_policy_id_);
DCHECK_LE, so that it prints the actual values with this fails.

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/poli...
File chrome/browser/policy/policy_statistics_collector_unittest.cc (right):

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/poli...
chrome/browser/policy/policy_statistics_collector_unittest.cc:47:
MOCK_METHOD1(RecordPolicyUse, void(int));
I'm cool with this test, I'll just leave this suggestion for the record: tests
for collection of metrics can be written as browser_tests that trigger the
collection and then inspect about:histograms. This wouldn't have to be virtual
in that case.

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/poli...
chrome/browser/policy/policy_statistics_collector_unittest.cc:92:
policy_statistics_collector_.reset();
No need to do this explicitly.

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/test...
File chrome/browser/policy/test_task_runner.h (right):

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/test...
chrome/browser/policy/test_task_runner.h:23: base::TimeDelta));
#include "base/time.h"

http://codereview.chromium.org/10916235/diff/23002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10916235/diff/23002/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:163: 'browser/policy/test_task_runner.h',
These are only used in unit_tests, so they should be included for that target
only.

[qfel, 2012-09-20 13:00:02]

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/poli...
File chrome/browser/policy/policy_statistics_collector.cc (right):

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/poli...
chrome/browser/policy/policy_statistics_collector.cc:71: DCHECK(id <=
max_policy_id_);
On 2012/09/20 12:06:21, Joao da Silva wrote:
> DCHECK_LE, so that it prints the actual values with this fails.

Done.

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/poli...
File chrome/browser/policy/policy_statistics_collector_unittest.cc (right):

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/poli...
chrome/browser/policy/policy_statistics_collector_unittest.cc:92:
policy_statistics_collector_.reset();
On 2012/09/20 12:06:21, Joao da Silva wrote:
> No need to do this explicitly.

Just to have eventual problems in cleanup caught by correct test.

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/test...
File chrome/browser/policy/test_task_runner.h (right):

http://codereview.chromium.org/10916235/diff/23002/chrome/browser/policy/test...
chrome/browser/policy/test_task_runner.h:23: base::TimeDelta));
On 2012/09/20 12:06:21, Joao da Silva wrote:
> #include "base/time.h"

This just overrides TaskRunner's method, so TaskRunner's header contains the
includes already. callback and location includes are there for complete types,
which are required for gmock to work - but TimeDelta is passed by value, so we
can be sure TaskRunner's header includes necessary stuff for this.

http://codereview.chromium.org/10916235/diff/23002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10916235/diff/23002/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:163: 'browser/policy/test_task_runner.h',
On 2012/09/20 12:06:21, Joao da Silva wrote:
> These are only used in unit_tests, so they should be included for that target
> only.

Done.

[commit-bot: I haz the power, 2012-09-20 13:01:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qfel@google.com/10916235/37001

[commit-bot: I haz the power, 2012-09-20 13:32:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-20 14:43:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qfel@google.com/10916235/35003

[commit-bot: I haz the power, 2012-09-20 16:44:34]

Change committed as 157786