Add a mechanism to pause and resume geolocation requests.

content/browser/geolocation/geolocation_dispatcher_host.cc

Index: content/browser/geolocation/geolocation_dispatcher_host.cc
diff --git a/content/browser/geolocation/geolocation_dispatcher_host.cc b/content/browser/geolocation/geolocation_dispatcher_host.cc
index 96071c748f5e72d5e00dce006be77a7ee903e730..4101449da03f2001139f812d6d5e964f1ebd1b9e 100644
--- a/content/browser/geolocation/geolocation_dispatcher_host.cc
+++ b/content/browser/geolocation/geolocation_dispatcher_host.cc
@@ -69,23 +69,31 @@ class GeolocationDispatcherHostImpl : public GeolocationDispatcherHost {
       bool enable_high_accuracy);
   void OnStopUpdating(int render_view_id);
 
+  virtual void PauseOrResume(int render_view_id, bool should_pause) OVERRIDE;
+
   // Updates the |geolocation_provider_| with the currently required update
-  // options, based on |renderer_high_accuracy_|.
-  void RefreshHighAccuracy();
+  // options.
+  void RefreshGeolocationOptions();
 
   void OnLocationUpdate(const Geoposition& position);
 
+  bool AreAllRenderersPaused() const;
+
   int render_process_id_;
   scoped_refptr<GeolocationPermissionContext> geolocation_permission_context_;
 
-  // Iterated when sending location updates to renderer processes. The fan out
-  // to individual bridge IDs happens renderer side, in order to minimize
-  // context switches.
+  struct RendererGeolocationOptions {
+    bool high_accuracy;
+    bool is_paused;
+  };
+
+  // Used to keep track of the renderers in this process that are using
+  // geolocation and the options associated with them. The map is iterated
+  // when a location update is available and the fan out to individual bridge
+  // IDs happens renderer side, in order to minimize context switches.
   // Only used on the IO thread.
-  std::set<int> geolocation_renderer_ids_;
-  // Maps renderer_id to whether high accuracy is requested for this particular
-  // bridge.
-  std::map<int, bool> renderer_high_accuracy_;
+  std::map<int, RendererGeolocationOptions> geolocation_renderers_;
+
   // Only set whilst we are registered with the geolocation provider.
   GeolocationProviderImpl* geolocation_provider_;
 
