Dispatch geolocation IPCs on the UI thread. Aside from simplifying the code to avoid a lot of threa…

content/browser/geolocation/geolocation_dispatcher_host.cc

Index: content/browser/geolocation/geolocation_dispatcher_host.cc
===================================================================
--- content/browser/geolocation/geolocation_dispatcher_host.cc	(revision 269041)
+++ content/browser/geolocation/geolocation_dispatcher_host.cc	(working copy)
@@ -11,6 +11,8 @@
 #include "content/browser/renderer_host/render_message_filter.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/geolocation_permission_context.h"
 #include "content/public/common/geoposition.h"
 #include "content/common/geolocation_messages.h"
@@ -61,16 +63,10 @@
                             GEOPOSITION_ERROR_CODE_COUNT);
 }
 
-void NotifyGeolocationProviderPermissionGranted() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  GeolocationProviderImpl::GetInstance()->UserDidOptIntoLocationServices();
-}
-
 void SendGeolocationPermissionResponse(int render_process_id,
                                        int render_view_id,
                                        int bridge_id,
                                        bool allowed) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   RenderViewHostImpl* render_view_host =
       RenderViewHostImpl::FromID(render_process_id, render_view_id);
   if (!render_view_host)
@@ -78,41 +74,37 @@
   render_view_host->Send(
       new GeolocationMsg_PermissionSet(render_view_id, bridge_id, allowed));
 
-  if (allowed) {
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        base::Bind(&NotifyGeolocationProviderPermissionGranted));
-  }
+  if (allowed)
+    GeolocationProviderImpl::GetInstance()->UserDidOptIntoLocationServices();
 }
 
 }  // namespace
 
 GeolocationDispatcherHost::GeolocationDispatcherHost(
-    int render_process_id,
-    GeolocationPermissionContext* geolocation_permission_context)
-    : BrowserMessageFilter(GeolocationMsgStart),
-      render_process_id_(render_process_id),
-      geolocation_permission_context_(geolocation_permission_context),
-      geolocation_provider_(NULL) {
-  callback_ = base::Bind(
-      &GeolocationDispatcherHost::OnLocationUpdate, base::Unretained(this));
-  // This is initialized by ResourceMessageFilter. Do not add any non-trivial
+    WebContents* web_contents)
+    : WebContentsObserver(web_contents),
+      watching_requested_(false),
+      paused_(false),
+      high_accuracy_(false) {
+  // This is initialized by WebContentsImpl. Do not add any non-trivial
   // initialization here, defer to OnRegisterBridge which is triggered whenever
   // a javascript geolocation object is actually initialized.
 }
 
 GeolocationDispatcherHost::~GeolocationDispatcherHost() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  if (geolocation_provider_)
-    geolocation_provider_->RemoveLocationUpdateCallback(callback_);
 }
 
-bool GeolocationDispatcherHost::OnMessageReceived(
-    const IPC::Message& msg, bool* msg_was_ok) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  *msg_was_ok = true;
+void GeolocationDispatcherHost::RenderViewHostChanged(
+    RenderViewHost* old_host,
+    RenderViewHost* new_host) {
+  watching_requested_ = false;
+  paused_ = false;
+  geolocation_subscription_.reset();
+}
+
+bool GeolocationDispatcherHost::OnMessageReceived(const IPC::Message& msg) {
   bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP_EX(GeolocationDispatcherHost, msg, *msg_was_ok)
+  IPC_BEGIN_MESSAGE_MAP(GeolocationDispatcherHost, msg)
     IPC_MESSAGE_HANDLER(GeolocationHostMsg_CancelPermissionRequest,
                         OnCancelPermissionRequest)
     IPC_MESSAGE_HANDLER(GeolocationHostMsg_RequestPermission,
@@ -124,143 +116,87 @@
   return handled;
 }
 
-void GeolocationDispatcherHost::OnLocationUpdate(
-    const Geoposition& geoposition) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  RecordGeopositionErrorCode(geoposition.error_code);
-  for (std::map<int, RendererGeolocationOptions>::iterator it =
-       geolocation_renderers_.begin();
-       it != geolocation_renderers_.end(); ++it) {
-    if (!(it->second.is_paused))
-      Send(new GeolocationMsg_PositionUpdated(it->first, geoposition));
-  }
-}
-
 void GeolocationDispatcherHost::OnRequestPermission(
-    int render_view_id,
     int bridge_id,
     const GURL& requesting_frame,
     bool user_gesture) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DVLOG(1) << __FUNCTION__ << " " << render_process_id_ << ":"
-           << render_view_id << ":" << bridge_id;
-  if (geolocation_permission_context_.get()) {
-    geolocation_permission_context_->RequestGeolocationPermission(
-        render_process_id_,
-        render_view_id,
+  GeolocationPermissionContext* context =
+      web_contents()->GetBrowserContext()->GetGeolocationPermissionContext();
+  int render_process_id = web_contents()->GetRenderProcessHost()->GetID();
+  int render_view_id = web_contents()->GetRenderViewHost()->GetRoutingID();
+  if (context) {
+    context->RequestGeolocationPermission(
+        web_contents(),
         bridge_id,
         requesting_frame,
         user_gesture,
         base::Bind(&SendGeolocationPermissionResponse,
-                   render_process_id_,
+                   render_process_id,
                    render_view_id,
                    bridge_id));
   } else {
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        base::Bind(&SendGeolocationPermissionResponse, render_process_id_,
-                   render_view_id, bridge_id, true));
+    SendGeolocationPermissionResponse(
+        render_process_id, render_view_id, bridge_id, true);
   }
 }
 
 void GeolocationDispatcherHost::OnCancelPermissionRequest(
-    int render_view_id,
     int bridge_id,
     const GURL& requesting_frame) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DVLOG(1) << __FUNCTION__ << " " << render_process_id_ << ":"
