Move WifiPollingPolicy to a global instance

The WifiPollingPolicy controls how frequently the network location
provider will initiate Wi-Fi scans. Previously, the policy was attached
to the WifiDataProvider, which is destroyed when there are no active
geolocation API calls. Destruction of the provider causes us to lose track
of our polling policy.

The policy allows the first scan to be performed immediately and schedules
subsequent scans at regular intervals. When the provider is destroyed and
recreated rapidly it considers each scan to be "first", which allows many
Wi-Fi scans to be performed in a short window.

This change moves the policy state to a global instance so it can be saved
when the provider is destroyed and recreated. The new policy allows the
first-ever scan to be performed immediately, and subsequent scans may also
be performed immediately if enough time has lapsed since the previous scan.
In all other situations, the configured polling interval is enforced.

BUG=764954

Change-Id: I00649dde2a707127e53e0330511172fef4a43471
Reviewed-on: https://chromium-review.googlesource.com/834988
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526782}

device/geolocation/BUILD.gn

@@ -53,6 +53,7 @@ component("geolocation") {
     "wifi_data_provider_manager.h",
     "wifi_data_provider_win.cc",
     "wifi_data_provider_win.h",
+    "wifi_polling_policy.cc",
     "wifi_polling_policy.h",
   ]
 

device/geolocation/location_arbitrator.cc

@@ -14,6 +14,7 @@
 #include "build/build_config.h"
 #include "device/geolocation/network_location_provider.h"
 #include "device/geolocation/public/cpp/geoposition.h"
