Address Robin's code review comments in initial FlpHal submission.

Change-Id: I50889599fdc5938a19b8bff4f11e64f44bcebdbf

6 files changed, 156 insertions, 54 deletions

diff --git a/location/lib/java/com/android/location/provider/FusedLocationHardware.java b/location/lib/java/com/android/location/provider/FusedLocationHardware.java
index abea9fb..bc5a8a1 100644
--- a/location/lib/java/com/android/location/provider/FusedLocationHardware.java
+++ b/location/lib/java/com/android/location/provider/FusedLocationHardware.java
@@ -9,7 +9,7 @@
  *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIOS OF ANY KIND, either express or implied.
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
@@ -21,20 +21,26 @@ import android.hardware.location.IFusedLocationHardwareSink;
 
 import android.location.Location;
 
+import android.os.Handler;
+import android.os.Looper;
+import android.os.Message;
 import android.os.RemoteException;
 import android.util.Log;
 
-import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Map;
 
 /**
  * Class that exposes IFusedLocationHardware functionality to unbundled services.
- * Namely this is used by GmsCore Fused Location Provider.
  */
 public final class FusedLocationHardware {
     private final String TAG = "FusedLocationHardware";
 
     private IFusedLocationHardware mLocationHardware;
-    ArrayList<FusedLocationHardwareSink> mSinkList = new ArrayList<FusedLocationHardwareSink>();
+
+    // the list uses a copy-on-write pattern to update its contents
+    HashMap<FusedLocationHardwareSink, DispatcherHandler> mSinkList =
+            new HashMap<FusedLocationHardwareSink, DispatcherHandler>();
 
     private IFusedLocationHardwareSink mInternalSink = new IFusedLocationHardwareSink.Stub() {
         @Override
@@ -48,6 +54,9 @@ public final class FusedLocationHardware {
         }
     };
 
+    /**
+     * @hide
+     */
     public FusedLocationHardware(IFusedLocationHardware locationHardware) {
         mLocationHardware = locationHardware;
     }
@@ -55,19 +64,24 @@ public final class FusedLocationHardware {
     /*
      * Methods to provide a Facade for IFusedLocationHardware
      */
-    public void registerSink(FusedLocationHardwareSink sink) {
-        if(sink == null) {
-            return;
+    public void registerSink(FusedLocationHardwareSink sink, Looper looper) {
+        if(sink == null || looper == null) {
+            throw new IllegalArgumentException("Parameter sink and looper cannot be null.");
         }
 
-        boolean registerSink = false;
+        boolean registerSink;
         synchronized (mSinkList) {
             // register only on first insertion
             registerSink = mSinkList.size() == 0;
             // guarantee uniqueness
-            if(!mSinkList.contains(sink)) {
-                mSinkList.add(sink);
+            if(mSinkList.containsKey(sink)) {
+                return;
             }
+
+            HashMap<FusedLocationHardwareSink, DispatcherHandler> newSinkList =
+                    new HashMap<FusedLocationHardwareSink, DispatcherHandler>(mSinkList);
+            newSinkList.put(sink, new DispatcherHandler(looper));
+            mSinkList = newSinkList;
         }
 
         if(registerSink) {
@@ -81,14 +95,23 @@ public final class FusedLocationHardware {
 
     public void unregisterSink(FusedLocationHardwareSink sink) {
         if(sink == null) {
-            return;
+            throw new IllegalArgumentException("Parameter sink cannot be null.");
         }
 
-        boolean unregisterSink = false;
+        boolean unregisterSink;
         synchronized(mSinkList) {
-            mSinkList.remove(sink);
-            // unregister after the last sink
-            unregisterSink = mSinkList.size() == 0;
+            if(!mSinkList.containsKey(sink)) {
+                //done
+                return;
+            }
+
+            HashMap<FusedLocationHardwareSink, DispatcherHandler> newSinkList =
+                    new HashMap<FusedLocationHardwareSink, DispatcherHandler>(mSinkList);
+            newSinkList.remove(sink);
+            //unregister after the last sink
+            unregisterSink = newSinkList.size() == 0;
+
+            mSinkList = newSinkList;
         }
 
         if(unregisterSink) {
@@ -176,27 +199,82 @@ public final class FusedLocationHardware {
     }
 
     /*
-     * Helper methods
+     * Helper methods and classes
      */
+    private class DispatcherHandler extends Handler {
+        public static final int DISPATCH_LOCATION = 1;
+        public static final int DISPATCH_DIAGNOSTIC_DATA = 2;
+
+        public DispatcherHandler(Looper looper) {
+            super(looper, null /*callback*/ , true /*async*/);
+        }
+
+        @Override
+        public void handleMessage(Message message) {
+            MessageCommand command = (MessageCommand) message.obj;
+            switch(message.what) {
+                case DISPATCH_LOCATION:
+                    command.dispatchLocation();
+                    break;
+                case DISPATCH_DIAGNOSTIC_DATA:
+                    command.dispatchDiagnosticData();
+                default:
+                    Log.e(TAG, "Invalid dispatch message");
+                    break;
+            }
+        }
+    }
+
+    private class MessageCommand {
+        private final FusedLocationHardwareSink mSink;
+        private final Location[] mLocations;
+        private final String mData;
+
+        public MessageCommand(
+                FusedLocationHardwareSink sink,
+                Location[] locations,
+                String data) {
+            mSink = sink;
+            mLocations = locations;
+            mData = data;
+        }
+
+        public void dispatchLocation() {
+            mSink.onLocationAvailable(mLocations);
+        }
+
+        public void dispatchDiagnosticData() {
+            mSink.onDiagnosticDataAvailable(mData);
+        }
+    }
+
     private void dispatchLocations(Location[] locations) {
-        ArrayList<FusedLocationHardwareSink> sinks = null;
+        HashMap<FusedLocationHardwareSink, DispatcherHandler> sinks;
         synchronized (mSinkList) {
-            sinks = new ArrayList<FusedLocationHardwareSink>(mSinkList);
+            sinks = mSinkList;
         }
 
-        for(FusedLocationHardwareSink sink : sinks) {
-            sink.onLocationAvailable(locations);
+        for(Map.Entry<FusedLocationHardwareSink, DispatcherHandler> entry : sinks.entrySet()) {
+            Message message = Message.obtain(
+                    entry.getValue(),
+                    DispatcherHandler.DISPATCH_LOCATION,
+                    new MessageCommand(entry.getKey(), locations, null /*data*/));
+            message.sendToTarget();
         }
     }
 
     private void dispatchDiagnosticData(String data) {
-        ArrayList<FusedLocationHardwareSink> sinks = null;
+        HashMap<FusedLocationHardwareSink, DispatcherHandler> sinks;
         synchronized(mSinkList) {
-            sinks = new ArrayList<FusedLocationHardwareSink>(mSinkList);
+            sinks = mSinkList;
         }
 
-        for(FusedLocationHardwareSink sink : sinks) {
-            sink.onDiagnosticDataAvailable(data);
+        for(Map.Entry<FusedLocationHardwareSink, DispatcherHandler> entry : sinks.entrySet()) {
+            Message message = Message.obtain(
+                    entry.getValue(),
+                    DispatcherHandler.DISPATCH_DIAGNOSTIC_DATA,
+                    new MessageCommand(entry.getKey(), null /*locations*/, data));
+            message.sendToTarget();
         }
     }
 }
diff --git a/location/lib/java/com/android/location/provider/FusedLocationHardwareSink.java b/location/lib/java/com/android/location/provider/FusedLocationHardwareSink.java
index 118b663..2c39fa8 100644
--- a/location/lib/java/com/android/location/provider/FusedLocationHardwareSink.java
+++ b/location/lib/java/com/android/location/provider/FusedLocationHardwareSink.java
@@ -20,7 +20,6 @@ import android.location.Location;
 
 /**
  * Base class for sinks to interact with FusedLocationHardware.
- * This is mainly used by GmsCore Fused Provider.
  */
 public abstract class FusedLocationHardwareSink {
     /*
diff --git a/location/lib/java/com/android/location/provider/FusedProvider.java b/location/lib/java/com/android/location/provider/FusedProvider.java
index bc9feef..c966ade 100644
--- a/location/lib/java/com/android/location/provider/FusedProvider.java
+++ b/location/lib/java/com/android/location/provider/FusedProvider.java
@@ -28,8 +28,6 @@ import android.os.IBinder;
  *
  * <p>IMPORTANT: This class is effectively a public API for unbundled applications, and must remain
  * API stable. See README.txt in the root of this package for more information.
- *
- * @hide
  */
 public abstract class FusedProvider {
     private IFusedProvider.Stub mProvider = new IFusedProvider.Stub() {
diff --git a/services/java/com/android/server/LocationManagerService.java b/services/java/com/android/server/LocationManagerService.java
index 49746ff..a32699a 100644
--- a/services/java/com/android/server/LocationManagerService.java
+++ b/services/java/com/android/server/LocationManagerService.java
@@ -437,7 +437,10 @@ public class LocationManagerService extends ILocationManager.Stub {
         FusedProxy fusedProxy = FusedProxy.createAndBind(
                 mContext,
                 mLocationHandler,
-                flpHardwareProvider.getLocationHardware());
+                flpHardwareProvider.getLocationHardware(),
+                com.android.internal.R.bool.config_enableFusedLocationOverlay,
+                com.android.internal.R.string.config_fusedLocationProviderPackageName,
+                com.android.internal.R.array.config_locationProviderPackageNames);
 
         if(fusedProxy == null) {
             Slog.e(TAG, "No FusedProvider found.");
diff --git a/services/java/com/android/server/location/FlpHardwareProvider.java b/services/java/com/android/server/location/FlpHardwareProvider.java
index 469f7ce..226c18c 100644
--- a/services/java/com/android/server/location/FlpHardwareProvider.java
+++ b/services/java/com/android/server/location/FlpHardwareProvider.java
@@ -28,11 +28,10 @@ import android.location.LocationManager;
 
 import android.content.Context;
 import android.os.Bundle;
-import android.os.Handler;
+import android.os.Looper;
 import android.os.RemoteException;
 import android.os.SystemClock;
 import android.util.Log;
-import android.util.Slog;
 
 /**
  * This class is an interop layer for JVM types and the JNI code that interacts
@@ -48,6 +47,7 @@ public class FlpHardwareProvider {
 
     private final static String TAG = "FlpHardwareProvider";
     private final Context mContext;
+    private final Object mLocationSinkLock = new Object();
 
     public static FlpHardwareProvider getInstance(Context context) {
         if (sSingletonInstance == null) {
@@ -61,7 +61,6 @@ public class FlpHardwareProvider {
         mContext = context;
 
         // register for listening for passive provider data
-        Handler handler = new Handler();
         LocationManager manager = (LocationManager) mContext.getSystemService(
                 Context.LOCATION_SERVICE);
         manager.requestLocationUpdates(
@@ -69,7 +68,7 @@ public class FlpHardwareProvider {
                 0 /* minTime */,
                 0 /* minDistance */,
                 new NetworkLocationListener(),
-                handler.getLooper());
+                Looper.myLooper());
     }
 
     public static boolean isSupported() {
@@ -87,9 +86,13 @@ public class FlpHardwareProvider {
             location.setElapsedRealtimeNanos(SystemClock.elapsedRealtimeNanos());
         }
 
+        IFusedLocationHardwareSink sink;
+        synchronized (mLocationSinkLock) {
+            sink = mLocationSink;
+        }
         try {
-            if (mLocationSink != null) {
-                mLocationSink.onLocationAvailable(locations);
+            if (sink != null) {
+                sink.onLocationAvailable(locations);
             }
         } catch (RemoteException e) {
             Log.e(TAG, "RemoteException calling onLocationAvailable");
@@ -98,9 +101,13 @@ public class FlpHardwareProvider {
 
     // FlpDiagnosticCallbacks members
     private void onDataReport(String data) {
+        IFusedLocationHardwareSink sink;
+        synchronized (mLocationSinkLock) {
+            sink = mLocationSink;
+        }
         try {
             if (mLocationSink != null) {
-                mLocationSink.onDiagnosticDataAvailable(data);
+                sink.onDiagnosticDataAvailable(data);
             }
         } catch (RemoteException e) {
             Log.e(TAG, "RemoteException calling onDiagnosticDataAvailable");
@@ -199,19 +206,24 @@ public class FlpHardwareProvider {
     private final IFusedLocationHardware mLocationHardware = new IFusedLocationHardware.Stub() {
         @Override
         public void registerSink(IFusedLocationHardwareSink eventSink) {
-            // only one sink is allowed at the moment
-            if (mLocationSink != null) {
-                throw new RuntimeException("IFusedLocationHardware does not support multiple sinks");
+            synchronized (mLocationSinkLock) {
+                // only one sink is allowed at the moment
+                if (mLocationSink != null) {
+                    throw new RuntimeException(
+                            "IFusedLocationHardware does not support multiple sinks");
+                }
+
+                mLocationSink = eventSink;
             }
-
-            mLocationSink = eventSink;
         }
 
         @Override
         public void unregisterSink(IFusedLocationHardwareSink eventSink) {
-            // don't throw if the sink is not registered, simply make it a no-op
-            if (mLocationSink == eventSink) {
-                mLocationSink = null;
+            synchronized (mLocationSinkLock) {
+                // don't throw if the sink is not registered, simply make it a no-op
+                if (mLocationSink == eventSink) {
+                    mLocationSink = null;
+                }
             }
         }
 
diff --git a/services/java/com/android/server/location/FusedProxy.java b/services/java/com/android/server/location/FusedProxy.java
index 8278b96..f7fac77 100644
--- a/services/java/com/android/server/location/FusedProxy.java
+++ b/services/java/com/android/server/location/FusedProxy.java
@@ -33,10 +33,8 @@ import android.util.Log;
  */
 public final class FusedProxy {
     private final String TAG = "FusedProxy";
-
-    private ServiceWatcher mServiceWatcher;
-
-    private FusedLocationHardwareSecure mLocationHardware;
+    private final ServiceWatcher mServiceWatcher;
+    private final FusedLocationHardwareSecure mLocationHardware;
 
     /**
      * Constructor of the class.
@@ -46,7 +44,13 @@ public final class FusedProxy {
      * @param handler           The handler needed for construction.
      * @param locationHardware  The instance of the Fused location hardware assigned to the proxy.
      */
-    private FusedProxy(Context context, Handler handler, IFusedLocationHardware locationHardware) {
+    private FusedProxy(
+            Context context,
+            Handler handler,
+            IFusedLocationHardware locationHardware,
+            int overlaySwitchResId,
+            int defaultServicePackageNameResId,
+            int initialPackageNameResId) {
         mLocationHardware = new FusedLocationHardwareSecure(
                 locationHardware,
                 context,
@@ -63,9 +67,9 @@ public final class FusedProxy {
                 context,
                 TAG,
                 "com.android.location.service.FusedProvider",
-                com.android.internal.R.bool.config_enableFusedLocationOverlay,
-                com.android.internal.R.string.config_fusedLocationProviderPackageName,
-                com.android.internal.R.array.config_locationProviderPackageNames,
+                overlaySwitchResId,
+                defaultServicePackageNameResId,
+                initialPackageNameResId,
                 newServiceWork,
                 handler);
     }
@@ -82,9 +86,17 @@ public final class FusedProxy {
     public static FusedProxy createAndBind(
             Context context,
             Handler handler,
-            IFusedLocationHardware locationHardware
-            ) {
-        FusedProxy fusedProxy = new FusedProxy(context, handler, locationHardware);
+            IFusedLocationHardware locationHardware,
+            int overlaySwitchResId,
+            int defaultServicePackageNameResId,
+            int initialPackageNameResId) {
+        FusedProxy fusedProxy = new FusedProxy(
+                context,
+                handler,
+                locationHardware,
+                overlaySwitchResId,
+                defaultServicePackageNameResId,
+                initialPackageNameResId);
 
         // try to bind the Fused provider
         if (!fusedProxy.mServiceWatcher.start()) {