Adds EngineGeolocationFeature for Blimp Geolocation project.

blimp/engine/feature/geolocation/blimp_location_provider.h

Index: blimp/engine/feature/geolocation/blimp_location_provider.h
diff --git a/blimp/engine/feature/geolocation/blimp_location_provider.h b/blimp/engine/feature/geolocation/blimp_location_provider.h
index d3d16f22a9041915f79a05dd2465a446497b9479..65601153c72d1c23eda789b41e9d67196d828711 100644
--- a/blimp/engine/feature/geolocation/blimp_location_provider.h
+++ b/blimp/engine/feature/geolocation/blimp_location_provider.h
@@ -5,6 +5,7 @@
 #ifndef BLIMP_ENGINE_FEATURE_GEOLOCATION_BLIMP_LOCATION_PROVIDER_H_
 #define BLIMP_ENGINE_FEATURE_GEOLOCATION_BLIMP_LOCATION_PROVIDER_H_
 
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/location_provider.h"
 #include "content/public/common/geoposition.h"
 
@@ -14,23 +15,44 @@ namespace engine {
 // Location provider for Blimp using the device's provider over the network.
 class BlimpLocationProvider : public content::LocationProvider {
  public:
+  // A delegate that handles outgoing geolocation messages.
+  class Delegate {

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

This interface looks exactly like the LocationProvider interface, except that
you've renamed some things, and we don't have GetPosition() nor
OnPermissionGranted() - can we follow the naming scheme of LocationProvider
instead, and just have a comment explaining that the Delegate implements a
subset of the LocationProvider's function?

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

Done.

+   public:
+    virtual ~Delegate() {}
+
+    // Notifies the client that the is listening engine is listening for
+    // geoposition information with the given accuracy.
+    virtual void UpdateListenState(bool enable_high_accuracy) = 0;
+
+    // Notifies the client that the engine is no longer listening for
+    // updates.
+    virtual void StopListenState() = 0;

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

This name is rather confusing; what does it mean to "stop listen state"?

You might reasonably replace the start/stop calls with a single
RequestAccuracy(NONE/HIGH/LOW), if that makes the code simpler.

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

It's now changed to StopListener, as per the previous comment.

Rather keep the Start/Stop functions separated, that way the
BlimpLocaitonProvider doesn't need to know about None/High/Low, if that makes
sense.

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

IIUC you're arguing that this allows you to isolate the BlimpLocationProvider
from the NONE/HIGH/LOW enums in your proto?

The BlimpLocationProvider is part of the EngineGeolocationFeature
implementation, though, so it's not unreasonable for it to know about that enum.

It's also the BlimpLocationProvider's sole function to adapt between the
content::LocationProvider API, and something more useful to the
EngineGeolocationFeature - otherwise we could just have EngineGeolocationFeature
implement the LocationProvider API itself, and have this class be a dumb
LocationProvider proxy.

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

Done.

+
+    // Requests an updated geoposition from the client.
+    virtual void RequestRefresh() = 0;
+
+    virtual void SetUpdateCallback(
+        const base::Callback<void(const content::Geoposition&)>& callback) = 0;
+
+    virtual void NotifyCallback(const content::Geoposition& position) = 0;

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

Why does the Delegate need a NotifyCallback() API?

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

Done.

+  };
+
   BlimpLocationProvider();
   ~BlimpLocationProvider() override;
 
+  void SetDelegate(base::WeakPtr<Delegate> delegate);

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

This needs a comment to explain what the |delegate| is for, and why we pass in a
WeakPtr to it.

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

Tried to explain. Please tell me if I'm off.

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

nit: Looks like you can move the |delegate| parameter to be a parameter of the
BlimpLocationProvider constructor, since you never need to construct one
_without_ a delegate, right?

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

Done.

+
   // content::LocationProvider implementation.
   bool StartProvider(bool high_accuracy) override;
   void StopProvider() override;
   void GetPosition(content::Geoposition* position) override;
   void RequestRefresh() override;
   void OnPermissionGranted() override;
-
- private:
-  void NotifyCallback(const content::Geoposition& position);
-  void OnLocationResponse(const content::Geoposition& position);
   void SetUpdateCallback(
       const LocationProviderUpdateCallback& callback) override;
 
-  LocationProviderUpdateCallback callback_;
+ private:
+  base::WeakPtr<Delegate> delegate_ = nullptr;

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

I'm surprised that this even compiles, since you're trying to initialize your
WeakPtr from a raw pointer.

WeakPtrs (like unique_ptr<>, scoped_refptr<> etc) are initialized to null by
default.

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

Didn't know that. Removing nullptr.

 
   content::Geoposition position_;

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

nit: Add a comment to explain why this is required - presumably because
GetPosition() expects us to return a cached position immediately?

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

Done.