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..2de4026a87661d77460c7f48855fc8fce42d38e5 100644
--- a/device/geolocation/network_location_provider.cc
+++ b/device/geolocation/network_location_provider.cc
@@ -117,21 +117,20 @@ 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_;
+  if (IsStarted())
+    StopProvider();
 }
 
 void NetworkLocationProvider::SetUpdateCallback(
@@ -139,57 +138,16 @@ void NetworkLocationProvider::SetUpdateCallback(
   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())  // Already DLOG() in constructor.

[dougt, 2017/04/17 18:58:52]

Have you considered just dropping this check?  request_ isn't going to change
and you've already DLOG().  Maybe just let the provider start at this point?

[mcasas, 2017/04/17 20:47:55]

Changed to a DCHECK to document it as being a 
precondition of this method.

     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 +155,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 +205,61 @@ 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;
 
   // TODO(joth): Rather than cancel pending requests, we should create a new
   // NetworkLocationRequest for each and hold a set of pending requests.
+#if !defined(NDEBUG)
   if (request_->is_request_pending()) {
     DVLOG(1) << "NetworkLocationProvider - pre-empting pending network request "

[dougt, 2017/04/17 18:58:52]

is this DVLOG() needed anymore?  Maybe just drop it.

[mcasas, 2017/04/17 20:47:55]

Hmm well, I didn't want to remove it entirely because of the
associated comment in l.215. Will simplify.

                 "with new data. Wifi APs: "
              << wifi_data_.access_point_data.size();
   }
+#endif
   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;

[dougt, 2017/04/17 18:58:52]

I am confused why this code remembers the position -- even when it's not valid?

[mcasas, 2017/04/17 20:47:55]

Bug perhaps? I haven't changed this method's logic.

+  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