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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/geolocation/geolocation_dispatcher_host.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/metrics/histogram.h"
 #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"
 
 namespace content {
 namespace {
 
 // Geoposition error codes for reporting in UMA.
 enum GeopositionErrorCode {
   // NOTE: Do not renumber these as that would confuse interpretation of
@@ skipping 30 matching lines @@
       break;
     case Geoposition::ERROR_CODE_TIMEOUT:
       code = GEOPOSITION_ERROR_CODE_TIMEOUT;
       break;
   }
   UMA_HISTOGRAM_ENUMERATION("Geolocation.LocationUpdate.ErrorCode",
                             code,
                             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)
     return;
   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

[bulach, 2014/05/08 17:29:26]

nit: while at it if you don't mind: OnStartUpdating (RegisterBridge is long
gone...)

[jam, 2014/05/08 19:09:53]

Done.

   // 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,
                         OnRequestPermission)
     IPC_MESSAGE_HANDLER(GeolocationHostMsg_StartUpdating, OnStartUpdating)
     IPC_MESSAGE_HANDLER(GeolocationHostMsg_StopUpdating, OnStopUpdating)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void GeolocationDispatcherHost::OnLocationUpdate(
     const Geoposition& geoposition) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[bulach, 2014/05/08 17:29:26]

probably a spurious optimization, but anyways...
we could potentially register the callback and tell the GeolocationProvider
which thread we want to be dispatched?
it'd avoid hopping here from the geolocation thread to ui to io..

[jam, 2014/05/08 19:09:53]

In general, I think code should live as much as possible on the UI thread.
Developers shouldn't have to worry about the extra thread hop cost when sending
an IPC from the UI thread (for almost all code). i.e. in this case, a
geolocation update is pretty rare, and expensive one to get, so the thread hop
cost is trivial in comparison. Also, to do this from geolocation to IO would
mean needing to keep a reference to the BrowserMessageFilter since this class
isn't thread safe; adding more complexity. Finally, this wouldn't work with out
of process iframes where this class will probably send the update to multiple
processes.

[bulach, 2014/05/09 09:50:17]

thanks for the detailed explanation! I agree it's better as you have it, thanks!

+
   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));
-  }
+  if (!paused_)
+    Send(new GeolocationMsg_PositionUpdated(routing_id(), 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);
-    }
+void GeolocationDispatcherHost::PauseOrResume(bool should_pause) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  paused_ = should_pause;
+  RefreshGeolocationOptions();
+}
+
+void GeolocationDispatcherHost::RefreshGeolocationOptions() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (watching_requested_ & !paused_) {

[bulach, 2014/05/08 17:29:26]

nit: s/&/&&/

[jam, 2014/05/08 19:09:53]

oops, nice catch, thanks.

+    geolocation_subscription_ = GeolocationProvider::GetInstance()->
+        AddLocationUpdateCallback(
+            base::Bind(&GeolocationDispatcherHost::OnLocationUpdate,
+                       base::Unretained(this)),
+            high_accuracy_);
   } else {
-    RendererGeolocationOptions* opts = &(it->second);
-    if (opts->is_paused != should_pause)
-      opts->is_paused = should_pause;
-    RefreshGeolocationOptions();
+    geolocation_subscription_.reset();
   }
 }
 
-void GeolocationDispatcherHost::RefreshGeolocationOptions() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
-  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);
-  } else {
-    if (geolocation_provider_)
-      geolocation_provider_->RemoveLocationUpdateCallback(callback_);
-    geolocation_provider_ = NULL;
-  }
-}
-
 }  // namespace content