Geolocation: change GeolocationDelegate to injected by content/

content/browser/geolocation/geolocation_provider_impl.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_provider_impl.h"
 
 #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"
 #include "base/memory/singleton.h"
 #include "base/single_thread_task_runner.h"
 #include "content/browser/geolocation/location_arbitrator_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/geolocation_delegate.h"
 
 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,
     bool enable_high_accuracy) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   std::unique_ptr<GeolocationProvider::Subscription> subscription;
   if (enable_high_accuracy) {
     subscription = high_accuracy_callbacks_.Add(callback);
   } else {
     subscription = low_accuracy_callbacks_.Add(callback);
@@ skipping 27 matching lines @@
   DCHECK(OnGeolocationThread());
   // Will be true only in testing.
   if (ignore_location_updates_)
     return;
   BrowserThread::PostTask(BrowserThread::UI,
                           FROM_HERE,
                           base::Bind(&GeolocationProviderImpl::NotifyClients,
                                      base::Unretained(this), position));
 }
 
+// static
 GeolocationProviderImpl* GeolocationProviderImpl::GetInstance() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return base::Singleton<GeolocationProviderImpl>::get();
 }
 
 GeolocationProviderImpl::GeolocationProviderImpl()
     : base::Thread("Geolocation"),
       user_did_opt_into_location_services_(false),
       ignore_location_updates_(false) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ skipping 86 matching lines @@
 
 void GeolocationProviderImpl::CleanUp() {
   DCHECK(OnGeolocationThread());
   arbitrator_.reset();
 }
 
 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