Adds EngineGeolocationFeature for Blimp Geolocation project.

blimp/engine/feature/geolocation/blimp_location_provider.cc

Index: blimp/engine/feature/geolocation/blimp_location_provider.cc
diff --git a/blimp/engine/feature/geolocation/blimp_location_provider.cc b/blimp/engine/feature/geolocation/blimp_location_provider.cc
index de86b9fc1e65b31384c8cbdd43cca52128e388ca..7c78cde4c9178d4e46432862e41fc2401153dca2 100644
--- a/blimp/engine/feature/geolocation/blimp_location_provider.cc
+++ b/blimp/engine/feature/geolocation/blimp_location_provider.cc
@@ -4,6 +4,9 @@
 
 #include "blimp/engine/feature/geolocation/blimp_location_provider.h"
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/common/geoposition.h"
 
 namespace blimp {
@@ -15,12 +18,25 @@ BlimpLocationProvider::~BlimpLocationProvider() {
   StopProvider();
 }
 
+void BlimpLocationProvider::SetDelegate(
+    base::WeakPtr<BlimpLocationProvider::Delegate> delegate) {
+  DCHECK(delegate);
+  DCHECK(!delegate_);
+
+  delegate_ = delegate;
+}
+
 bool BlimpLocationProvider::StartProvider(bool high_accuracy) {
-  NOTIMPLEMENTED();
+  DCHECK(delegate_);

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

|delegate_| is a WeakPtr, so it's perfectly valid for it to be null, surely?

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

Done.

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

I meant that, if it is possible for the WeakPtr to have become null by this
point then you must check it before you try to use it.

All WeakPtr does is allow the owner to "invalidate" it, after which it behaves
like a null pointer - callers can check it each time they're about to use it and
skip doing work that would use the pointer if it has become null.

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

Done.

+
+  delegate_->UpdateListenState(high_accuracy);
   return true;
 }
 
 void BlimpLocationProvider::StopProvider() {
+  DCHECK(delegate_);
+
+  delegate_->StopListenState();
 }
 
 void BlimpLocationProvider::GetPosition(content::Geoposition* position) {
@@ -30,7 +46,9 @@ void BlimpLocationProvider::GetPosition(content::Geoposition* position) {
 }
 
 void BlimpLocationProvider::RequestRefresh() {
-  NOTIMPLEMENTED();
+  DCHECK(delegate_);
+
+  delegate_->RequestRefresh();
 }
 
 void BlimpLocationProvider::OnPermissionGranted() {
@@ -38,24 +56,11 @@ void BlimpLocationProvider::OnPermissionGranted() {
   NOTIMPLEMENTED();

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

Add a comment to explain why we're both requesting a refresh AND saying
NOTIMPLEMENTED here?

What is this API actually supposed to do?  Are we not supposed to start
generating output until we get this call, for example..?

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

I think I can remove the NOTIMPLEMENTED here. 

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

Acknowledged.

 }
 
-void BlimpLocationProvider::NotifyCallback(
-    const content::Geoposition& position) {
-  DCHECK(!callback_.is_null());
-
-  callback_.Run(this, position);
-}
-
-void BlimpLocationProvider::OnLocationResponse(
-    const content::Geoposition& position) {
-  NotifyCallback(position);
-  NOTIMPLEMENTED();
-}
-
 void BlimpLocationProvider::SetUpdateCallback(
     const LocationProviderUpdateCallback& callback) {
   DCHECK(!callback.is_null());

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

LocationProvider does not specify that it's invalid to provide a null callback,
and if you look at the base implementation, it actually explicitly allows for it
being null - so your DCHECK is enforcing a requirement that callers don't expect
to have to meet.

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

Done.

 
-  callback_ = callback;
+  delegate_->SetUpdateCallback(base::Bind(callback, base::Unretained(this)));

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

You're passing Bind()ing to Unretained(this) - what happens if you are deleted
before the Delegate? Might it still send you another event, and thereby cause
you to crash?

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

I don't think that could happen? BlimpLocationProvider is owned by
LocationArbitratorImpl, where this callback eventually ends up. The
BlimpLocationProvider is given to be the first argument to OnLocationResponse.
If there is a concern, wouldn't we just want to add a check in
OnLocationResponse?

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

Not sure what you mean by "ends up". You're passing the callback to the
Delegate, i.e. to the Feature - what stops the Feature calling it after the
LocationArbitratorImpl has torn-down the BlimpLocationProvider?
Ah, so you are arguing that OnLocationResponse can cope with being called with a
pointer to a dead LocationProvider - which is the case if it really only uses it
to compare with its internal list of providers?

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

I believe so yes. It just uses that pointer to update it's position_provider
field. 
https://cs.chromium.org/chromium/src/content/browser/geolocation/location_arb...

So should I put a check in OnLocationResponse? 

 }
 
 }  // namespace engine