Adds EngineGeolocationFeature for Blimp Geolocation project.

blimp/engine/app/blimp_content_browser_client.cc

 // Copyright (c) 2015 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 "base/memory/weak_ptr.h"
 #include "blimp/engine/app/blimp_content_browser_client.h"
 #include "blimp/engine/app/blimp_browser_main_parts.h"
 #include "blimp/engine/app/settings_manager.h"
 #include "blimp/engine/feature/geolocation/blimp_location_provider.h"
+#include "blimp/engine/feature/geolocation/engine_geolocation_feature.h"
 #include "blimp/engine/mojo/blob_channel_service.h"
+#include "blimp/engine/session/blimp_engine_session.h"
 #include "content/public/browser/geolocation_delegate.h"
 #include "services/shell/public/cpp/interface_registry.h"
 
 namespace blimp {
 namespace engine {
 
 namespace {
 // A provider of services needed by Geolocation.
 class BlimpGeolocationDelegate : public content::GeolocationDelegate {
  public:
-  BlimpGeolocationDelegate() = default;
+  explicit BlimpGeolocationDelegate(
+      base::WeakPtr<EngineGeolocationFeature> feature) {

[Wez, 2016/07/12 21:35:12]

nit: Looks like you mean to just pass WeakPtr<BlimpLocationProvider::Delegate>
here (in which case rename parameter to feature_delegate or something).

[CJ, 2016/07/13 21:49:19]

Done.

+    feature_ = feature;
+  }
 
   bool UseNetworkLocationProviders() final { return false; }
 
-  std::unique_ptr<content::LocationProvider> OverrideSystemLocationProvider()
-      final {
-    return base::WrapUnique(new BlimpLocationProvider());
+  std::unique_ptr<content::LocationProvider>
+      OverrideSystemLocationProvider() final {
+    BlimpLocationProvider* location_provider = new BlimpLocationProvider();
+    location_provider->SetDelegate(feature_);
+    return base::WrapUnique(location_provider);
   }
 
  private:
+  base::WeakPtr<BlimpLocationProvider::Delegate> feature_;

[Wez, 2016/07/12 21:35:12]

You're assigning this from an EngineGeolocationFeature, above, though - are you
sure this compiles?

[CJ, 2016/07/13 21:49:19]

It does. EngineGeolocationFeature is a BlimpLocationProvider::Delegate. 

[Wez, 2016/07/14 01:19:21]

Right, but it's strange to require that the caller pass an
EngineGeolocationFeature pointer, but then to store it as a less-specific thing
- either you need to have an EngineGeolocationFeature, or you can cope with any
BlimpLocationProvider::Delegate

(this would be more relevant if this class were to get its own set of
unit-tests, since being able to pass a BlimpLocationProvider::Delegate would
enable you to pass it a mock)

[CJ, 2016/07/14 23:55:08]

Acknowledged.

+
   DISALLOW_COPY_AND_ASSIGN(BlimpGeolocationDelegate);
 };
 
 }  // anonymous namespace
 
 BlimpContentBrowserClient::BlimpContentBrowserClient() {}
 
 BlimpContentBrowserClient::~BlimpContentBrowserClient() {}
 
 content::BrowserMainParts* BlimpContentBrowserClient::CreateBrowserMainParts(
@@ skipping 15 matching lines @@
   blimp_browser_main_parts_->GetSettingsManager()->UpdateWebkitPreferences(
       prefs);
 }
 
 BlimpBrowserContext* BlimpContentBrowserClient::GetBrowserContext() {
   return blimp_browser_main_parts_->GetBrowserContext();
 }
 
 content::GeolocationDelegate*
 BlimpContentBrowserClient::CreateGeolocationDelegate() {
-  return new BlimpGeolocationDelegate();
+  EngineGeolocationFeature* geolocation_feature = blimp_browser_main_parts()
+                                                      ->GetBlimpEngineSession()
+                                                      ->GetGeolocationFeature();

[Wez, 2016/07/12 21:35:12]

nit: Is this the git cl format preferred line-wrap?

[CJ, 2016/07/13 21:49:19]

Yes.

+  BlimpGeolocationDelegate* delegate =
+      new BlimpGeolocationDelegate(geolocation_feature->GetWeakPtr());

[Wez, 2016/07/12 21:35:13]

Rather than expose WeakPtr<EngineGelocationFeature> outside the feature, why not
move BlimpGeolocationDelegate to be an internal implementation detail of
EngineGeolocationFeature, since that's effectively what it is?

So then EGFeature just has a CreateGeolocationDelegate() method.

[CJ, 2016/07/13 21:49:19]

BlimpGeolocationDelegate has nothing to do with EngineGeolocationFeature except
that's where I happen to be hooking BlimpLocationProvider and
EngineGeolocationFeature together. BlimpGeolocationDelegate has all the
previously content related functions in it. I am a little confused. 

[Wez, 2016/07/14 01:19:21]

I'm not sure what you mean by "has nothing to do with" -
EngineGeolocationFeature encapsulates the Engine-side geolocation
implementation, of which our content::GeolocationDelegate and
content::LocationProvider implementations are a part. They seem pretty closely
related. :P

[CJ, 2016/07/14 23:55:08]

Done.

+  return delegate;
 }
 
 void BlimpContentBrowserClient::ExposeInterfacesToRenderer(
     shell::InterfaceRegistry* registry,
     content::RenderProcessHost* render_process_host) {
   registry->AddInterface<mojom::BlobChannel>(
       base::Bind(&BlobChannelService::Create,
                  blimp_browser_main_parts_->GetBlobChannelSender()));
 }
 
 }  // namespace engine
 }  // namespace blimp