+#include "device/geolocation/wifi_polling_policy.h"
 
 namespace device {
 
@@ -33,7 +34,12 @@ LocationArbitrator::LocationArbitrator(
       is_permission_granted_(false),
       is_running_(false) {}
 
-LocationArbitrator::~LocationArbitrator() = default;
+LocationArbitrator::~LocationArbitrator() {
+  // Destroy the global WifiPollingPolicy. The policy is created and used by the
+  // network location provider but should be retained across network provider
+  // restarts to ensure the time of the most recent WiFi scan is not lost.
+  WifiPollingPolicy::Shutdown();
+}
 
 bool LocationArbitrator::HasPermissionBeenGrantedForTest() const {
   return is_permission_granted_;

device/geolocation/wifi_data_provider_chromeos.cc

@@ -36,20 +36,16 @@ WifiDataProviderChromeOs::~WifiDataProviderChromeOs() = default;
 void WifiDataProviderChromeOs::StartDataProvider() {
   DCHECK(CalledOnClientThread());
 
-  DCHECK(polling_policy_ == nullptr);
-  polling_policy_.reset(
-      new GenericWifiPollingPolicy<kDefaultPollingIntervalMilliseconds,
-                                   kNoChangePollingIntervalMilliseconds,
-                                   kTwoNoChangePollingIntervalMilliseconds,
-                                   kNoWifiPollingIntervalMilliseconds>);
-
-  ScheduleStart();
+  if (!WifiPollingPolicy::IsInitialized())
+    WifiPollingPolicy::Initialize(CreatePollingPolicy());
+  DCHECK(WifiPollingPolicy::IsInitialized());
+
+  ScheduleStart(WifiPollingPolicy::Get()->PollingInterval());
 }
 
 void WifiDataProviderChromeOs::StopDataProvider() {
   DCHECK(CalledOnClientThread());
 
-  polling_policy_.reset();
   ScheduleStop();
 }
 
@@ -83,7 +79,7 @@ void WifiDataProviderChromeOs::DidWifiScanTaskNoResults() {
   // Schedule next scan if started (StopDataProvider could have been called
   // in between DoWifiScanTaskOnNetworkHandlerThread and this method).
   if (started_)
-    ScheduleNextScan(polling_policy_->NoWifiInterval());
+    ScheduleNextScan(WifiPollingPolicy::Get()->NoWifiInterval());
 }
 
 void WifiDataProviderChromeOs::DidWifiScanTask(const WifiData& new_data) {
@@ -93,8 +89,8 @@ void WifiDataProviderChromeOs::DidWifiScanTask(const WifiData& new_data) {
   // Schedule next scan if started (StopDataProvider could have been called
   // in between DoWifiScanTaskOnNetworkHandlerThread and this method).
   if (started_) {
-    polling_policy_->UpdatePollingInterval(update_available);
-    ScheduleNextScan(polling_policy_->PollingInterval());
+    WifiPollingPolicy::Get()->UpdatePollingInterval(update_available);
+    ScheduleNextScan(WifiPollingPolicy::Get()->PollingInterval());
   }
 
   if (update_available || !is_first_scan_complete_) {
@@ -124,7 +120,7 @@ void WifiDataProviderChromeOs::ScheduleStop() {
   started_ = false;
 }
 
-void WifiDataProviderChromeOs::ScheduleStart() {
+void WifiDataProviderChromeOs::ScheduleStart(int interval) {
   DCHECK(CalledOnClientThread());
   DCHECK(!started_);
   if (!NetworkHandler::IsInitialized()) {
@@ -132,13 +128,12 @@ void WifiDataProviderChromeOs::ScheduleStart() {
     return;
   }
   started_ = true;
-  // Perform first scan ASAP regardless of the polling policy. If this scan
-  // fails we'll retry at a rate in line with the polling policy.
-  NetworkHandler::Get()->task_runner()->PostTask(
+  NetworkHandler::Get()->task_runner()->PostDelayedTask(
       FROM_HERE,
       base::Bind(
           &WifiDataProviderChromeOs::DoWifiScanTaskOnNetworkHandlerThread,
-          this));
+          this),
+      base::TimeDelta::FromMilliseconds(interval));
 }
 
 bool WifiDataProviderChromeOs::GetAccessPointData(
@@ -176,6 +171,14 @@ bool WifiDataProviderChromeOs::GetAccessPointData(
   return true;
 }
 
+std::unique_ptr<WifiPollingPolicy>
+WifiDataProviderChromeOs::CreatePollingPolicy() {
+  return std::make_unique<GenericWifiPollingPolicy<
+      kDefaultPollingIntervalMilliseconds, kNoChangePollingIntervalMilliseconds,
+      kTwoNoChangePollingIntervalMilliseconds,
+      kNoWifiPollingIntervalMilliseconds>>();
+}
+
 // static
 WifiDataProvider* WifiDataProviderManager::DefaultFactoryFunction() {
   return new WifiDataProviderChromeOs();

device/geolocation/wifi_data_provider_chromeos.h

@@ -39,7 +39,7 @@ class DEVICE_GEOLOCATION_EXPORT WifiDataProviderChromeOs
   void ScheduleNextScan(int interval);
 
   // Will schedule starting of the scanning process.
-  void ScheduleStart();
+  void ScheduleStart(int interval);
 
   // Will schedule stopping of the scanning process.
   void ScheduleStop();
@@ -47,8 +47,8 @@ class DEVICE_GEOLOCATION_EXPORT WifiDataProviderChromeOs
   // Get access point data from chromeos.
   bool GetAccessPointData(WifiData::AccessPointDataSet* data);
 
-  // Controls the polling update interval. (client thread)
-  std::unique_ptr<WifiPollingPolicy> polling_policy_;
+  // Create the policy for controlling the polling update interval.
+  std::unique_ptr<WifiPollingPolicy> CreatePollingPolicy();
 
   // The latest wifi data. (client thread)
   WifiData wifi_data_;

device/geolocation/wifi_data_provider_common.cc

@@ -35,18 +35,15 @@ void WifiDataProviderCommon::StartDataProvider() {
     return;
   }
 
-  DCHECK(!polling_policy_);
-  polling_policy_ = CreatePollingPolicy();
-  DCHECK(polling_policy_);
+  if (!WifiPollingPolicy::IsInitialized())
+    WifiPollingPolicy::Initialize(CreatePollingPolicy());
+  DCHECK(WifiPollingPolicy::IsInitialized());
 
-  // Perform first scan ASAP regardless of the polling policy. If this scan
-  // fails we'll retry at a rate in line with the polling policy.
-  ScheduleNextScan(0);
+  ScheduleNextScan(WifiPollingPolicy::Get()->PollingInterval());
 }
 
 void WifiDataProviderCommon::StopDataProvider() {
   wlan_api_.reset();
-  polling_policy_.reset();
 }
 
 bool WifiDataProviderCommon::GetData(WifiData* data) {
@@ -60,12 +57,12 @@ void WifiDataProviderCommon::DoWifiScanTask() {
   bool update_available = false;
   WifiData new_data;
   if (!wlan_api_->GetAccessPointData(&new_data.access_point_data)) {
-    ScheduleNextScan(polling_policy_->NoWifiInterval());
+    ScheduleNextScan(WifiPollingPolicy::Get()->NoWifiInterval());
   } else {
     update_available = wifi_data_.DiffersSignificantly(new_data);
     wifi_data_ = new_data;
-    polling_policy_->UpdatePollingInterval(update_available);
-    ScheduleNextScan(polling_policy_->PollingInterval());
+    WifiPollingPolicy::Get()->UpdatePollingInterval(update_available);
+    ScheduleNextScan(WifiPollingPolicy::Get()->PollingInterval());
   }
   if (update_available || !is_first_scan_complete_) {
     is_first_scan_complete_ = true;

device/geolocation/wifi_data_provider_common.h

@@ -72,9 +72,6 @@ class DEVICE_GEOLOCATION_EXPORT WifiDataProviderCommon
   // Underlying OS wifi API.
   std::unique_ptr<WlanApiInterface> wlan_api_;
 
-  // Controls the polling update interval.
-  std::unique_ptr<WifiPollingPolicy> polling_policy_;
-
   // Holder for delayed tasks; takes care of cleanup.
   base::WeakPtrFactory<WifiDataProviderCommon> weak_factory_;
 

device/geolocation/wifi_data_provider_common_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "device/geolocation/wifi_data_provider_manager.h"
+#include "device/geolocation/wifi_polling_policy.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -60,7 +61,8 @@ class MockPollingPolicy : public WifiPollingPolicy {
 class WifiDataProviderCommonWithMock : public WifiDataProviderCommon {
  public:
   WifiDataProviderCommonWithMock()
-      : wlan_api_(new MockWlanApi), polling_policy_(new MockPollingPolicy) {}
+      : wlan_api_(new MockWlanApi),
+        polling_policy_(std::make_unique<MockPollingPolicy>()) {}
 
   // WifiDataProviderCommon
   std::unique_ptr<WlanApiInterface> CreateWlanApi() override {
@@ -97,6 +99,7 @@ class GeolocationWifiDataProviderCommonTest : public testing::Test {
   void TearDown() override {
     provider_->RemoveCallback(&wifi_data_callback_);
     provider_->StopDataProvider();
+    WifiPollingPolicy::Shutdown();
   }
 
  protected:

device/geolocation/wifi_polling_policy.cc

+// Copyright 2018 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 "device/geolocation/wifi_polling_policy.h"
+
+namespace device {
+
+namespace {
+WifiPollingPolicy* g_wifi_polling_policy;
+}  // namespace
+
+// static
+void WifiPollingPolicy::Initialize(std::unique_ptr<WifiPollingPolicy> policy) {
+  g_wifi_polling_policy = policy.release();
+}
+
+// static
+void WifiPollingPolicy::Shutdown() {
+  if (g_wifi_polling_policy)
+    delete g_wifi_polling_policy;
+  g_wifi_polling_policy = nullptr;
+}
+
+// static
+WifiPollingPolicy* WifiPollingPolicy::Get() {
+  DCHECK(g_wifi_polling_policy);
+  return g_wifi_polling_policy;
+}
+
+// static
+bool WifiPollingPolicy::IsInitialized() {
+  return g_wifi_polling_policy != nullptr;
+}
+
+}  // namespace device

device/geolocation/wifi_polling_policy.h

@@ -5,21 +5,36 @@
 #ifndef DEVICE_GEOLOCATION_WIFI_POLLING_POLICY_H_
 #define DEVICE_GEOLOCATION_WIFI_POLLING_POLICY_H_
 
+#include <memory>
+
 #include "base/macros.h"
+#include "base/time/time.h"
+#include "device/geolocation/geolocation_export.h"
 
 namespace device {
 
 // Allows sharing and mocking of the update polling policy function.
-class WifiPollingPolicy {
+class DEVICE_GEOLOCATION_EXPORT WifiPollingPolicy {
  public:
-  WifiPollingPolicy() {}
-  virtual ~WifiPollingPolicy() {}
+  virtual ~WifiPollingPolicy() = default;
+
+  // Methods for managing the single instance of WifiPollingPolicy. The WiFi
+  // policy is global so it can outlive the WifiDataProvider instance, which is
+  // shut down and destroyed when no WiFi scanning is active.
+  static void Initialize(std::unique_ptr<WifiPollingPolicy>);
+  static void Shutdown();
+  static WifiPollingPolicy* Get();
+  static bool IsInitialized();
+
   // Calculates the new polling interval for wiFi scans, given the previous
   // interval and whether the last scan produced new results.
   virtual void UpdatePollingInterval(bool scan_results_differ) = 0;
   virtual int PollingInterval() = 0;
   virtual int NoWifiInterval() = 0;
 
+ protected:
+  WifiPollingPolicy() = default;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(WifiPollingPolicy);
 };
@@ -33,6 +48,7 @@ template <int DEFAULT_INTERVAL,
 class GenericWifiPollingPolicy : public WifiPollingPolicy {
  public:
   GenericWifiPollingPolicy() : polling_interval_(DEFAULT_INTERVAL) {}
+
   // WifiPollingPolicy
   void UpdatePollingInterval(bool scan_results_differ) override {
     if (scan_results_differ) {
@@ -45,11 +61,37 @@ class GenericWifiPollingPolicy : public WifiPollingPolicy {
       polling_interval_ = TWO_NO_CHANGE_INTERVAL;
     }
   }
-  int PollingInterval() override { return polling_interval_; }
-  int NoWifiInterval() override { return NO_WIFI_INTERVAL; }
+  int PollingInterval() override { return ComputeInterval(polling_interval_); }
+  int NoWifiInterval() override { return ComputeInterval(NO_WIFI_INTERVAL); }
 
  private:
+  int ComputeInterval(int polling_interval) {
+    base::Time now = base::Time::Now();
+
+    int64_t remaining_millis = 0;
+    if (!next_scan_.is_null()) {
+      // Compute the remaining duration of the current interval. If the interval
+      // is not yet complete, we will schedule a scan to occur once it is.
+      base::TimeDelta remaining = next_scan_ - now;
+      remaining_millis = remaining.InMilliseconds();
+    }
+
+    // If the current interval is complete (or if this is our first scan), scan
+    // now and schedule the next scan to occur at |polling_interval|
+    // milliseconds into the future.
+    if (remaining_millis <= 0) {
+      next_scan_ = now + base::TimeDelta::FromMilliseconds(polling_interval);
+      remaining_millis = 0;
+    }
+
+    return remaining_millis;
+  }
+
   int polling_interval_;
+
+  // The scheduled time of the next scan, or a null value if no scan has
+  // occurred yet.
+  base::Time next_scan_;
 };
 
 }  // namespace device