Geolocation: change GeolocationDelegate to injected by content/

content/browser/geolocation/geolocation_provider_impl.cc

Index: content/browser/geolocation/geolocation_provider_impl.cc
diff --git a/content/browser/geolocation/geolocation_provider_impl.cc b/content/browser/geolocation/geolocation_provider_impl.cc
index 2343fa7d816464d9c637db2ed8a709e229f73283..4a35358e948df93e0522b316c6bd96b4c7acaad8 100644
--- a/content/browser/geolocation/geolocation_provider_impl.cc
+++ b/content/browser/geolocation/geolocation_provider_impl.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -19,10 +20,23 @@
 
 namespace content {
 
+namespace {
+base::LazyInstance<std::unique_ptr<GeolocationDelegate>>::Leaky g_delegate =
+    LAZY_INSTANCE_INITIALIZER;

[Wez, 2016/07/25 22:25:35]

Why do we need a LazyInstance here, rather than a bare-pointer?

[mcasas, 2016/07/26 18:46:34]

Essentially, because I wanted the |g_delegate| to clean
itself up during shutdown.  Using a naked pointer would
have implied making it a member variable of 
GeolocationProvider to clean it up, but that'd have forced
the creation of the (evil?) singleton...

[Wez, 2016/07/26 19:08:31]

If you want it cleaned up on shutdown then you either want a Singleton or a
non-Leaky LazyInstance.  What you have right now won't clean up on shutdown,
AFAIK.

Also I'm not sure that we actually instantiate AtExitManager in the Browser
process; I think we may deliberately skip destructing Singletons etc on browser
teardown because it usually just generates unnecessary memory paging, to no
purpose.

+}  // anonymous namespace
+
+// static
 GeolocationProvider* GeolocationProvider::GetInstance() {
   return GeolocationProviderImpl::GetInstance();
 }
 
+// static
+void GeolocationProvider::SetGeolocationDelegate(
+    GeolocationDelegate* delegate) {
+  if (!g_delegate.Pointer())

[Ken Rockot(use gerrit already), 2016/07/20 17:47:52]

g_delegate.Pointer() is always true. It's the address of the lazy unique_ptr
object.

You want to test g_delegate.Get(). And really this should be a DCHECK since it's
almost definitely an error to call SetGeolocationDelegate more than once in a
process.

[mcasas, 2016/07/20 20:47:08]

Done.

+    g_delegate.Pointer()->reset(delegate);

[Ken Rockot(use gerrit already), 2016/07/20 17:47:52]

optional nit: I think the above is evidence that using Pointer() on a
LazyInstance<unique_ptr<T>> is confusing. IMHO you should just use Get()
everywhere. e.g.:

  g_delegate.Get().reset(delegate);

[mcasas, 2016/07/20 20:47:08]

Done.

+}
+
 std::unique_ptr<GeolocationProvider::Subscription>
 GeolocationProviderImpl::AddLocationUpdateCallback(
     const LocationUpdateCallback& callback,
@@ -70,6 +84,7 @@ void GeolocationProviderImpl::OnLocationUpdate(const Geoposition& position) {
                                      base::Unretained(this), position));
 }
 
+// static
 GeolocationProviderImpl* GeolocationProviderImpl::GetInstance() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return base::Singleton<GeolocationProviderImpl>::get();
@@ -176,14 +191,12 @@ std::unique_ptr<LocationArbitrator>
 GeolocationProviderImpl::CreateArbitrator() {
   LocationArbitratorImpl::LocationUpdateCallback callback = base::Bind(
       &GeolocationProviderImpl::OnLocationUpdate, base::Unretained(this));
-
-  // Use the embedder's Delegate or fall back to the default one.
-  delegate_.reset(GetContentClient()->browser()->CreateGeolocationDelegate());
-  if (!delegate_)
-    delegate_.reset(new GeolocationDelegate);
+  // Use the embedder's |g_delegate| or fall back to the default one.
+  if (!g_delegate.Get())
+    g_delegate.Pointer()->reset(new GeolocationDelegate);
 
   return base::WrapUnique(
-      new LocationArbitratorImpl(callback, delegate_.get()));
+      new LocationArbitratorImpl(callback, g_delegate.Pointer()->get()));
 }
 
 }  // namespace content