Reland "Move WifiPollingPolicy to a global instance"

This is a reland of bd9b9ea0
Original change's description:
> 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.
> 
> The global policy instance is leaked at shutdown.
> 
> BUG=764954
> 
> Change-Id: If16c73dfd7d6535346e86d0fab5df24f0c7b297d
> Reviewed-on: https://chromium-review.googlesource.com/850444
> Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#527219}

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

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/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) {
@@ -64,12 +61,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 {
+// Leaks at exit.
+WifiPollingPolicy* g_wifi_polling_policy;
+}  // namespace
+
+// static
+void WifiPollingPolicy::Initialize(std::unique_ptr<WifiPollingPolicy> policy) {
+  DCHECK(!g_wifi_polling_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