Add extra logging to help in debugging flaky geolocation browser tests.

Refactor mock location provider to send new position to provider thread, rather than writing it into shared memory that was missing locking.

BUG=None
TEST=Geoloc* browser and unit tests.

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

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

4 files changed, 48 insertions, 66 deletions

diff --git a/chrome/browser/geolocation/geolocation_browsertest.cc b/chrome/browser/geolocation/geolocation_browsertest.cc
index 60b61cb..9efac98 100644
--- a/chrome/browser/geolocation/geolocation_browsertest.cc
+++ b/chrome/browser/geolocation/geolocation_browsertest.cc
@@ -173,8 +173,7 @@ class GeolocationNotificationObserver : public NotificationObserver {
 
 void NotifyGeoposition(const Geoposition& geoposition) {
   DCHECK(MockLocationProvider::instance_);
-  MockLocationProvider::instance_->position_ = geoposition;
-  MockLocationProvider::instance_->HandlePositionChanged();
+  MockLocationProvider::instance_->HandlePositionChanged(geoposition);
   LOG(WARNING) << "MockLocationProvider listeners updated";
 }
 
@@ -332,6 +331,11 @@ class GeolocationBrowserTest : public InProcessBrowserTest {
         expected, function, current_browser_->GetSelectedTabContents());
   }
 
+  // InProcessBrowserTest
+  virtual void TearDownInProcessBrowserTestFixture() {
+    LOG(WARNING) << "TearDownInProcessBrowserTestFixture. Test Finished.";
+  }
+
   InfoBarDelegate* infobar_;
   Browser* current_browser_;
   // path element of a URL referencing the html content for this test.
@@ -466,7 +470,9 @@ IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, IFramesWithFreshPosition) {
   iframe_xpath_ = L"//iframe[@id='iframe_1']";
   // Infobar was displayed, allow access and check there's no error code.
   SetInfobarResponse(iframe1_url_, true);
+  LOG(WARNING) << "Checking position...";
   CheckGeoposition(fresh_position);
+  LOG(WARNING) << "...done.";
 }
 
 
diff --git a/chrome/browser/geolocation/location_arbitrator_unittest.cc b/chrome/browser/geolocation/location_arbitrator_unittest.cc
index 888254c..7a76566 100644
--- a/chrome/browser/geolocation/location_arbitrator_unittest.cc
+++ b/chrome/browser/geolocation/location_arbitrator_unittest.cc
@@ -53,25 +53,6 @@ class MockProviderFactory : public GeolocationArbitrator::ProviderFactory {
   MockLocationProvider* gps_;
 };
 
