Fix DeviceStatusCollectorTest.MaxStoredPeriods.

chrome/browser/policy/device_status_collector.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/policy/device_status_collector.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ skipping 75 matching lines @@
       max_stored_future_activity_days_(kMaxStoredFutureActivityDays),
       local_state_(local_state),
       last_idle_check_(Time()),
       geolocation_update_in_progress_(false),
       statistics_provider_(provider),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
       location_update_requester_(location_update_requester),
       report_version_info_(false),
       report_activity_times_(false),
       report_boot_mode_(false),
-      report_location_(false) {
+      report_location_(false) {}
+
+DeviceStatusCollector::~DeviceStatusCollector() {
+  cros_settings_->RemoveSettingsObserver(chromeos::kReportDeviceVersionInfo,
+                                         this);
+  cros_settings_->RemoveSettingsObserver(chromeos::kReportDeviceActivityTimes,
+                                         this);
+  cros_settings_->RemoveSettingsObserver(chromeos::kReportDeviceBootMode, this);
+  cros_settings_->RemoveSettingsObserver(chromeos::kReportDeviceLocation, this);
+}
+
+void DeviceStatusCollector::Init() {
   if (!location_update_requester_)
     location_update_requester_ = &content::RequestLocationUpdate;
   idle_poll_timer_.Start(FROM_HERE,
                          TimeDelta::FromSeconds(kIdlePollIntervalSeconds),
                          this, &DeviceStatusCollector::CheckIdleState);
 
   cros_settings_ = chromeos::CrosSettings::Get();
 
   // Watch for changes to the individual policies that control what the status
   // reports contain.
@@ skipping 30 matching lines @@
   // Get the the OS and firmware version info.
   version_loader_.GetVersion(
       VersionLoader::VERSION_FULL,
       base::Bind(&DeviceStatusCollector::OnOSVersion, base::Unretained(this)),
       &tracker_);
   version_loader_.GetFirmware(
       base::Bind(&DeviceStatusCollector::OnOSFirmware, base::Unretained(this)),
       &tracker_);
 }
 
-DeviceStatusCollector::~DeviceStatusCollector() {
-  cros_settings_->RemoveSettingsObserver(chromeos::kReportDeviceVersionInfo,
-                                         this);
-  cros_settings_->RemoveSettingsObserver(chromeos::kReportDeviceActivityTimes,
-                                         this);
-  cros_settings_->RemoveSettingsObserver(chromeos::kReportDeviceBootMode, this);
-  cros_settings_->RemoveSettingsObserver(chromeos::kReportDeviceLocation, this);
-}
-
 // static
 void DeviceStatusCollector::RegisterPrefs(PrefService* local_state) {
   local_state->RegisterDictionaryPref(prefs::kDeviceActivityTimes,
                                       new base::DictionaryValue);
   local_state->RegisterDictionaryPref(prefs::kDeviceLocation,
                                       new base::DictionaryValue);
 }
 
 void DeviceStatusCollector::CheckIdleState() {
   CalculateIdleState(kIdleStateThresholdSeconds,
@@ skipping 96 matching lines @@
     return;
 
   Time now = GetCurrentTime();
 
   if (state == IDLE_STATE_ACTIVE) {
     // If it's been too long since the last report, or if the activity is
     // negative (which can happen when the clock changes), assume a single
     // interval of activity.
     int active_seconds = (now - last_idle_check_).InSeconds();
     if (active_seconds < 0 ||
-        active_seconds >= static_cast<int>((2 * kIdlePollIntervalSeconds)))
+        active_seconds >= static_cast<int>((2 * kIdlePollIntervalSeconds))) {
       AddActivePeriod(now - TimeDelta::FromSeconds(kIdlePollIntervalSeconds),
                       now);
-    else
+    } else {
       AddActivePeriod(last_idle_check_, now);
+    }
 
     PruneStoredActivityPeriods(now);
   }
   last_idle_check_ = now;
 }
 
 void DeviceStatusCollector::GetActivityTimes(
     em::DeviceStatusReportRequest* request) {
   DictionaryPrefUpdate update(local_state_, prefs::kDeviceActivityTimes);
   base::DictionaryValue* activity_times = update.Get();
@@ skipping 96 matching lines @@
     UpdateReportingSettings();
   else
     NOTREACHED();
 }
 
 void DeviceStatusCollector::ScheduleGeolocationUpdateRequest() {
   if (geolocation_update_timer_.IsRunning() || geolocation_update_in_progress_)
     return;
 
   if (position_.Validate()) {
-    TimeDelta elapsed = Time::Now() - position_.timestamp;
+    TimeDelta elapsed = GetCurrentTime() - position_.timestamp;

[jar (doing other things), 2012/11/16 20:29:23]

The old code used Time::Now(), which is not a monotonic time source (it can jump
when we go in/out of daylight savings, and it jumps when a user "fixes" their
clock).

Is this the same flavor of time?  I wasn't sure what was being called... but it
seems to mask the probable issue.  

Most commonly, we use base::TimeTicks, which is guaranteed to be monotonically
increasing.... but I wasn't sure where postion_.timestamp came from.

[Ryan Sleevi, 2012/11/16 22:20:48]

I have to agree with Jim here. I think conflating the timestamp received from
the Geolocation source (line 438?) with the system time might be a mistake. It
seems much more reasonable to use base::TimeTicks to manage the polling interval
- to keep it at a true 30 minute interval - and then include in the report the
timesource according to GPS (presumably, a UTC timestamp)

Just a $.02 drive by

[Joao da Silva, 2012/11/20 09:10:52]

It's the same; GetCurrentTime() is a virtual that returns Time::Now() but can be
overridden in tests.
Those are good points. I'm not familiar with this code, adding @bartfab to
comment on that.

     TimeDelta interval =
         TimeDelta::FromSeconds(kGeolocationPollIntervalSeconds);
     if (elapsed > interval) {
       geolocation_update_in_progress_ = true;
       location_update_requester_(base::Bind(
           &DeviceStatusCollector::ReceiveGeolocationUpdate,
           weak_factory_.GetWeakPtr()));
     } else {
       geolocation_update_timer_.Start(
           FROM_HERE,
@@ skipping 30 matching lines @@
     location.SetDouble(kSpeed, position.speed);
     location.SetString(kTimestamp,
         base::Int64ToString(position.timestamp.ToInternalValue()));
     local_state_->Set(prefs::kDeviceLocation, location);
   }
 
   ScheduleGeolocationUpdateRequest();
 }
 
 }  // namespace policy