Makes use of EngineGeolocationFeature weak_ptr threadsafe.

blimp/engine/feature/geolocation/blimp_location_provider.h

 // Copyright (c) 2016 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.
 
 #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 "blimp/common/proto/geolocation.pb.h"
 #include "device/geolocation/geoposition.h"
 #include "device/geolocation/location_provider.h"
 
 namespace blimp {
 namespace engine {
 
 // Location provider for Blimp using the device's provider over the network.
+// All calls made from BlimpLocationProvider involving |delegate_| must be
+// posted using |delegate_task_runner_|.

[Wez, 2016/09/13 20:19:07]

nit: This is part of the Delegate contract, so you don't need to stipulate it
here - just mention it in the Delegate comment, or a comment on the constructor.

[CJ, 2016/09/13 23:34:02]

Done.

 class BlimpLocationProvider : public device::LocationProvider {
  public:
   // A delegate that implements a subset of LocationProvider's functions.
+  // All methods will be executed on |delegate_task_runner_|.
   class Delegate {
    public:
     using GeopositionReceivedCallback =
         base::Callback<void(const device::Geoposition&)>;
 
     virtual ~Delegate() {}
 
     virtual void RequestAccuracy(
         GeolocationSetInterestLevelMessage::Level level) = 0;
     virtual void OnPermissionGranted() = 0;
     virtual void SetUpdateCallback(
         const GeopositionReceivedCallback& callback) = 0;
   };
 
-  explicit BlimpLocationProvider(base::WeakPtr<Delegate> delegate);
+  BlimpLocationProvider(
+      base::WeakPtr<Delegate> delegate,
+      scoped_refptr<base::SingleThreadTaskRunner> delegate_task_runner);

[Wez, 2016/09/13 20:19:07]

nit: Strictly a SequencedTaskRunner is sufficient here, since that's the
requirement WeakPtr itself has

[CJ, 2016/09/13 23:34:02]

Are you saying remove the scoped_refptr part?

[Wez, 2016/09/16 00:40:16]

So, I'm saying you should accept a scoped_refptr<SequencedTaskRunner> rather
than <SingleThreadTaskRunner>, since strictly you only need the sequenced
property, and don't care if it's a dedicated thread.

[CJ, 2016/09/16 22:55:53]

Done.

   ~BlimpLocationProvider() override;
 
   // device::LocationProvider implementation.
   bool StartProvider(bool high_accuracy) override;
   void StopProvider() override;
   const device::Geoposition& GetPosition() override;
   void OnPermissionGranted() override;
   void SetUpdateCallback(
       const LocationProviderUpdateCallback& callback) override;
 
  private:
   // This delegate handles a subset of the LocationProvider functionality.
   base::WeakPtr<Delegate> delegate_;
 
+  scoped_refptr<base::SingleThreadTaskRunner> delegate_task_runner_;
+
   device::Geoposition cached_position_;
 
   // True if a successful StartProvider call has occured.
   bool is_started_;
 
   DISALLOW_COPY_AND_ASSIGN(BlimpLocationProvider);
 };
 
 }  // namespace engine
 }  // namespace blimp
 
 #endif  // BLIMP_ENGINE_FEATURE_GEOLOCATION_BLIMP_LOCATION_PROVIDER_H_