Bring Geolocation to life!

Bolt the geolocation dispatcher host up to the location arbitrator
Introduces a new method for fetching a singleton default location arbitrator with minimal fuss.
Fix bug in the geolocation dispatcher where bridge id & route id were swapped on send.

BUG=http://crbug.com/11246
TEST=run browser with --enable-geolocaiton & Open http://maps.google.co.uk/maps/m

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

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

6 files changed, 79 insertions, 47 deletions

diff --git a/chrome/browser/geolocation/geolocation_browsertest.cc b/chrome/browser/geolocation/geolocation_browsertest.cc
index 03eccb2..1957893 100644
--- a/chrome/browser/geolocation/geolocation_browsertest.cc
+++ b/chrome/browser/geolocation/geolocation_browsertest.cc
@@ -6,6 +6,7 @@
 #include "chrome/browser/app_modal_dialog.h"
 #include "chrome/browser/browser.h"
 #include "chrome/browser/browser_list.h"
+#include "chrome/browser/geolocation/location_arbitrator.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/renderer_host/render_view_host.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
@@ -69,6 +70,7 @@ class GeolocationBrowserTest : public InProcessBrowserTest {
   };
 
   void Initialize(InitializationOptions options) {
+    GeolocationArbitrator::SetUseMockProvider(true);
     if (!server_.get()) {
       server_ = StartHTTPServer();
     }
@@ -172,6 +174,9 @@ class GeolocationBrowserTest : public InProcessBrowserTest {
     InProcessBrowserTest::SetUpCommandLine(command_line);
     command_line->AppendSwitch(switches::kEnableGeolocation);
   }
+  virtual void TearDownInProcessBrowserTestFixture() {
+    GeolocationArbitrator::SetUseMockProvider(false);
+  }
 
   scoped_refptr<HTTPTestServer> server_;
   InfoBarDelegate* infobar_;
diff --git a/chrome/browser/geolocation/geolocation_dispatcher_host.cc b/chrome/browser/geolocation/geolocation_dispatcher_host.cc
index 2ea0a48..4a5e0bc 100644
--- a/chrome/browser/geolocation/geolocation_dispatcher_host.cc
+++ b/chrome/browser/geolocation/geolocation_dispatcher_host.cc
@@ -23,6 +23,8 @@ GeolocationDispatcherHost::GeolocationDispatcherHost(
 }
 
 GeolocationDispatcherHost::~GeolocationDispatcherHost() {
+  if (location_arbitrator_)
+    location_arbitrator_->RemoveObserver(this);
 }
 
 bool GeolocationDispatcherHost::OnMessageReceived(
@@ -49,16 +51,8 @@ bool GeolocationDispatcherHost::OnMessageReceived(
   return handled;
 }
 
-void GeolocationDispatcherHost::NotifyPositionUpdated(
+void GeolocationDispatcherHost::OnLocationUpdate(
     const Geoposition& geoposition) {
-  if (!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
-    ChromeThread::PostTask(
-        ChromeThread::UI, FROM_HERE,
-        NewRunnableMethod(
-            this, &GeolocationDispatcherHost::NotifyPositionUpdated,
-            geoposition));
-    return;
-  }
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
   for (std::set<GeolocationServiceRenderId>::iterator geo =
@@ -99,12 +93,17 @@ void GeolocationDispatcherHost::OnRequestPermission(
 void GeolocationDispatcherHost::OnStartUpdating(
     int route_id, int bridge_id, bool high_accuracy) {
   LOG(INFO) << "start updating" << route_id;
-  // TODO(bulach): connect this with GeolocationServiceProvider.
+  if (!location_arbitrator_)
+    location_arbitrator_ = GeolocationArbitrator::GetInstance();
+  location_arbitrator_->AddObserver(
+      this, GeolocationArbitrator::UpdateOptions(high_accuracy));
 }
 
 void GeolocationDispatcherHost::OnStopUpdating(int route_id, int bridge_id) {
   LOG(INFO) << "stop updating" << route_id;
-  // TODO(bulach): connect this with GeolocationServiceProvider.
+  if (location_arbitrator_)
+    location_arbitrator_->RemoveObserver(this);
+  location_arbitrator_ = NULL;
 }
 
 void GeolocationDispatcherHost::OnSuspend(int route_id, int bridge_id) {
diff --git a/chrome/browser/geolocation/geolocation_dispatcher_host.h b/chrome/browser/geolocation/geolocation_dispatcher_host.h
index c054bbb..b8d2c84 100644
--- a/chrome/browser/geolocation/geolocation_dispatcher_host.h
+++ b/chrome/browser/geolocation/geolocation_dispatcher_host.h
@@ -9,18 +9,21 @@
 
 #include "base/basictypes.h"
 #include "base/ref_counted.h"
+#include "chrome/browser/geolocation/location_arbitrator.h"
 #include "ipc/ipc_message.h"
 
 class GeolocationPermissionContext;
-struct Geoposition;
 class GURL;
 class ResourceMessageFilter;
+class URLRequestContextGetter;
+struct Geoposition;
 
 // GeolocationDispatcherHost is a delegate for Geolocation messages used by
 // ResourceMessageFilter.
 // It's the complement of GeolocationDispatcher (owned by RenderView).
 class GeolocationDispatcherHost
-    : public base::RefCountedThreadSafe<GeolocationDispatcherHost> {
+    : public base::RefCountedThreadSafe<GeolocationDispatcherHost>,
+      public GeolocationArbitrator::Delegate {
  public:
   GeolocationDispatcherHost(
       int resource_message_filter_process_id,
@@ -30,8 +33,8 @@ class GeolocationDispatcherHost
   // handled. Called in the browser process.
   bool OnMessageReceived(const IPC::Message& msg, bool* msg_was_ok);
 
-  // Tells the render view that a new geolocation position is available.
-  void NotifyPositionUpdated(const Geoposition& geoposition);
+  // GeolocationArbitrator::Delegate
+  virtual void OnLocationUpdate(const Geoposition& position);
 
  private:
   friend class base::RefCountedThreadSafe<GeolocationDispatcherHost>;
@@ -70,6 +73,8 @@ class GeolocationDispatcherHost
   };
   // Only used on the IO thread.
   std::set<GeolocationServiceRenderId> geolocation_renderers_;
+  // Only set whilst we are registered with the arbitrator.
+  scoped_refptr<GeolocationArbitrator> location_arbitrator_;
 
   DISALLOW_COPY_AND_ASSIGN(GeolocationDispatcherHost);
 };
diff --git a/chrome/browser/geolocation/location_arbitrator.cc b/chrome/browser/geolocation/location_arbitrator.cc
index 3e76425..7529dd2 100644
--- a/chrome/browser/geolocation/location_arbitrator.cc
+++ b/chrome/browser/geolocation/location_arbitrator.cc
@@ -14,11 +14,14 @@
 #include "chrome/browser/net/url_request_context_getter.h"
 #include "chrome/browser/geolocation/access_token_store.h"
 #include "chrome/browser/geolocation/location_provider.h"
+#include "chrome/browser/profile.h"
 #include "chrome/common/geoposition.h"
 #include "googleurl/src/gurl.h"
 
 namespace {
 const char* kDefaultNetworkProviderUrl = "https://www.google.com/loc/json";
+bool g_use_mock_provider = false;
+static GeolocationArbitrator* g_instance_ = NULL;
 }  // namespace
 
 class GeolocationArbitratorImpl
@@ -34,7 +37,6 @@ class GeolocationArbitratorImpl
   virtual void AddObserver(GeolocationArbitrator::Delegate* delegate,
                            const UpdateOptions& update_options);
   virtual bool RemoveObserver(GeolocationArbitrator::Delegate* delegate);
-  virtual void SetUseMockProvider(bool use_mock);
 
   // ListenerInterface
   virtual void LocationUpdateAvailable(LocationProviderBase* provider);
@@ -60,7 +62,6 @@ class GeolocationArbitratorImpl
   Geoposition position_;
 
   CancelableRequestConsumer request_consumer_;
-  bool use_mock_;
 };
 
 GeolocationArbitratorImpl::GeolocationArbitratorImpl(
@@ -68,14 +69,17 @@ GeolocationArbitratorImpl::GeolocationArbitratorImpl(
     URLRequestContextGetter* context_getter)
     : access_token_store_(access_token_store),
       context_getter_(context_getter),
-      default_url_(kDefaultNetworkProviderUrl),
-      use_mock_(false) {
+      default_url_(kDefaultNetworkProviderUrl) {
   DCHECK(default_url_.is_valid());
+  DCHECK(NULL == g_instance_);
+  g_instance_ = this;
 }
 
 GeolocationArbitratorImpl::~GeolocationArbitratorImpl() {
   DCHECK(CalledOnValidThread());
   DCHECK(observers_.empty()) << "Not all observers have unregistered";
+  DCHECK(this == g_instance_);
+  g_instance_ = NULL;
 }
 
 void GeolocationArbitratorImpl::AddObserver(
@@ -102,12 +106,6 @@ bool GeolocationArbitratorImpl::RemoveObserver(
   return remove > 0;
 }
 
-void GeolocationArbitratorImpl::SetUseMockProvider(bool use_mock) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(provider_ == NULL);
-  use_mock_ = use_mock;
-}
-
 void GeolocationArbitratorImpl::LocationUpdateAvailable(
     LocationProviderBase* provider) {
   DCHECK(CalledOnValidThread());
@@ -134,7 +132,7 @@ void GeolocationArbitratorImpl::OnAccessTokenStoresLoaded(
   DCHECK(provider_ == NULL)
       << "OnAccessTokenStoresLoaded : has existing location "
       << "provider. Race condition caused repeat load of tokens?";
-  if (use_mock_) {
+  if (g_use_mock_provider) {
     provider_.reset(NewMockLocationProvider());
   } else {
     // TODO(joth): Once we have arbitration implementation, iterate the whole
@@ -178,16 +176,30 @@ bool GeolocationArbitratorImpl::HasHighAccuracyObserver() {
 }
 
 GeolocationArbitrator* GeolocationArbitrator::Create(
+    AccessTokenStore* access_token_store,
     URLRequestContextGetter* context_getter) {
-  return new GeolocationArbitratorImpl(NewChromePrefsAccessTokenStore(),
-                                       context_getter);
+  DCHECK(!g_instance_);
+  return new GeolocationArbitratorImpl(access_token_store, context_getter);
 }
 
-GeolocationArbitrator* GeolocationArbitrator::Create(
-    AccessTokenStore* access_token_store,
-    URLRequestContextGetter* context_getter) {
-  return new GeolocationArbitratorImpl(access_token_store,
-                                       context_getter);
+GeolocationArbitrator* GeolocationArbitrator::GetInstance() {
+  if (!g_instance_) {
+    // Construct the arbitrator using default token store and url context. We
+    // get the url context getter from the default profile as it's not
+    // particularly important which profile it is attached to: the network
+    // request implementation disables cookies anyhow.
+    Create(NewChromePrefsAccessTokenStore(),
+           Profile::GetDefaultRequestContext());
+    DCHECK(g_instance_);
+  }
+  return g_instance_;
+}
+
+void GeolocationArbitrator::SetUseMockProvider(bool use_mock) {
+  g_use_mock_provider = use_mock;
+}
+
+GeolocationArbitrator::GeolocationArbitrator() {
 }
 
 GeolocationArbitrator::~GeolocationArbitrator() {
diff --git a/chrome/browser/geolocation/location_arbitrator.h b/chrome/browser/geolocation/location_arbitrator.h
index 86b6d99..96e1de2 100644
--- a/chrome/browser/geolocation/location_arbitrator.h
+++ b/chrome/browser/geolocation/location_arbitrator.h
@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_GEOLOCATION_LOCATION_ARBITRATOR_H_
 #define CHROME_BROWSER_GEOLOCATION_LOCATION_ARBITRATOR_H_
 
+#include "base/ref_counted.h"
+
 class AccessTokenStore;
 class URLRequestContextGetter;
 struct Geoposition;
@@ -17,17 +19,19 @@ struct Geoposition;
 // This class is responsible for handling updates from multiple underlying
 // providers and resolving them to a single 'best' location fix at any given
 // moment.
-class GeolocationArbitrator {
+class GeolocationArbitrator : public base::RefCounted<GeolocationArbitrator> {
  public:
-  // 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.
+  // Creates and returns a new instance of the location arbitrator. Allows
+  // injection of the access token store and url getter context, for testing.
   static GeolocationArbitrator* Create(
       AccessTokenStore* access_token_store,
       URLRequestContextGetter* context_getter);
 
+  // Gets a pointer to the singleton instance of the location arbitrator, which
+  // is in turn bound to the browser's global context objects. Ownership is NOT
+  // returned.
+  static GeolocationArbitrator* GetInstance();
+
   class Delegate {
    public:
     // This will be called whenever the 'best available' location is updated,
@@ -45,8 +49,6 @@ class GeolocationArbitrator {
     bool use_high_accuracy;
   };
 
-  virtual ~GeolocationArbitrator();
-
   // Must be called from the same thread as the arbitrator was created on.
   // The update options passed are used as a 'hint' for the provider preferences
   // for this particular observer, however the delegate could receive callbacks
@@ -62,7 +64,16 @@ class GeolocationArbitrator {
 
   // TODO(joth): This is a stop-gap for testing; once we have decoupled
   // provider factory we should extract mock creation from the arbitrator.
-  virtual void SetUseMockProvider(bool use_mock) = 0;
+  static void SetUseMockProvider(bool use_mock);
+
+ protected:
+  friend class base::RefCounted<GeolocationArbitrator>;
+  GeolocationArbitrator();
+  // RefCounted object; no not delete directly.
+  virtual ~GeolocationArbitrator();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(GeolocationArbitrator);
 };
 
 #endif  // CHROME_BROWSER_GEOLOCATION_LOCATION_ARBITRATOR_H_
diff --git a/chrome/browser/geolocation/location_arbitrator_unittest.cc b/chrome/browser/geolocation/location_arbitrator_unittest.cc
index a8f44cf..dfa08c3 100644
--- a/chrome/browser/geolocation/location_arbitrator_unittest.cc
+++ b/chrome/browser/geolocation/location_arbitrator_unittest.cc
@@ -42,22 +42,22 @@ class GeolocationLocationArbitratorTest : public testing::Test {
  protected:
   virtual void SetUp() {
     access_token_store_ = new FakeAccessTokenStore;
-    arbitrator_.reset(GeolocationArbitrator::Create(access_token_store_.get(),
-                                                    NULL));
-    arbitrator_->SetUseMockProvider(true);
+    GeolocationArbitrator::SetUseMockProvider(true);
+    arbitrator_ = GeolocationArbitrator::Create(access_token_store_.get(),
+                                                NULL);
   }
 
   virtual void TearDown() {
   }
 
   scoped_refptr<FakeAccessTokenStore> access_token_store_;
-  scoped_ptr<GeolocationArbitrator> arbitrator_;
+  scoped_refptr<GeolocationArbitrator> arbitrator_;
 };
 
 TEST_F(GeolocationLocationArbitratorTest, CreateDestroy) {
   EXPECT_TRUE(access_token_store_);
   EXPECT_TRUE(arbitrator_ != NULL);
-  arbitrator_.reset();
+  arbitrator_ = NULL;
   SUCCEED();
 }