-void SetPositionFix(Geoposition* position,
-                    double latitude,
-                    double longitude,
-                    double accuracy,
-                    const base::Time& timestamp) {
-  ASSERT_TRUE(position);
-  position->error_code = Geoposition::ERROR_CODE_NONE;
-  position->latitude = latitude;
-  position->longitude = longitude;
-  position->accuracy = accuracy;
-  position->timestamp = timestamp;
-  ASSERT_TRUE(position->IsInitialized());
-  ASSERT_TRUE(position->IsValidFix());
-}
-
-void SetReferencePosition(Geoposition* position) {
-  SetPositionFix(position, 51.0, -0.1, 400, base::Time::FromDoubleT(54321.0));
-}
-
 double g_fake_time_now_secs = 1;
 
 base::Time GetTimeNow() {
@@ -82,6 +63,25 @@ void AdvanceTimeNow(const base::TimeDelta& delta) {
   g_fake_time_now_secs += delta.InSecondsF();
 }
 
+void SetPositionFix(MockLocationProvider* provider,
+                    double latitude,
+                    double longitude,
+                    double accuracy) {
+  Geoposition position;
+  position.error_code = Geoposition::ERROR_CODE_NONE;
+  position.latitude = latitude;
+  position.longitude = longitude;
+  position.accuracy = accuracy;
+  position.timestamp = GetTimeNow();
+  ASSERT_TRUE(position.IsInitialized());
+  ASSERT_TRUE(position.IsValidFix());
+  provider->HandlePositionChanged(position);
+}
+
+void SetReferencePosition(MockLocationProvider* provider) {
+  SetPositionFix(provider, 51.0, -0.1, 400);
+}
+
 class GeolocationLocationArbitratorTest : public testing::Test {
  protected:
   virtual void SetUp() {
@@ -158,8 +158,7 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, providers_->gps_->state_);
   EXPECT_FALSE(observer_->last_position_.IsInitialized());
 
-  SetReferencePosition(&providers_->cell_->position_);
-  providers_->cell_->HandlePositionChanged();
+  SetReferencePosition(providers_->cell_);
 
   EXPECT_TRUE(observer_->last_position_.IsInitialized());
   EXPECT_EQ(providers_->cell_->position_.latitude,
@@ -195,8 +194,7 @@ TEST_F(GeolocationLocationArbitratorTest, SetObserverOptionsAfterFixArrives) {
   ASSERT_TRUE(providers_->gps_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, providers_->cell_->state_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, providers_->gps_->state_);
-  SetReferencePosition(&providers_->cell_->position_);
-  providers_->cell_->HandlePositionChanged();
+  SetReferencePosition(providers_->cell_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, providers_->cell_->state_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, providers_->gps_->state_);
   arbitrator_->SetObserverOptions(GeolocationObserverOptions(true));
@@ -209,61 +207,48 @@ TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
   ASSERT_TRUE(providers_->cell_);
   ASSERT_TRUE(providers_->gps_);
 
-  SetPositionFix(&providers_->cell_->position_, 1, 2, 150, GetTimeNow());
-  providers_->cell_->HandlePositionChanged();
+  SetPositionFix(providers_->cell_, 1, 2, 150);
 
   // First position available
   EXPECT_TRUE(observer_->last_position_.IsValidFix());
   CheckLastPositionInfo(1, 2, 150);
 
-  SetPositionFix(&providers_->gps_->position_, 3, 4, 50, GetTimeNow());
-  providers_->gps_->HandlePositionChanged();
+  SetPositionFix(providers_->gps_, 3, 4, 50);
 
   // More accurate fix available
   CheckLastPositionInfo(3, 4, 50);
 
-  SetPositionFix(&providers_->cell_->position_, 5, 6, 150, GetTimeNow());
-  providers_->cell_->HandlePositionChanged();
+  SetPositionFix(providers_->cell_, 5, 6, 150);
 
   // New fix is available but it's less accurate, older fix should be kept.
   CheckLastPositionInfo(3, 4, 50);
 
   // Advance time, and notify once again
   AdvanceTimeNow(SwitchOnFreshnessCliff());
-  providers_->cell_->HandlePositionChanged();
+  providers_->cell_->HandlePositionChanged(providers_->cell_->position_);
 
   // New fix is available, less accurate but fresher
   CheckLastPositionInfo(5, 6, 150);
 
   // Advance time, and set a low accuracy position
   AdvanceTimeNow(SwitchOnFreshnessCliff());
-  SetPositionFix(&providers_->cell_->position_, 5.676731, 139.629385, 1000,
-                 GetTimeNow());
-  providers_->cell_->HandlePositionChanged();
+  SetPositionFix(providers_->cell_, 5.676731, 139.629385, 1000);
   CheckLastPositionInfo(5.676731, 139.629385, 1000);
 
   // 15 secs later, step outside. Switches to gps signal.
   AdvanceTimeNow(base::TimeDelta::FromSeconds(15));
-  SetPositionFix(&providers_->gps_->position_, 3.5676457, 139.629198, 50,
-                 GetTimeNow());
-  providers_->gps_->HandlePositionChanged();
+  SetPositionFix(providers_->gps_, 3.5676457, 139.629198, 50);
   CheckLastPositionInfo(3.5676457, 139.629198, 50);
 
   // 5 mins later switch cells while walking. Stay on gps.
   AdvanceTimeNow(base::TimeDelta::FromMinutes(5));
-  SetPositionFix(&providers_->cell_->position_, 3.567832, 139.634648, 300,
-                 GetTimeNow());
-  SetPositionFix(&providers_->gps_->position_, 3.5677675, 139.632314, 50,
-                 GetTimeNow());
-  providers_->cell_->HandlePositionChanged();
-  providers_->gps_->HandlePositionChanged();
+  SetPositionFix(providers_->cell_, 3.567832, 139.634648, 300);
+  SetPositionFix(providers_->gps_, 3.5677675, 139.632314, 50);
   CheckLastPositionInfo(3.5677675, 139.632314, 50);
 
   // Ride train and gps signal degrades slightly. Stay on fresher gps
   AdvanceTimeNow(base::TimeDelta::FromMinutes(5));
-  SetPositionFix(&providers_->gps_->position_, 3.5679026, 139.634777, 300,
-                 GetTimeNow());
-  providers_->gps_->HandlePositionChanged();
+  SetPositionFix(providers_->gps_, 3.5679026, 139.634777, 300);
   CheckLastPositionInfo(3.5679026, 139.634777, 300);
 
   // 14 minutes later
@@ -271,34 +256,24 @@ TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
 
   // GPS reading misses a beat, but don't switch to cell yet to avoid
   // oscillating.
-  SetPositionFix(&providers_->gps_->position_, 3.5659005, 139.682579, 300,
-                 GetTimeNow());
-  providers_->gps_->HandlePositionChanged();
+  SetPositionFix(providers_->gps_, 3.5659005, 139.682579, 300);
 
   AdvanceTimeNow(base::TimeDelta::FromSeconds(7));
-  SetPositionFix(&providers_->cell_->position_, 3.5689579, 139.691420, 1000,
-                 GetTimeNow());
-  providers_->cell_->HandlePositionChanged();
+  SetPositionFix(providers_->cell_, 3.5689579, 139.691420, 1000);
   CheckLastPositionInfo(3.5659005, 139.682579, 300);
 
   // 1 minute later
   AdvanceTimeNow(base::TimeDelta::FromMinutes(1));
 
   // Enter tunnel. Stay on fresher gps for a moment.
-  SetPositionFix(&providers_->cell_->position_, 3.5657078, 139.68922, 300,
-                 GetTimeNow());
-  providers_->cell_->HandlePositionChanged();
-  SetPositionFix(&providers_->gps_->position_, 3.5657104, 139.690341, 300,
-                 GetTimeNow());
-  providers_->gps_->HandlePositionChanged();
+  SetPositionFix(providers_->cell_, 3.5657078, 139.68922, 300);
+  SetPositionFix(providers_->gps_, 3.5657104, 139.690341, 300);
   CheckLastPositionInfo(3.5657104, 139.690341, 300);
 
   // 2 minutes later
   AdvanceTimeNow(base::TimeDelta::FromMinutes(2));
   // Arrive in station. Cell moves but GPS is stale. Switch to fresher cell.
-  SetPositionFix(&providers_->cell_->position_, 3.5658700, 139.069979, 1000,
-                 GetTimeNow());
-  providers_->cell_->HandlePositionChanged();
+  SetPositionFix(providers_->cell_, 3.5658700, 139.069979, 1000);
   CheckLastPositionInfo(3.5658700, 139.069979, 1000);
 }
 
diff --git a/chrome/browser/geolocation/mock_location_provider.cc b/chrome/browser/geolocation/mock_location_provider.cc
index 8d3a73f..689ab9c 100644
--- a/chrome/browser/geolocation/mock_location_provider.cc
+++ b/chrome/browser/geolocation/mock_location_provider.cc
@@ -33,14 +33,15 @@ MockLocationProvider::~MockLocationProvider() {
   *self_ref_ = NULL;
 }
 
-void MockLocationProvider::HandlePositionChanged() {
+void MockLocationProvider::HandlePositionChanged(const Geoposition& position) {
   if (provider_loop_->BelongsToCurrentThread()) {
     // The location arbitrator unit tests rely on this method running
     // synchronously.
+    position_ = position;
     UpdateListeners();
   } else {
     Task* task = NewRunnableMethod(
-        this, &MockLocationProvider::HandlePositionChanged);
+        this, &MockLocationProvider::HandlePositionChanged, position);
     provider_loop_->PostTask(FROM_HERE, task);
   }
 }
@@ -102,7 +103,7 @@ class AutoMockLocationProvider : public MockLocationProvider {
       listeners_updated_ = true;
       MessageLoop::current()->PostTask(
           FROM_HERE, task_factory_.NewRunnableMethod(
-          &MockLocationProvider::HandlePositionChanged));
+          &MockLocationProvider::HandlePositionChanged, position_));
     }
   }
 
diff --git a/chrome/browser/geolocation/mock_location_provider.h b/chrome/browser/geolocation/mock_location_provider.h
index ced5d89..17f11d8 100644
--- a/chrome/browser/geolocation/mock_location_provider.h
+++ b/chrome/browser/geolocation/mock_location_provider.h
@@ -23,7 +23,7 @@ class MockLocationProvider : public LocationProviderBase {
   ~MockLocationProvider();
 
   // Updates listeners with the new position.
-  void HandlePositionChanged();
+  void HandlePositionChanged(const Geoposition& position);
 
   // LocationProviderBase implementation.
   virtual bool StartProvider(bool high_accuracy);