@@ -132,9 +140,10 @@ bool GeolocationDispatcherHostImpl::OnMessageReceived(
 void GeolocationDispatcherHostImpl::OnLocationUpdate(
     const Geoposition& geoposition) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  for (std::set<int>::iterator it = geolocation_renderer_ids_.begin();
-       it != geolocation_renderer_ids_.end(); ++it) {
-    Send(new GeolocationMsg_PositionUpdated(*it, geoposition));
+  for (std::map<int, RendererGeolocationOptions>::iterator it =
+       geolocation_renderers_.begin();
+       it != geolocation_renderers_.end(); ++it) {
+    Send(new GeolocationMsg_PositionUpdated(it->first, geoposition));
   }
 }
 
@@ -188,27 +197,48 @@ void GeolocationDispatcherHostImpl::OnStartUpdating(
   UMA_HISTOGRAM_BOOLEAN(
       "Geolocation.GeolocationDispatcherHostImpl.EnableHighAccuracy",
       enable_high_accuracy);
-  if (!geolocation_renderer_ids_.count(render_view_id))
-    geolocation_renderer_ids_.insert(render_view_id);
-
-  renderer_high_accuracy_[render_view_id] = enable_high_accuracy;
-  RefreshHighAccuracy();
+  RendererGeolocationOptions opts = { enable_high_accuracy, false };
+  geolocation_renderers_[render_view_id] = opts;
+  RefreshGeolocationOptions();
 }
 
 void GeolocationDispatcherHostImpl::OnStopUpdating(int render_view_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DVLOG(1) << __FUNCTION__ << " " << render_process_id_ << ":"
            << render_view_id;
-  if (renderer_high_accuracy_.erase(render_view_id))
-    RefreshHighAccuracy();
+  DCHECK_EQ(1U, geolocation_renderers_.count(render_view_id));
+  geolocation_renderers_.erase(render_view_id);
+  RefreshGeolocationOptions();
+}
 
-  DCHECK_EQ(1U, geolocation_renderer_ids_.count(render_view_id));
-  geolocation_renderer_ids_.erase(render_view_id);
+void GeolocationDispatcherHostImpl::PauseOrResume(int render_view_id,
+                                                  bool should_pause) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (geolocation_renderers_.find(render_view_id) ==
+      geolocation_renderers_.end())
+    return;
+
+  RendererGeolocationOptions* opts =
+      &(geolocation_renderers_[render_view_id]);

[joth, 2013/11/08 20:41:14]

nit: capture the find() result in an iterator above and use it->second here, to
avoid double lookup (and potential bug that [] can create an empty entry)

[benm (inactive), 2013/11/08 21:10:28]

Done.

+  if (opts->is_paused != should_pause) {
+    opts->is_paused = should_pause;
+    RefreshGeolocationOptions();
+  }
+}
+
+bool GeolocationDispatcherHostImpl::AreAllRenderersPaused() const {
+  std::map<int, RendererGeolocationOptions>::const_iterator i =
+      geolocation_renderers_.begin();
+  for (; i != geolocation_renderers_.end(); ++i) {
+    if (!i->second.is_paused)
+      return false;
+  }
+  return true;
 }
 
-void GeolocationDispatcherHostImpl::RefreshHighAccuracy() {
+void GeolocationDispatcherHostImpl::RefreshGeolocationOptions() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  if (renderer_high_accuracy_.empty()) {
+  if (AreAllRenderersPaused()) {

[joth, 2013/11/08 20:41:14]

Do we really need to special case this AreAllRenderersPaused upfront?

The pseudo code I wrote in previous review should naturally deal with that case
too without the need for an extra method (which also implies walking the map
twice)


bool need_updates = false;
bool use_high_accuracy = false;

for (it : geolocation_renderers_) {
  need_updates |= !(it->is_pause);
  use_high_accuracy |= it->high_accuracy;
  if (register_for_updates && use_high_accuracy)
    break;  // optimization
}

if (need_updates) {
  if (!geolocation_provider_)
    geolocation_provider_ = GeolocationProviderImpl::GetInstance();
  geolocation_provider_->AddLocationUpdateCallback(callback, use_high_accuract);
} else {
  if (geolocation_provider_)
    geolocation_provider_->RemoveLocationUpdateCallback(callback_);
  geolocation_provider_ = NULL;
}



(I'm suspicious whether the geolocation_provider_ member serves any real
purpose, but that's a different discussion)

[benm (inactive), 2013/11/08 21:10:28]

Gotcha, this makes sense. Sorry think I missed this comment with all the other
refactoring and shuffling. Cheers!

     if (geolocation_provider_) {
       geolocation_provider_->RemoveLocationUpdateCallback(callback_);
       geolocation_provider_ = NULL;
@@ -218,9 +248,10 @@ void GeolocationDispatcherHostImpl::RefreshHighAccuracy() {
       geolocation_provider_ = GeolocationProviderImpl::GetInstance();
     // Re-add to re-establish our options, in case they changed.
     bool use_high_accuracy = false;
-    std::map<int, bool>::iterator i = renderer_high_accuracy_.begin();
-    for (; i != renderer_high_accuracy_.end(); ++i) {
-      if (i->second) {
+    std::map<int, RendererGeolocationOptions>::iterator i =
+        geolocation_renderers_.begin();
+    for (; i != geolocation_renderers_.end(); ++i) {
+      if (i->second.high_accuracy) {
         use_high_accuracy = true;
         break;
       }