-           << render_view_id << ":" << bridge_id;
-  if (geolocation_permission_context_.get()) {
-    geolocation_permission_context_->CancelGeolocationPermissionRequest(
-        render_process_id_, render_view_id, bridge_id, requesting_frame);
+  GeolocationPermissionContext* context =
+      web_contents()->GetBrowserContext()->GetGeolocationPermissionContext();
+  if (context) {
+    context->CancelGeolocationPermissionRequest(
+        web_contents(), bridge_id, requesting_frame);
   }
 }
 
 void GeolocationDispatcherHost::OnStartUpdating(
-    int render_view_id,
     const GURL& requesting_frame,
     bool enable_high_accuracy) {
   // StartUpdating() can be invoked as a result of high-accuracy mode
   // being enabled / disabled. No need to record the dispatcher again.
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DVLOG(1) << __FUNCTION__ << " " << render_process_id_ << ":"
-           << render_view_id;
   UMA_HISTOGRAM_BOOLEAN(
       "Geolocation.GeolocationDispatcherHostImpl.EnableHighAccuracy",
       enable_high_accuracy);
 
-  std::map<int, RendererGeolocationOptions>::iterator it =
-            geolocation_renderers_.find(render_view_id);
-  if (it == geolocation_renderers_.end()) {
-    bool should_start_paused = false;
-    if (pending_paused_geolocation_renderers_.erase(render_view_id) == 1) {
-      should_start_paused = true;
-    }
-    RendererGeolocationOptions opts = {
-      enable_high_accuracy,
-      should_start_paused
-    };
-    geolocation_renderers_[render_view_id] = opts;
-  } else {
-    it->second.high_accuracy = enable_high_accuracy;
-  }
+  watching_requested_ = true;
+  high_accuracy_ = enable_high_accuracy;
   RefreshGeolocationOptions();
 }
 
-void GeolocationDispatcherHost::OnStopUpdating(int render_view_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DVLOG(1) << __FUNCTION__ << " " << render_process_id_ << ":"
-           << render_view_id;
-  DCHECK_EQ(1U, geolocation_renderers_.count(render_view_id));
-  geolocation_renderers_.erase(render_view_id);
+void GeolocationDispatcherHost::OnStopUpdating() {
+  watching_requested_ = false;
   RefreshGeolocationOptions();
 }
 
-void GeolocationDispatcherHost::PauseOrResume(int render_view_id,
-                                              bool should_pause) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  std::map<int, RendererGeolocationOptions>::iterator it =
-      geolocation_renderers_.find(render_view_id);
-  if (it == geolocation_renderers_.end()) {
-    // This renderer is not using geolocation yet, but if it does before
-    // we get a call to resume, we should start it up in the paused state.
-    if (should_pause) {
-      pending_paused_geolocation_renderers_.insert(render_view_id);
-    } else {
-      pending_paused_geolocation_renderers_.erase(render_view_id);
-    }
-  } else {
-    RendererGeolocationOptions* opts = &(it->second);
-    if (opts->is_paused != should_pause)
-      opts->is_paused = should_pause;
-    RefreshGeolocationOptions();
-  }
+void GeolocationDispatcherHost::PauseOrResume(bool should_pause) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  paused_ = should_pause;
+  RefreshGeolocationOptions();
 }
 
 void GeolocationDispatcherHost::RefreshGeolocationOptions() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  bool needs_updates = false;
-  bool use_high_accuracy = false;
-  std::map<int, RendererGeolocationOptions>::const_iterator i =
-      geolocation_renderers_.begin();
-  for (; i != geolocation_renderers_.end(); ++i) {
-    needs_updates |= !(i->second.is_paused);
-    use_high_accuracy |= i->second.high_accuracy;
-    if (needs_updates && use_high_accuracy)
-      break;
-  }
-  if (needs_updates) {
-    if (!geolocation_provider_)
-      geolocation_provider_ = GeolocationProviderImpl::GetInstance();
-    // Re-add to re-establish our options, in case they changed.
-    geolocation_provider_->AddLocationUpdateCallback(
-        callback_, use_high_accuracy);
+  if (watching_requested_ & !paused_) {
+    geolocation_subscription_ = GeolocationProvider::GetInstance()->
+        AddLocationUpdateCallback(
+            base::Bind(&GeolocationDispatcherHost::OnLocationUpdate,
+                       base::Unretained(this)),
+            high_accuracy_);
   } else {
-    if (geolocation_provider_)
-      geolocation_provider_->RemoveLocationUpdateCallback(callback_);
-    geolocation_provider_ = NULL;
+    geolocation_subscription_.reset();
   }
 }
 
+void GeolocationDispatcherHost::OnLocationUpdate(

[Michael van Ouwerkerk, 2014/05/08 13:20:02]

nit: if this method remains before OnRequestPermission the diff might read more
easily

[jam, 2014/05/08 15:04:58]

Done.

+    const Geoposition& geoposition) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  RecordGeopositionErrorCode(geoposition.error_code);
+  if (!paused_)
+    Send(new GeolocationMsg_PositionUpdated(routing_id(), geoposition));
+}
+
 }  // namespace content