Fix some bugs discovered whilst making trial integration of geolocaiton pieces (http://codereview.chromium.org/650060/) & add tests for these cases.

BUG=none
TEST=GeolocationNetworkProviderTest.{NoRequestOnStartupUntilWifiData|NewDataReplacesExistingNetworkRequest}

Review URL: http://codereview.chromium.org/652066

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@39734 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 103 insertions, 88 deletions

diff --git a/chrome/browser/geolocation/network_location_provider.cc b/chrome/browser/geolocation/network_location_provider.cc
index 21e536a..24e2a84 100644
--- a/chrome/browser/geolocation/network_location_provider.cc
+++ b/chrome/browser/geolocation/network_location_provider.cc
@@ -146,7 +146,6 @@ NetworkLocationProvider::~NetworkLocationProvider() {
 // LocationProviderBase implementation
 void NetworkLocationProvider::GetPosition(Geoposition *position) {
   DCHECK(position);
-  AutoLock lock(position_mutex_);
   *position = position_;
 }
 
@@ -159,21 +158,15 @@ void NetworkLocationProvider::UpdatePosition() {
 // DeviceDataProviderInterface::ListenerInterface implementation.
 void NetworkLocationProvider::DeviceDataUpdateAvailable(
     RadioDataProvider* provider) {
-  {
-    AutoLock lock(data_mutex_);
-    DCHECK(provider == radio_data_provider_);
-    is_radio_data_complete_ = radio_data_provider_->GetData(&radio_data_);
-  }
+  DCHECK(provider == radio_data_provider_);
+  is_radio_data_complete_ = radio_data_provider_->GetData(&radio_data_);
   OnDeviceDataUpdated();
 }
 
 void NetworkLocationProvider::DeviceDataUpdateAvailable(
     WifiDataProvider* provider) {
-  {
-    AutoLock lock(data_mutex_);
-    DCHECK(provider == wifi_data_provider_);
-    is_wifi_data_complete_ = wifi_data_provider_->GetData(&wifi_data_);
-  }
+  DCHECK(provider == wifi_data_provider_);
+  is_wifi_data_complete_ = wifi_data_provider_->GetData(&wifi_data_);
   OnDeviceDataUpdated();
 }
 
@@ -184,12 +177,8 @@ void NetworkLocationProvider::LocationResponseAvailable(
     const string16& access_token) {
   DCHECK(CalledOnValidThread());
   // Record the position and update our cache.
-  {
-    AutoLock position_lock(position_mutex_);
-    position_ = position;
-  }
+  position_ = position;
   if (position.IsValidFix()) {
-    AutoLock lock(data_mutex_);
     position_cache_->CachePosition(radio_data_, wifi_data_, position);
   }
 
@@ -199,8 +188,6 @@ void NetworkLocationProvider::LocationResponseAvailable(
     access_token_store_->SaveAccessToken(request_->url(), access_token);
   }
 
-  // If new data arrived whilst request was pending reissue the request.
-  UpdatePosition();
   // Let listeners know that we now have a position available.
   UpdateListeners();
 }
@@ -225,12 +212,9 @@ bool NetworkLocationProvider::StartProvider() {
       delayed_start_task_.NewRunnableMethod(
           &NetworkLocationProvider::RequestPosition),
       kDataCompleteWaitPeriod);
-  {
   // Get the device data.
-  AutoLock lock(data_mutex_);
   is_radio_data_complete_ = radio_data_provider_->GetData(&radio_data_);
   is_wifi_data_complete_ = wifi_data_provider_->GetData(&wifi_data_);
-  }
   if (is_radio_data_complete_ || is_wifi_data_complete_)
     OnDeviceDataUpdated();
   return true;
@@ -242,38 +226,34 @@ void NetworkLocationProvider::RequestPosition() {
 
   delayed_start_task_.RevokeAll();
   const Geoposition* cached_position;
-  {
-    AutoLock lock(data_mutex_);
-    cached_position = position_cache_->FindPosition(radio_data_, wifi_data_);
-  }
+  cached_position = position_cache_->FindPosition(radio_data_, wifi_data_);
   DCHECK_NE(device_data_updated_timestamp_, kint64min) <<
     "Timestamp must be set before looking up position";
   if (cached_position) {
     DCHECK(cached_position->IsValidFix());
     // Record the position and update its timestamp.
-    {
-      AutoLock lock(position_mutex_);
-      position_ = *cached_position;
-      // The timestamp of a position fix is determined by the timestamp
-      // of the source data update. (The value of position_.timestamp from
-      // the cache could be from weeks ago!)
-      position_.timestamp = device_data_updated_timestamp_;
-    }
+    position_ = *cached_position;
+    // The timestamp of a position fix is determined by the timestamp
+    // of the source data update. (The value of position_.timestamp from
+    // the cache could be from weeks ago!)
+    position_.timestamp = device_data_updated_timestamp_;
     is_new_data_available_ = false;
     // Let listeners know that we now have a position available.
     UpdateListeners();
     return;
   }
 
-  DCHECK(request_ != NULL);
-
-  // TODO(joth): Consider timing out any pending request.
-  if (request_->is_request_pending())
-    return;
-
   is_new_data_available_ = false;
 
-  AutoLock data_lock(data_mutex_);
+  // TODO(joth): Rather than cancel pending requests, we should create a new
+  // NetworkLocationRequest for each and hold a set of pending requests. This
+  // means the associated wifi data, timestamps etc. could be tagged onto each
+  // request allowing self-consistent cache update when each request completes.
+  if (request_->is_request_pending()) {
+    LOG(INFO) << "NetworkLocationProvider - pre-empting pending network request"
+              << "with new data. Wifi APs: "
+              << wifi_data_.access_point_data.size();
+  }
   request_->MakeRequest(access_token_, radio_data_, wifi_data_,
                         device_data_updated_timestamp_);
 }
@@ -282,9 +262,9 @@ void NetworkLocationProvider::OnDeviceDataUpdated() {
   DCHECK(CalledOnValidThread());
   device_data_updated_timestamp_ = GetCurrentTimeMillis();
 
-  is_new_data_available_ = is_radio_data_complete_ || is_radio_data_complete_;
+  is_new_data_available_ = is_radio_data_complete_ || is_wifi_data_complete_;
   if (delayed_start_task_.empty() ||
-      (is_radio_data_complete_ && is_radio_data_complete_)) {
+      (is_radio_data_complete_ && is_wifi_data_complete_)) {
     UpdatePosition();
   }
 }
diff --git a/chrome/browser/geolocation/network_location_provider.h b/chrome/browser/geolocation/network_location_provider.h
index efcb20b..ac6e88a 100644
--- a/chrome/browser/geolocation/network_location_provider.h
+++ b/chrome/browser/geolocation/network_location_provider.h
@@ -8,7 +8,6 @@
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/lock.h"
 #include "base/string16.h"
 #include "base/thread.h"
 #include "chrome/browser/geolocation/device_data_provider.h"
@@ -61,26 +60,22 @@ class NetworkLocationProvider
   RadioDataProvider* radio_data_provider_;
   WifiDataProvider* wifi_data_provider_;
 
-  // The radio and wifi data, flags to indicate if each data set is complete,
-  // and their guarding mutex.
+  // The radio and wifi data, flags to indicate if each data set is complete.
   RadioData radio_data_;
   WifiData wifi_data_;
   bool is_radio_data_complete_;
   bool is_wifi_data_complete_;
-  Lock data_mutex_;
 
   // The timestamp for the latest device data update.
   int64 device_data_updated_timestamp_;
 
   string16 access_token_;
 
-  // The current best position estimate and its guarding mutex
+  // The current best position estimate.
   Geoposition position_;
-  Lock position_mutex_;
 
   bool is_new_data_available_;
 
-
   // The network location request object, and the url it uses.
   scoped_ptr<NetworkLocationRequest> request_;
 
diff --git a/chrome/browser/geolocation/network_location_provider_unittest.cc b/chrome/browser/geolocation/network_location_provider_unittest.cc
index 1b05f38..8042a99 100644
--- a/chrome/browser/geolocation/network_location_provider_unittest.cc
+++ b/chrome/browser/geolocation/network_location_provider_unittest.cc
@@ -51,18 +51,20 @@ class MockDeviceDataProviderImpl
     : public DeviceDataProviderImplBase<DataType> {
  public:
   // Factory method for use with DeviceDataProvider::SetFactory.
-  static DeviceDataProviderImplBase<DataType>* Create() {
-    return new MockDeviceDataProviderImpl<DataType>();
+  static DeviceDataProviderImplBase<DataType>* GetInstance() {
+    CHECK(instance_);
+    return instance_;
   }
-  static MockDeviceDataProviderImpl<DataType>* instance() {
-    CHECK(instance_ != NULL);
+
+  static MockDeviceDataProviderImpl<DataType>* CreateInstance() {
+    CHECK(!instance_);
+    instance_ = new MockDeviceDataProviderImpl<DataType>;
     return instance_;
   }
 
-  MockDeviceDataProviderImpl() {
-    CHECK(instance_ == NULL);
-    instance_ = this;
+  MockDeviceDataProviderImpl() : got_data_(true) {
   }
+
   virtual ~MockDeviceDataProviderImpl() {
     CHECK(this == instance_);
     instance_ = NULL;
@@ -75,26 +77,25 @@ class MockDeviceDataProviderImpl
   virtual void StopDataProvider() {}
   virtual bool GetData(DataType* data_out) {
     CHECK(data_out);
-    AutoLock lock(data_mutex_);
     *data_out = data_;
-    // We always have all the data we can get, so return true.
-    return true;
+    return got_data_;
   }
 
   void SetData(const DataType& new_data) {
-    data_mutex_.Acquire();
+    got_data_ = true;
     const bool differs = data_.DiffersSignificantly(new_data);
     data_ = new_data;
-    data_mutex_.Release();
     if (differs)
       this->NotifyListeners();
   }
 
+  void set_got_data(bool got_data) { got_data_ = got_data; }
+
  private:
   static MockDeviceDataProviderImpl<DataType>* instance_;
 
   DataType data_;
-  Lock data_mutex_;
+  bool got_data_;
 
   DISALLOW_COPY_AND_ASSIGN(MockDeviceDataProviderImpl);
 };
@@ -109,6 +110,10 @@ class GeolocationNetworkProviderTest : public testing::Test {
   virtual void SetUp() {
     URLFetcher::set_factory(&url_fetcher_factory_);
     access_token_store_ = new FakeAccessTokenStore;
+    radio_data_provider_ =
+        MockDeviceDataProviderImpl<RadioData>::CreateInstance();
+    wifi_data_provider_ =
+        MockDeviceDataProviderImpl<WifiData>::CreateInstance();
   }
 
   virtual void TearDown() {
@@ -132,9 +137,19 @@ class GeolocationNetworkProviderTest : public testing::Test {
     // TODO(joth): Really these should be in SetUp, not here, but they take no
     // effect on Mac OS Release builds if done there. I kid not. Figure out why.
     RadioDataProvider::SetFactory(
-        MockDeviceDataProviderImpl<RadioData>::Create);
+        MockDeviceDataProviderImpl<RadioData>::GetInstance);
     WifiDataProvider::SetFactory(
-        MockDeviceDataProviderImpl<WifiData>::Create);
+        MockDeviceDataProviderImpl<WifiData>::GetInstance);
+  }
+
+  // Returns the current url fetcher (if any) and advances the id ready for the
+  // next test step.
+  TestURLFetcher* advance_url_fetcher_id() {
+    TestURLFetcher* fetcher = url_fetcher_factory_.GetFetcherByID(
+            NetworkLocationRequest::url_fetcher_id_for_tests);
+    if (fetcher)
+      ++NetworkLocationRequest::url_fetcher_id_for_tests;
+    return fetcher;
   }
 
   static int IndexToChannal(int index) { return index + 4; }
@@ -233,6 +248,8 @@ class GeolocationNetworkProviderTest : public testing::Test {
   MessageLoop main_message_loop_;
   scoped_refptr<FakeAccessTokenStore> access_token_store_;
   TestURLFetcherFactory url_fetcher_factory_;
+  scoped_refptr<MockDeviceDataProviderImpl<RadioData> > radio_data_provider_;
+  scoped_refptr<MockDeviceDataProviderImpl<WifiData> > wifi_data_provider_;
 };
 
 
@@ -248,7 +265,7 @@ TEST_F(GeolocationNetworkProviderTest, CreateDestroy) {
 TEST_F(GeolocationNetworkProviderTest, StartProvider) {
   scoped_ptr<LocationProviderBase> provider(CreateProvider());
   EXPECT_TRUE(provider->StartProvider());
-  TestURLFetcher* fetcher = url_fetcher_factory_.GetFetcherByID(0);
+  TestURLFetcher* fetcher = advance_url_fetcher_id();
   ASSERT_TRUE(fetcher != NULL);
 
   EXPECT_EQ(test_server_url_, fetcher->original_url());
@@ -277,7 +294,7 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   scoped_ptr<LocationProviderBase> provider(CreateProvider());
   EXPECT_TRUE(provider->StartProvider());
 
-  TestURLFetcher* fetcher = url_fetcher_factory_.GetFetcherByID(0);
+  TestURLFetcher* fetcher = advance_url_fetcher_id();
   ASSERT_TRUE(fetcher != NULL);
   CheckEmptyRequestIsValid(fetcher->upload_data());
   // Complete the network request with bad position fix (using #define so we
@@ -300,12 +317,11 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   provider->GetPosition(&position);
   EXPECT_FALSE(position.IsValidFix());
 
-  // Now wifi data arrives
+  // Now wifi data arrives -- SetData will notify listeners.
   const int kFirstScanAps = 6;
-  MockDeviceDataProviderImpl<WifiData>::instance()->SetData(
-      CreateReferenceWifiScanData(kFirstScanAps));  // Will notify listeners
+  wifi_data_provider_->SetData(CreateReferenceWifiScanData(kFirstScanAps));
   main_message_loop_.RunAllPending();
-  fetcher = url_fetcher_factory_.GetFetcherByID(kFirstScanAps);
+  fetcher = advance_url_fetcher_id();
   ASSERT_TRUE(fetcher != NULL);
   // The request should have access token (set previously) and the wifi data.
   CheckRequestIsValid(fetcher->upload_data(),
@@ -343,10 +359,9 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   // Wifi updated again, with one less AP. This is 'close enough' to the
   // previous scan, so no new request made.
   const int kSecondScanAps = kFirstScanAps - 1;
-  MockDeviceDataProviderImpl<WifiData>::instance()->SetData(
-      CreateReferenceWifiScanData(kSecondScanAps));
+  wifi_data_provider_->SetData(CreateReferenceWifiScanData(kSecondScanAps));
   main_message_loop_.RunAllPending();
-  fetcher = url_fetcher_factory_.GetFetcherByID(kSecondScanAps);
+  fetcher = advance_url_fetcher_id();
   EXPECT_FALSE(fetcher);
 
   provider->GetPosition(&position);
@@ -356,10 +371,9 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
 
   // Now a third scan with more than twice the original amount -> new request.
   const int kThirdScanAps = kFirstScanAps * 2 + 1;
-  MockDeviceDataProviderImpl<WifiData>::instance()->SetData(
-      CreateReferenceWifiScanData(kThirdScanAps));
+  wifi_data_provider_->SetData(CreateReferenceWifiScanData(kThirdScanAps));
   main_message_loop_.RunAllPending();
-  fetcher = url_fetcher_factory_.GetFetcherByID(kThirdScanAps);
+  fetcher = advance_url_fetcher_id();
   EXPECT_TRUE(fetcher);
   // ...reply with a network error.
   fetcher->delegate()->OnURLFetchComplete(
@@ -374,13 +388,9 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   EXPECT_FALSE(position.IsValidFix());
 
   // Wifi scan returns to original set: should be serviced from cache.
-  const TestURLFetcher* orig_fetcher =
-      url_fetcher_factory_.GetFetcherByID(kFirstScanAps);
-  MockDeviceDataProviderImpl<WifiData>::instance()->SetData(
-      CreateReferenceWifiScanData(kFirstScanAps));
+  wifi_data_provider_->SetData(CreateReferenceWifiScanData(kFirstScanAps));
   main_message_loop_.RunAllPending();
-  fetcher = url_fetcher_factory_.GetFetcherByID(kFirstScanAps);
-  EXPECT_EQ(orig_fetcher, fetcher);  // No new request created.
+  EXPECT_FALSE(advance_url_fetcher_id());  // No new request created.
 
   provider->GetPosition(&position);
   EXPECT_EQ(51.0, position.latitude);
@@ -388,5 +398,33 @@ TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
   EXPECT_TRUE(position.IsValidFix());
 }
 
-// TODO(joth): Add tests for corner cases around the 2 second startup delay
-//            (e.g. timer firing, or being pre-empted by data arriving)
+TEST_F(GeolocationNetworkProviderTest, NoRequestOnStartupUntilWifiData) {
+  MessageLoopQuitListener listener;
+  wifi_data_provider_->set_got_data(false);
+  scoped_ptr<LocationProviderBase> provider(CreateProvider());
+  EXPECT_TRUE(provider->StartProvider());
+  provider->RegisterListener(&listener);
+
+  main_message_loop_.RunAllPending();
+  EXPECT_FALSE(advance_url_fetcher_id())
+      << "Network request should not be created right away on startup when "
+         "wifi data has not yet arrived";
+
+  wifi_data_provider_->SetData(CreateReferenceWifiScanData(1));
+  main_message_loop_.RunAllPending();
+  EXPECT_TRUE(advance_url_fetcher_id());
+}
+
+TEST_F(GeolocationNetworkProviderTest, NewDataReplacesExistingNetworkRequest) {
+  // Send initial request with empty device data
+  scoped_ptr<LocationProviderBase> provider(CreateProvider());
+  EXPECT_TRUE(provider->StartProvider());
+  TestURLFetcher* fetcher = advance_url_fetcher_id();
+  EXPECT_TRUE(fetcher);
+
+  // Now wifi data arrives; new request should be sent.
+  wifi_data_provider_->SetData(CreateReferenceWifiScanData(4));
+  main_message_loop_.RunAllPending();
+  fetcher = advance_url_fetcher_id();
+  EXPECT_TRUE(fetcher);
+}
diff --git a/chrome/browser/geolocation/network_location_request.cc b/chrome/browser/geolocation/network_location_request.cc
index 6c3373c..9aed423 100644
--- a/chrome/browser/geolocation/network_location_request.cc
+++ b/chrome/browser/geolocation/network_location_request.cc
@@ -61,6 +61,8 @@ void AddRadioData(const RadioData& radio_data, DictionaryValue* body_object);
 void AddWifiData(const WifiData& wifi_data, DictionaryValue* body_object);
 }  // namespace
 
+int NetworkLocationRequest::url_fetcher_id_for_tests = 0;
+
 NetworkLocationRequest::NetworkLocationRequest(URLRequestContextGetter* context,
                                                const GURL& url,
                                                const string16& host_name,
@@ -89,8 +91,7 @@ bool NetworkLocationRequest::MakeRequest(const string16& access_token,
   timestamp_ = timestamp;
 
   url_fetcher_.reset(URLFetcher::Create(
-      wifi_data.access_point_data.size(),  // Used for testing
-      url_, URLFetcher::POST, this));
+      url_fetcher_id_for_tests, url_, URLFetcher::POST, this));
   url_fetcher_->set_upload_data(kMimeApplicationJson, post_body);
   url_fetcher_->set_request_context(url_context_);
   url_fetcher_->set_load_flags(
@@ -184,8 +185,7 @@ void GetLocationFromResponse(bool http_post_result,
     FormatPositionError(server_url, L"No response received", position);
     return;
   }
-  if (status_code != 200) {  // XXX is '200' in a constant? Can't see it
-    // The response was bad.
+  if (status_code != 200) {  // HTTP OK.
     std::wstring message = L"Returned error code ";
     message += IntToWString(status_code);
     FormatPositionError(server_url, message, position);
diff --git a/chrome/browser/geolocation/network_location_request.h b/chrome/browser/geolocation/network_location_request.h
index 97cf16c..35aa245 100644
--- a/chrome/browser/geolocation/network_location_request.h
+++ b/chrome/browser/geolocation/network_location_request.h
@@ -21,6 +21,8 @@ struct Position;
 // It performs formatting of the request and interpretation of the response.
 class NetworkLocationRequest : private URLFetcher::Delegate {
  public:
+  // ID passed to URLFetcher::Create(). Used for testing.
+  static int url_fetcher_id_for_tests;
   // Interface for receiving callbacks from a NetworkLocationRequest object.
   class ListenerInterface {
    public: