Add content API for requesting the current geolocation

content/browser/geolocation/geolocation_provider.cc

Index: content/browser/geolocation/geolocation_provider.cc
diff --git a/content/browser/geolocation/geolocation_provider.cc b/content/browser/geolocation/geolocation_provider.cc
index d61c7c7d064244162350a2d4e0b73b35095e05b0..6ea52f14f5d340a292ab62e26d1e80125f4ee9c9 100644
--- a/content/browser/geolocation/geolocation_provider.cc
+++ b/content/browser/geolocation/geolocation_provider.cc
@@ -6,49 +6,66 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/logging.h"
 #include "base/memory/singleton.h"
-#include "base/threading/thread_restrictions.h"
+#include "base/message_loop.h"
 #include "content/browser/geolocation/location_arbitrator.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
 
 GeolocationProvider* GeolocationProvider::GetInstance() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   return Singleton<GeolocationProvider>::get();
 }
 
 GeolocationProvider::GeolocationProvider()
     : base::Thread("Geolocation"),
-      client_loop_(base::MessageLoopProxy::current()),
       is_permission_granted_(false),
       ignore_location_updates_(false),
       arbitrator_(NULL) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 }
 
 GeolocationProvider::~GeolocationProvider() {
-  DCHECK(observers_.empty());  // observers must unregister.
+  // All observers should have unregistered before this singleton is destructed.
+  DCHECK(observers_.empty());
   Stop();
   DCHECK(!arbitrator_);
 }
 
 void GeolocationProvider::AddObserver(GeolocationObserver* observer,
     const GeolocationObserverOptions& update_options) {
-  DCHECK(OnClientThread());
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   observers_[observer] = update_options;
-  OnObserversChanged();
+  OnClientsChanged();
   if (position_.Validate() ||
       position_.error_code != content::Geoposition::ERROR_CODE_NONE)
     observer->OnLocationUpdate(position_);
 }
 
 bool GeolocationProvider::RemoveObserver(GeolocationObserver* observer) {
-  DCHECK(OnClientThread());
-  size_t remove = observers_.erase(observer);
-  OnObserversChanged();
-  return remove > 0;
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  size_t removed = observers_.erase(observer);
+  if (removed)
+    OnClientsChanged();
+  return removed > 0;
+}
+
+void GeolocationProvider::RequestCallback(
+    const content::GeolocationUpdateCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  callbacks_.push_back(callback);
+  OnClientsChanged();
+  OnPermissionGranted();
 }
 
-void GeolocationProvider::OnObserversChanged() {
-  DCHECK(OnClientThread());
+void GeolocationProvider::OnClientsChanged() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   base::Closure task;
-  if (observers_.empty()) {
+  if (observers_.empty() && callbacks_.empty()) {
     DCHECK(IsRunning());
     task = base::Bind(&GeolocationProvider::StopProviders,
                       base::Unretained(this));
@@ -58,29 +75,43 @@ void GeolocationProvider::OnObserversChanged() {
       if (HasPermissionBeenGranted())
         InformProvidersPermissionGranted();
     }
-
-    // The high accuracy requirement may have changed.
+    // Determine a set of options that satisfies all clients.
+    GeolocationObserverOptions options =
+        GeolocationObserverOptions::Collapse(observers_);
+    // For callbacks, high accuracy position information is always requested.
+    if (!callbacks_.empty())
+      options.Collapse(GeolocationObserverOptions(true));
+
+    // Send the current options to the providers as they may have changed.
     task = base::Bind(&GeolocationProvider::StartProviders,
                       base::Unretained(this),
-                      GeolocationObserverOptions::Collapse(observers_));
+                      options);
   }
 
   message_loop()->PostTask(FROM_HERE, task);
 }
 
-void GeolocationProvider::NotifyObservers(
-    const content::Geoposition& position) {
-  DCHECK(OnClientThread());
+void GeolocationProvider::NotifyClients(const content::Geoposition& position) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(position.Validate() ||
          position.error_code != content::Geoposition::ERROR_CODE_NONE);
   position_ = position;
   ObserverMap::const_iterator it = observers_.begin();
   while (it != observers_.end()) {
-    // Advance iterator before callback to guard against synchronous unregister.
+    // Advance iterator before calling the observer to guard against synchronous
+    // unregister.
     GeolocationObserver* observer = it->first;
     ++it;
     observer->OnLocationUpdate(position_);
   }
+  if (!callbacks_.empty()) {
+    for (CallbackList::iterator it = callbacks_.begin();
+         it != callbacks_.end();
+         ++it)
+      it->Run(position);

[John Knottenbelt, 2012/05/04 10:16:58]

It seems possible, perhaps likely, that a callback may call
GeolocationProvider::RequestCallback which could result in the vector being
reallocated and so the iterators may be invalidated.

I can think of two ways to avoid this problem. 
  - instead of using an iterator, use operator [] and index variable. callbacks
registered during callback invocation will be called in the same pass.
  - before iterating through the vector, swap it with an empty one. callbacks
registered during the callback invocation will be called at the next update.

I think that the second option makes more sense than the first.

[bartfab (slow), 2012/05/04 10:35:58]

Nice catch. Done.

+    callbacks_.clear();
+    OnClientsChanged();
+  }
 }
 
 void GeolocationProvider::StartProviders(
@@ -97,9 +128,10 @@ void GeolocationProvider::StopProviders() {
 }
 
 void GeolocationProvider::OnPermissionGranted() {
-  DCHECK(OnClientThread());
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  bool was_permission_granted = is_permission_granted_;
   is_permission_granted_ = true;
-  if (IsRunning())
+  if (IsRunning() && !was_permission_granted)
     InformProvidersPermissionGranted();
 }
 
@@ -135,29 +167,25 @@ void GeolocationProvider::OnLocationUpdate(
   // Will be true only in testing.
   if (ignore_location_updates_)
     return;
-  client_loop_->PostTask(
-      FROM_HERE,
-      base::Bind(&GeolocationProvider::NotifyObservers,
-                 base::Unretained(this), position));
+  BrowserThread::PostTask(BrowserThread::IO,
+                          FROM_HERE,
+                          base::Bind(&GeolocationProvider::NotifyClients,
+                                     base::Unretained(this), position));
 }
 
 void GeolocationProvider::OverrideLocationForTesting(
     const content::Geoposition& position) {
-  DCHECK(OnClientThread());
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   position_ = position;
   ignore_location_updates_ = true;
-  NotifyObservers(position);
+  NotifyClients(position);
 }
 
 bool GeolocationProvider::HasPermissionBeenGranted() const {
-  DCHECK(OnClientThread());
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   return is_permission_granted_;
 }
 
-bool GeolocationProvider::OnClientThread() const {
-  return client_loop_->BelongsToCurrentThread();
-}
-
 bool GeolocationProvider::OnGeolocationThread() const {
   return MessageLoop::current() == message_loop();
 }