Geolocation: cleanup NetworkLocationProvider

device/geolocation/network_location_provider.cc

Index: device/geolocation/network_location_provider.cc
diff --git a/device/geolocation/network_location_provider.cc b/device/geolocation/network_location_provider.cc
index 52aec681a2022af9066b0d7cc74206b6285c36a3..e8e10f7e6a1ce35bf66ce33c700dc7d7391f7a56 100644
--- a/device/geolocation/network_location_provider.cc
+++ b/device/geolocation/network_location_provider.cc
@@ -117,79 +117,38 @@ NetworkLocationProvider::NetworkLocationProvider(
       access_token_(access_token),
       is_permission_granted_(false),
       is_new_data_available_(false),
+      request_(new NetworkLocationRequest(
+          url_context_getter,
+          url,
+          base::Bind(&NetworkLocationProvider::OnLocationResponse,
+                     base::Unretained(this)))),
       position_cache_(new PositionCache),
       weak_factory_(this) {
-  request_.reset(new NetworkLocationRequest(
-      url_context_getter, url,
-      base::Bind(&NetworkLocationProvider::OnLocationResponse,
-                 base::Unretained(this))));
+  DLOG_IF(WARNING, url.is_valid())

[scheib, 2017/04/18 03:33:17]

DLOG if the url is valid? That doesn't seem to match the warning text of it
being a bad URL. You intended !url.is_valid()?

[mcasas, 2017/04/18 05:39:02]

Done.

+      << __func__ << " Bad URL: " << url.possibly_invalid_spec();
 }
 
 NetworkLocationProvider::~NetworkLocationProvider() {
-  StopProvider();
-}
-
-// LocationProvider implementation
-const Geoposition& NetworkLocationProvider::GetPosition() {
-  return position_;
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (IsStarted())
+    StopProvider();
 }
 
 void NetworkLocationProvider::SetUpdateCallback(
     const LocationProvider::LocationProviderUpdateCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   location_provider_update_callback_ = callback;
 }
 
-void NetworkLocationProvider::OnPermissionGranted() {
-  const bool was_permission_granted = is_permission_granted_;
-  is_permission_granted_ = true;
-  if (!was_permission_granted && IsStarted()) {
-    RequestPosition();
-  }
-}
-
-void NetworkLocationProvider::OnWifiDataUpdate() {
-  DCHECK(wifi_data_provider_manager_);
-  is_wifi_data_complete_ = wifi_data_provider_manager_->GetData(&wifi_data_);
-  OnWifiDataUpdated();
-}
-
-void NetworkLocationProvider::OnLocationResponse(
-    const Geoposition& position,
-    bool server_error,
-    const base::string16& access_token,
-    const WifiData& wifi_data) {
-  DCHECK(CalledOnValidThread());
-  // Record the position and update our cache.
-  position_ = position;
-  if (position.Validate()) {
-    position_cache_->CachePosition(wifi_data, position);
-  }
-
-  // Record access_token if it's set.
-  if (!access_token.empty() && access_token_ != access_token) {
-    access_token_ = access_token;
-    access_token_store_->SaveAccessToken(request_->url(), access_token);
-  }
-
-  // Let listeners know that we now have a position available.
-  if (!location_provider_update_callback_.is_null())
-    location_provider_update_callback_.Run(this, position_);
-}
-
 bool NetworkLocationProvider::StartProvider(bool high_accuracy) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (IsStarted())
     return true;
-  DCHECK(!wifi_data_provider_manager_);
-  if (!request_->url().is_valid()) {
-    LOG(WARNING) << "StartProvider() : Failed, Bad URL: "
-                 << request_->url().possibly_invalid_spec();
+  if (!request_->url().is_valid())
     return false;
-  }
 
-  // Registers a callback with the data provider. The first call to Register
-  // will create a singleton data provider and it will be deleted when the last
-  // callback is removed with Unregister.
+  // Registers a callback with the data provider. The first call to Register()
+  // will create a singleton data provider that will be deleted on Unregister().
   wifi_data_provider_manager_ =
       WifiDataProviderManager::Register(&wifi_data_update_callback_);
 
@@ -197,44 +156,40 @@ bool NetworkLocationProvider::StartProvider(bool high_accuracy) {
       FROM_HERE, base::Bind(&NetworkLocationProvider::RequestPosition,
                             weak_factory_.GetWeakPtr()),
       base::TimeDelta::FromSeconds(kDataCompleteWaitSeconds));
-  // Get the wifi data.
-  is_wifi_data_complete_ = wifi_data_provider_manager_->GetData(&wifi_data_);
-  if (is_wifi_data_complete_)
-    OnWifiDataUpdated();
-  return true;
-}
 
-void NetworkLocationProvider::OnWifiDataUpdated() {
-  DCHECK(CalledOnValidThread());
-  wifi_timestamp_ = base::Time::Now();
-
-  is_new_data_available_ = is_wifi_data_complete_;
-  RequestPosition();
+  OnWifiDataUpdate();
+  return true;
 }
 
 void NetworkLocationProvider::StopProvider() {
-  DCHECK(CalledOnValidThread());
-  if (IsStarted()) {
-    wifi_data_provider_manager_->Unregister(&wifi_data_update_callback_);
-  }
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsStarted());
+  wifi_data_provider_manager_->Unregister(&wifi_data_update_callback_);
   wifi_data_provider_manager_ = nullptr;
   weak_factory_.InvalidateWeakPtrs();
 }
 
-// Other methods
+const Geoposition& NetworkLocationProvider::GetPosition() {
+  return position_;
+}
+
+void NetworkLocationProvider::OnPermissionGranted() {
+  const bool was_permission_granted = is_permission_granted_;
+  is_permission_granted_ = true;
+  if (!was_permission_granted && IsStarted())
+    RequestPosition();
+}
+
 void NetworkLocationProvider::RequestPosition() {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
-  // TODO(mcasas): consider not using HasWeakPtrs() https://crbug.com/629158.
-  if (weak_factory_.HasWeakPtrs() && !is_wifi_data_complete_)
-    return;
-  if (!is_new_data_available_)
+  if (!is_new_data_available_ || !is_wifi_data_complete_)
     return;
+  DCHECK(!wifi_timestamp_.is_null())
+      << "|wifi_timestamp_| must be set before looking up position";
 
   const Geoposition* cached_position =
       position_cache_->FindPosition(wifi_data_);
-  DCHECK(!wifi_timestamp_.is_null())
-      << "Timestamp must be set before looking up position";
   if (cached_position) {
     DCHECK(cached_position->Validate());
     // Record the position and update its timestamp.
@@ -251,25 +206,59 @@ void NetworkLocationProvider::RequestPosition() {
       location_provider_update_callback_.Run(this, position_);
     return;
   }
+
   // Don't send network requests until authorized. http://crbug.com/39171
   if (!is_permission_granted_)
     return;
 
-  weak_factory_.InvalidateWeakPtrs();
   is_new_data_available_ = false;

[scheib, 2017/04/18 03:33:17]

Describe why we don't need to invalidate weak ptrs in Change description.

 
   // TODO(joth): Rather than cancel pending requests, we should create a new
   // NetworkLocationRequest for each and hold a set of pending requests.
-  if (request_->is_request_pending()) {
-    DVLOG(1) << "NetworkLocationProvider - pre-empting pending network request "
-                "with new data. Wifi APs: "
-             << wifi_data_.access_point_data.size();
-  }
+  DLOG_IF(WARNING, request_->is_request_pending())
+      << "NetworkLocationProvider - pre-empting pending network request "
+         "with new data. Wifi APs: "
+      << wifi_data_.access_point_data.size();
+
   request_->MakeRequest(access_token_, wifi_data_, wifi_timestamp_);
 }
 
+void NetworkLocationProvider::OnWifiDataUpdate() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsStarted());
+  is_wifi_data_complete_ = wifi_data_provider_manager_->GetData(&wifi_data_);
+  if (!is_wifi_data_complete_)
+    return;
+
+  wifi_timestamp_ = base::Time::Now();
+  is_new_data_available_ = true;
+  RequestPosition();
+}
+
 bool NetworkLocationProvider::IsStarted() const {
   return wifi_data_provider_manager_ != nullptr;
 }
 
+void NetworkLocationProvider::OnLocationResponse(
+    const Geoposition& position,
+    bool server_error,
+    const base::string16& access_token,
+    const WifiData& wifi_data) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // Record the position and update our cache.
+  position_ = position;
+  if (position.Validate())
+    position_cache_->CachePosition(wifi_data, position);
+
+  // Record access_token if it's set.
+  if (!access_token.empty() && access_token_ != access_token) {
+    access_token_ = access_token;
+    access_token_store_->SaveAccessToken(request_->url(), access_token);
+  }
+
+  // Let listeners know that we now have a position available.
+  if (!location_provider_update_callback_.is_null())
+    location_provider_update_callback_.Run(this, position_);
+}
+
 }  // namespace device