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..64bb9d9213010a65607900c99eb04b6cd6fceeaa 100644
--- a/device/geolocation/network_location_provider.cc
+++ b/device/geolocation/network_location_provider.cc
@@ -117,40 +117,46 @@ 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())
+      << __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()) {
+  if (!was_permission_granted && IsStarted())
     RequestPosition();
-  }
 }
 
 void NetworkLocationProvider::OnWifiDataUpdate() {
-  DCHECK(wifi_data_provider_manager_);
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsStarted());
   is_wifi_data_complete_ = wifi_data_provider_manager_->GetData(&wifi_data_);
-  OnWifiDataUpdated();
+  if (!is_wifi_data_complete_)
+    return;
+
+  wifi_timestamp_ = base::Time::Now();
+  is_new_data_available_ = true;
+  RequestPosition();
 }
 
 void NetworkLocationProvider::OnLocationResponse(
@@ -158,12 +164,11 @@ void NetworkLocationProvider::OnLocationResponse(
     bool server_error,
     const base::string16& access_token,
     const WifiData& wifi_data) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   // Record the position and update our cache.
   position_ = position;
-  if (position.Validate()) {
+  if (position.Validate())
     position_cache_->CachePosition(wifi_data, position);
-  }
 
   // Record access_token if it's set.
   if (!access_token.empty() && access_token_ != access_token) {
@@ -177,19 +182,14 @@ void NetworkLocationProvider::OnLocationResponse(
 }
 
 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 +197,33 @@ 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::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.
@@ -255,16 +244,15 @@ void NetworkLocationProvider::RequestPosition() {
   if (!is_permission_granted_)
     return;
 
-  weak_factory_.InvalidateWeakPtrs();
   is_new_data_available_ = false;
 
   // 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_);
 }