Track options per bridge rather than per dispatcher host

Also fixes a bug in arbitrator that updating options might not stop high accuracy providers

BUG=40103
TEST=unit_tests --gtest_filter=*Geol*

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

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

5 files changed, 87 insertions, 36 deletions

diff --git a/chrome/browser/geolocation/geolocation_dispatcher_host.cc b/chrome/browser/geolocation/geolocation_dispatcher_host.cc
index 33d5c2b..c88e41d 100644
--- a/chrome/browser/geolocation/geolocation_dispatcher_host.cc
+++ b/chrome/browser/geolocation/geolocation_dispatcher_host.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/renderer_host/render_view_host_notification_task.h"
 #include "chrome/browser/renderer_host/resource_message_filter.h"
 #include "chrome/common/render_messages.h"
+#include "ipc/ipc_message.h"
 
 GeolocationDispatcherHost::GeolocationDispatcherHost(
     int resource_message_filter_process_id,
@@ -78,7 +79,8 @@ void GeolocationDispatcherHost::OnUnregisterDispatcher(int render_view_id) {
 
 void GeolocationDispatcherHost::OnRequestPermission(
     int render_view_id, int bridge_id, const GURL& requesting_frame) {
-  LOG(INFO) << "permission request";
+  DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_
+             << ":" << render_view_id << ":" << bridge_id;
   geolocation_permission_context_->RequestGeolocationPermission(
       resource_message_filter_process_id_, render_view_id, bridge_id,
       requesting_frame);
@@ -86,7 +88,8 @@ void GeolocationDispatcherHost::OnRequestPermission(
 
 void GeolocationDispatcherHost::OnCancelPermissionRequest(
     int render_view_id, int bridge_id, const GURL& requesting_frame) {
-  LOG(INFO) << "cancel permission request";
+  DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_
+             << ":" << render_view_id << ":" << bridge_id;
   geolocation_permission_context_->CancelGeolocationPermissionRequest(
       resource_message_filter_process_id_, render_view_id, bridge_id,
       requesting_frame);
@@ -98,34 +101,38 @@ void GeolocationDispatcherHost::OnStartUpdating(
   // WebKit sends the startupdating request before checking permissions, to
   // optimize the no-location-available case and reduce latency in the success
   // case (location lookup happens in parallel with the permission request).
-  LOG(INFO) << "start updating" << render_view_id;
+  DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_
+             << ":" << render_view_id << ":" << bridge_id;
+  bridge_update_options_[std::make_pair(render_view_id, bridge_id)] =
+      GeolocationArbitrator::UpdateOptions(enable_high_accuracy);
   location_arbitrator_ =
       geolocation_permission_context_->StartUpdatingRequested(
           resource_message_filter_process_id_, render_view_id, bridge_id,
           requesting_frame);
-  DCHECK(location_arbitrator_);
-  location_arbitrator_->AddObserver(
-      this, GeolocationArbitrator::UpdateOptions(enable_high_accuracy));
+  RefreshUpdateOptions();
 }
 
 void GeolocationDispatcherHost::OnStopUpdating(
     int render_view_id, int bridge_id) {
-  // TODO(joth): Balance calls to RemoveObserver here with AddObserver above.
-  // http://crbug.com/40103
-  LOG(INFO) << "stop updating" << render_view_id;
+  DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_
+             << ":" << render_view_id << ":" << bridge_id;
+  if (bridge_update_options_.erase(std::make_pair(render_view_id, bridge_id)))
+    RefreshUpdateOptions();
   geolocation_permission_context_->StopUpdatingRequested(
       resource_message_filter_process_id_, render_view_id, bridge_id);
 }
 
 void GeolocationDispatcherHost::OnSuspend(
     int render_view_id, int bridge_id) {
-  LOG(INFO) << "suspend" << render_view_id;
+  DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_
+             << ":" << render_view_id << ":" << bridge_id;
   // TODO(bulach): connect this with GeolocationArbitrator.
 }
 
 void GeolocationDispatcherHost::OnResume(
     int render_view_id, int bridge_id) {
-  LOG(INFO) << "resume" << render_view_id;
+  DLOG(INFO) << __FUNCTION__ << " " << resource_message_filter_process_id_
+             << ":" << render_view_id << ":" << bridge_id;
   // TODO(bulach): connect this with GeolocationArbitrator.
 }
 
@@ -162,3 +169,16 @@ void GeolocationDispatcherHost::UnregisterDispatcher(
   DCHECK_EQ(1u, geolocation_renderer_ids_.count(render_view_id));
   geolocation_renderer_ids_.erase(render_view_id);
 }
+
+void GeolocationDispatcherHost::RefreshUpdateOptions() {
+  DCHECK(location_arbitrator_);
+  if (bridge_update_options_.empty()) {
+    location_arbitrator_->RemoveObserver(this);
+    location_arbitrator_ = NULL;
+  } else {
+    location_arbitrator_->AddObserver(
+        this,
+        GeolocationArbitrator::UpdateOptions::Collapse(
+            bridge_update_options_));
+  }
+}
diff --git a/chrome/browser/geolocation/geolocation_dispatcher_host.h b/chrome/browser/geolocation/geolocation_dispatcher_host.h
index 6cff179..31da3a6 100644
--- a/chrome/browser/geolocation/geolocation_dispatcher_host.h
+++ b/chrome/browser/geolocation/geolocation_dispatcher_host.h
@@ -5,18 +5,20 @@
 #ifndef CHROME_BROWSER_GEOLOCATION_GEOLOCATION_DISPATCHER_HOST_H_
 #define CHROME_BROWSER_GEOLOCATION_GEOLOCATION_DISPATCHER_HOST_H_
 
+#include <map>
 #include <set>
+#include <utility>
 
 #include "base/basictypes.h"
 #include "base/ref_counted.h"
 #include "chrome/browser/geolocation/location_arbitrator.h"
-#include "ipc/ipc_message.h"
 
 class GeolocationPermissionContext;
 class GURL;
 class ResourceMessageFilter;
 class URLRequestContextGetter;
 struct Geoposition;
+namespace IPC { class Message; }
 
 // GeolocationDispatcherHost is a delegate for Geolocation messages used by
 // ResourceMessageFilter.
@@ -57,6 +59,9 @@ class GeolocationDispatcherHost
   // UI thread if not already in there.
   void RegisterDispatcher(int process_id, int render_view_id);
   void UnregisterDispatcher(int process_id, int render_view_id);
+  // Updates the |location_arbitrator_| with the currently required update
+  // options, based on |bridge_update_options_|.
+  void RefreshUpdateOptions();
 
   int resource_message_filter_process_id_;
   scoped_refptr<GeolocationPermissionContext> geolocation_permission_context_;
@@ -66,6 +71,11 @@ class GeolocationDispatcherHost
   // context switches.
   // Only used on the IO thread.
   std::set<int> geolocation_renderer_ids_;
+  // Maps <renderer_id, bridge_id> to the location arbitrator update options
+  // that correspond to this particular bridge.
+  typedef std::map<std::pair<int, int>, GeolocationArbitrator::UpdateOptions>
+      BridgeUpdateOptionsMap;
+  BridgeUpdateOptionsMap bridge_update_options_;
   // Only set whilst we are registered with the arbitrator.
   scoped_refptr<GeolocationArbitrator> location_arbitrator_;
 
diff --git a/chrome/browser/geolocation/location_arbitrator.cc b/chrome/browser/geolocation/location_arbitrator.cc
index 2776ec7..810beb2 100644
--- a/chrome/browser/geolocation/location_arbitrator.cc
+++ b/chrome/browser/geolocation/location_arbitrator.cc
@@ -65,8 +65,7 @@ class GeolocationArbitratorImpl
   void RegisterProvider(LocationProviderBase* provider,
                         ScopedVector<LocationProviderBase>* provider_vector);
   void ResetProviders(ScopedVector<LocationProviderBase>* providers);
-  void CreateProvidersIfRequired();
-  bool HasHighAccuracyObserver();
+  void ModifyProvidersIfRequired();
 
   // Returns true if |new_position| is an improvement over |old_position|.
   // Set |from_same_provider| to true if both the positions came from the same
@@ -136,8 +135,8 @@ GeolocationArbitratorImpl::GeolocationArbitratorImpl(
 GeolocationArbitratorImpl::~GeolocationArbitratorImpl() {
   DCHECK(CalledOnValidThread());
   DCHECK(observers_.empty()) << "Not all observers have unregistered";
-  ResetProviders(&gps_providers_);
-  ResetProviders(&network_providers_);
+  observers_.clear();
+  ModifyProvidersIfRequired();
   DCHECK(this == g_instance_);
   g_instance_ = NULL;
 }
@@ -147,7 +146,7 @@ void GeolocationArbitratorImpl::AddObserver(
     const UpdateOptions& update_options) {
   DCHECK(CalledOnValidThread());
   observers_[delegate] = update_options;
-  CreateProvidersIfRequired();
+  ModifyProvidersIfRequired();
 
   if (position_.IsInitialized()) {
     delegate->OnLocationUpdate(position_);
@@ -158,11 +157,7 @@ bool GeolocationArbitratorImpl::RemoveObserver(
     GeolocationArbitrator::Delegate* delegate) {
   DCHECK(CalledOnValidThread());
   size_t remove = observers_.erase(delegate);
-  // TODO(joth): Delayed callback to linger before destroying providers.
-  if (!HasHighAccuracyObserver())
-    ResetProviders(&gps_providers_);
-  if (observers_.empty())
-    ResetProviders(&network_providers_);
+  ModifyProvidersIfRequired();
   return remove > 0;
 }
 
@@ -260,9 +255,15 @@ void GeolocationArbitratorImpl::ResetProviders(
   providers->reset();
 }
 
-void GeolocationArbitratorImpl::CreateProvidersIfRequired() {
+void GeolocationArbitratorImpl::ModifyProvidersIfRequired() {
   DCHECK(CalledOnValidThread());
-  DCHECK(!observers_.empty());
+  // TODO(joth): Delayed callback to linger before destroying providers.
+  if (observers_.empty()) {
+    ResetProviders(&network_providers_);
+    ResetProviders(&gps_providers_);
+    return;
+  }
+
   if (network_providers_.empty() && !request_consumer_.HasPendingRequests()) {
     // There are no network providers either created or pending creation.
     access_token_store_->LoadAccessTokens(
@@ -270,20 +271,14 @@ void GeolocationArbitratorImpl::CreateProvidersIfRequired() {
         NewCallback(this,
                     &GeolocationArbitratorImpl::OnAccessTokenStoresLoaded));
   }
-  if (gps_providers_.empty() && HasHighAccuracyObserver()) {
-    RegisterProvider(provider_factory_->NewGpsLocationProvider(),
-                     &gps_providers_);
-  }
-}
 
-bool GeolocationArbitratorImpl::HasHighAccuracyObserver() {
-  DCHECK(CalledOnValidThread());
-  for (DelegateMap::const_iterator it = observers_.begin();
-      it != observers_.end(); ++it) {
-    if (it->second.use_high_accuracy)
-      return true;
+  if (UpdateOptions::Collapse(observers_).use_high_accuracy) {
+    if (gps_providers_.empty())
+      RegisterProvider(provider_factory_->NewGpsLocationProvider(),
+                       &gps_providers_);
+  } else {
+    ResetProviders(&gps_providers_);
   }
-  return false;
 }
 
 bool GeolocationArbitratorImpl::IsNewPositionBetter(
diff --git a/chrome/browser/geolocation/location_arbitrator.h b/chrome/browser/geolocation/location_arbitrator.h
index 085e320..74403b4 100644
--- a/chrome/browser/geolocation/location_arbitrator.h
+++ b/chrome/browser/geolocation/location_arbitrator.h
@@ -70,12 +70,30 @@ class GeolocationArbitrator : public base::RefCounted<GeolocationArbitrator> {
     virtual void OnLocationUpdate(const Geoposition& position) = 0;
 
    protected:
+    Delegate() {}
     virtual ~Delegate() {}
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Delegate);
   };
   struct UpdateOptions {
     UpdateOptions() : use_high_accuracy(false) {}
     explicit UpdateOptions(bool high_accuracy)
         : use_high_accuracy(high_accuracy) {}
+
+    // Given a map<ANYTHING, UpdateOptions> this function will iterate the map
+    // and collapse all the options found to a single instance that satisfies
+    // them all.
+    template <typename MAP>
+    static UpdateOptions Collapse(const MAP& options_map) {
+      for (typename MAP::const_iterator it = options_map.begin();
+           it != options_map.end(); ++it) {
+        if (it->second.use_high_accuracy)
+          return UpdateOptions(true);
+      }
+      return UpdateOptions(false);
+    }
+
     bool use_high_accuracy;
   };
 
diff --git a/chrome/browser/geolocation/location_arbitrator_unittest.cc b/chrome/browser/geolocation/location_arbitrator_unittest.cc
index be29d50..2a4eee2 100644
--- a/chrome/browser/geolocation/location_arbitrator_unittest.cc
+++ b/chrome/browser/geolocation/location_arbitrator_unittest.cc
@@ -210,11 +210,19 @@ TEST_F(GeolocationLocationArbitratorTest,
   MockLocationObserver observer;
   arbitrator_->AddObserver(
       &observer, GeolocationArbitrator::UpdateOptions(false));
+  access_token_store_->NotifyDelegateTokensLoaded();
+  EXPECT_TRUE(providers_->cell_);
   EXPECT_FALSE(providers_->gps_);
   arbitrator_->AddObserver(
       &observer, GeolocationArbitrator::UpdateOptions(true));
+  EXPECT_TRUE(providers_->cell_);
   EXPECT_TRUE(providers_->gps_);
+  arbitrator_->AddObserver(
+      &observer, GeolocationArbitrator::UpdateOptions(false));
+  EXPECT_TRUE(providers_->cell_);
+  EXPECT_FALSE(providers_->gps_);
   EXPECT_TRUE(arbitrator_->RemoveObserver(&observer));
+  EXPECT_FALSE(providers_->cell_);
   EXPECT_FALSE(arbitrator_->RemoveObserver(&observer));
 }