Fix bug found in the location arbitrator whilst doing trial integration: need to make callback when new observer registers. + add a test for it.

BUG=none
TEST=GeolocationLocationArbitratorTest.RegistrationAfterFixArrives

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

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

3 files changed, 63 insertions, 18 deletions

diff --git a/chrome/browser/geolocation/location_arbitrator.cc b/chrome/browser/geolocation/location_arbitrator.cc
index 6176239..3e76425 100644
--- a/chrome/browser/geolocation/location_arbitrator.cc
+++ b/chrome/browser/geolocation/location_arbitrator.cc
@@ -56,6 +56,9 @@ class GeolocationArbitratorImpl
   typedef std::map<GeolocationArbitrator::Delegate*, UpdateOptions> DelegateMap;
   DelegateMap observers_;
 
+  // The current best estimate of our position.
+  Geoposition position_;
+
   CancelableRequestConsumer request_consumer_;
   bool use_mock_;
 };
@@ -80,7 +83,11 @@ void GeolocationArbitratorImpl::AddObserver(
     const UpdateOptions& update_options) {
   DCHECK(CalledOnValidThread());
   observers_[delegate] = update_options;
-  CreateProviders();
+  if (provider_ == NULL) {
+    CreateProviders();
+  } else if (position_.IsInitialized()) {
+    delegate->OnLocationUpdate(position_);
+  }
 }
 
 bool GeolocationArbitratorImpl::RemoveObserver(
@@ -105,12 +112,15 @@ void GeolocationArbitratorImpl::LocationUpdateAvailable(
     LocationProviderBase* provider) {
   DCHECK(CalledOnValidThread());
   DCHECK(provider);
-  Geoposition position;
-  provider->GetPosition(&position);
+  provider->GetPosition(&position_);
+  DCHECK(position_.IsInitialized());
   // TODO(joth): Arbitrate.
-  for (DelegateMap::const_iterator it = observers_.begin();
-      it != observers_.end(); ++it) {
-    it->first->OnLocationUpdate(position);
+  DelegateMap::const_iterator it = observers_.begin();
+  while (it != observers_.end()) {
+    // Advance iterator before callback to guard against synchronous deregister.
+    Delegate* delegate = it->first;
+    ++it;
+    delegate->OnLocationUpdate(position_);
   }
 }
 
@@ -131,7 +141,8 @@ void GeolocationArbitratorImpl::OnAccessTokenStoresLoaded(
     // set rather than cherry-pick our defaul url.
     const AccessTokenStore::AccessTokenSet::const_iterator token =
         access_token_set.find(default_url_);
-    // TODO(joth): Check with GLS folks what hostname they'd like sent.
+    // TODO(joth): Follow up with GLS folks if they have a plan for replacing
+    // the hostname field. Sending chromium.org is a stop-gap.
     provider_.reset(NewNetworkLocationProvider(
         access_token_store_.get(), context_getter_.get(), default_url_,
         token != access_token_set.end() ? token->second : string16(),
@@ -146,7 +157,8 @@ void GeolocationArbitratorImpl::OnAccessTokenStoresLoaded(
 void GeolocationArbitratorImpl::CreateProviders() {
   DCHECK(CalledOnValidThread());
   DCHECK(!observers_.empty());
-  if (provider_ == NULL && !request_consumer_.HasPendingRequests()) {
+  DCHECK(provider_ == NULL);
+  if (!request_consumer_.HasPendingRequests()) {
     access_token_store_->LoadAccessTokens(
         &request_consumer_,
         NewCallback(this,
@@ -165,7 +177,13 @@ bool GeolocationArbitratorImpl::HasHighAccuracyObserver() {
   return false;
 }
 
-GeolocationArbitrator* GeolocationArbitrator::New(
+GeolocationArbitrator* GeolocationArbitrator::Create(
+    URLRequestContextGetter* context_getter) {
+  return new GeolocationArbitratorImpl(NewChromePrefsAccessTokenStore(),
+                                       context_getter);
+}
+
+GeolocationArbitrator* GeolocationArbitrator::Create(
     AccessTokenStore* access_token_store,
     URLRequestContextGetter* context_getter) {
   return new GeolocationArbitratorImpl(access_token_store,
diff --git a/chrome/browser/geolocation/location_arbitrator.h b/chrome/browser/geolocation/location_arbitrator.h
index df2e517..86b6d99 100644
--- a/chrome/browser/geolocation/location_arbitrator.h
+++ b/chrome/browser/geolocation/location_arbitrator.h
@@ -19,8 +19,12 @@ struct Geoposition;
 // moment.
 class GeolocationArbitrator {
  public:
-  // Creates and returns a new instance of the location arbitrator.
-  static GeolocationArbitrator* New(
+  // Creates and returns a new instance of the location arbitrator. Will use
+  // the default access token store implementation bound to Chrome Prefs.
+  static GeolocationArbitrator* Create(URLRequestContextGetter* context_getter);
+  // Creates and returns a new instance of the location arbitrator. As above
+  // but allows injectino of the access token store, for testing.
+  static GeolocationArbitrator* Create(
       AccessTokenStore* access_token_store,
       URLRequestContextGetter* context_getter);
 
@@ -36,6 +40,8 @@ class GeolocationArbitrator {
   };
   struct UpdateOptions {
     UpdateOptions() : use_high_accuracy(false) {}
+    explicit UpdateOptions(bool high_accuracy)
+        : use_high_accuracy(high_accuracy) {}
     bool use_high_accuracy;
   };
 
diff --git a/chrome/browser/geolocation/location_arbitrator_unittest.cc b/chrome/browser/geolocation/location_arbitrator_unittest.cc
index 63b22bb..a8f44cf 100644
--- a/chrome/browser/geolocation/location_arbitrator_unittest.cc
+++ b/chrome/browser/geolocation/location_arbitrator_unittest.cc
@@ -42,8 +42,8 @@ class GeolocationLocationArbitratorTest : public testing::Test {
  protected:
   virtual void SetUp() {
     access_token_store_ = new FakeAccessTokenStore;
-    arbitrator_.reset(GeolocationArbitrator::New(access_token_store_.get(),
-                                                 NULL));
+    arbitrator_.reset(GeolocationArbitrator::Create(access_token_store_.get(),
+                                                    NULL));
     arbitrator_->SetUseMockProvider(true);
   }
 
@@ -122,13 +122,34 @@ TEST_F(GeolocationLocationArbitratorTest, MultipleListener) {
   EXPECT_TRUE(arbitrator_->RemoveObserver(&observer3));
 }
 
-TEST_F(GeolocationLocationArbitratorTest, MultipleRegistrations) {
+TEST_F(GeolocationLocationArbitratorTest,
+       MultipleAddObserverCallsFromSameListener) {
   MockLocationObserver observer;
-  arbitrator_->AddObserver(&observer, GeolocationArbitrator::UpdateOptions());
-  GeolocationArbitrator::UpdateOptions high_accuracy_options;
-  high_accuracy_options.use_high_accuracy = true;
+  arbitrator_->AddObserver(
+      &observer, GeolocationArbitrator::UpdateOptions(false));
   // TODO(joth): Check this causes the GPS provider to fire up.
-  arbitrator_->AddObserver(&observer, high_accuracy_options);
+  arbitrator_->AddObserver(
+      &observer, GeolocationArbitrator::UpdateOptions(true));
   EXPECT_TRUE(arbitrator_->RemoveObserver(&observer));
   EXPECT_FALSE(arbitrator_->RemoveObserver(&observer));
 }
+
+TEST_F(GeolocationLocationArbitratorTest, RegistrationAfterFixArrives) {
+  MockLocationObserver observer1;
+  arbitrator_->AddObserver(&observer1, GeolocationArbitrator::UpdateOptions());
+
+  access_token_store_->NotifyDelegateTokensLoaded();
+  ASSERT_TRUE(MockLocationProvider::instance_);
+  EXPECT_FALSE(observer1.last_position_.IsInitialized());
+  SetReferencePosition(&MockLocationProvider::instance_->position_);
+  MockLocationProvider::instance_->UpdateListeners();
+  EXPECT_TRUE(observer1.last_position_.IsValidFix());
+
+  MockLocationObserver observer2;
+  EXPECT_FALSE(observer2.last_position_.IsValidFix());
+  arbitrator_->AddObserver(&observer2, GeolocationArbitrator::UpdateOptions());
+  EXPECT_TRUE(observer2.last_position_.IsValidFix());
+
+  EXPECT_TRUE(arbitrator_->RemoveObserver(&observer1));
+  EXPECT_TRUE(arbitrator_->RemoveObserver(&observer2));
+}