[Cronet] Add callback wrapper classes to enforce API version checking.

components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java

Index: components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
index fa2f646b0d61cdf0e792516b3623817a4c29d1df..f8719940235758347dfecd997823ec90872e2046 100644
--- a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
+++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
@@ -33,6 +33,7 @@ import java.net.URLConnection;
 import java.net.URLStreamHandlerFactory;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
@@ -119,12 +120,26 @@ public class CronetUrlRequestContext extends CronetEngineBase {
             new ObserverList<NetworkQualityRttListener>();
 
     @GuardedBy("mNetworkQualityLock")
+    private final Map<NetworkQualityRttListener,
+            VersionSafeCallbacks.NetworkQualityRttListenerWrapper> mRttListenerMap =
+            new HashMap<NetworkQualityRttListener,
+                    VersionSafeCallbacks.NetworkQualityRttListenerWrapper>();
+
+    @GuardedBy("mNetworkQualityLock")
     private final ObserverList<NetworkQualityThroughputListener> mThroughputListenerList =
             new ObserverList<NetworkQualityThroughputListener>();
 
+    @GuardedBy("mNetworkQualityLock")
+    private final Map<NetworkQualityThroughputListener,
+            VersionSafeCallbacks.NetworkQualityThroughputListenerWrapper> mThroughputListenerMap =
+            new HashMap<NetworkQualityThroughputListener,
+                    VersionSafeCallbacks.NetworkQualityThroughputListenerWrapper>();
+
     @GuardedBy("mFinishedListenerLock")
-    private final List<RequestFinishedInfo.Listener> mFinishedListenerList =
-            new ArrayList<RequestFinishedInfo.Listener>();
+    private final Map<RequestFinishedInfo.Listener,

[kapishnikov, 2016/11/18 20:11:18]

Can we instead of using the map, implement equals() & hashCode() method on
VersionSafeCallbacks.RequestFinishedInfoListener? Objects that are referencing
the same RequestFinishedInfo.Listener are equal. This would allow to continue
using lists instead of maps.

[pauljensen, 2016/11/19 01:12:18]

I thought about that but it had a couple downsides:
1. The delegation of equals() and hashcode() are somewhat counterintuitive and
kinda abnormal behavior that seems like it could trip someone up in the future
(e.g. someone removing the delegating equals() and hashcode() methods, and us
not having a test to check that we actually remove items from the list).
2. If you want to delete an element you have to create another wrapping class
just for the sake of searching for the pre-existing member and then throw it
away which seemed fairly inefficient.

I don't feel strongly though, if you prefer this behavior let me know and I'll
implement.

[kapishnikov, 2016/11/21 17:35:22]

I personally like the idea of implementing equals(). But I agree that some
functionality may not be intuitive, e.g. to delete a listener, it will be
necessary to create a new object. What about creating a new private nested class
that would wrap a list/map and do the necessary logic? At least, the top class
will be simplified.

One advantage of a list versus a map is that it uses less memory but I don't
think it should be a contributing factor here.

[pauljensen, 2016/11/22 12:38:02]

I delegated the equals() and hashCode() to the wrapped listener, and documented
it as such.  I think the is the simplest and cleanest implementation.  Let me
know what you think.

+            VersionSafeCallbacks.RequestFinishedInfoListener> mFinishedListenerMap =
+            new HashMap<RequestFinishedInfo.Listener,
+                    VersionSafeCallbacks.RequestFinishedInfoListener>();
 
     /**
      * Synchronize access to mCertVerifierData.
@@ -413,6 +428,8 @@ public class CronetUrlRequestContext extends CronetEngineBase {
                 }
             }
             mRttListenerList.addObserver(listener);
+            mRttListenerMap.put(
+                    listener, new VersionSafeCallbacks.NetworkQualityRttListenerWrapper(listener));
         }
     }
 
@@ -422,11 +439,13 @@ public class CronetUrlRequestContext extends CronetEngineBase {
             throw new IllegalStateException("Network quality estimator must be enabled");
         }
         synchronized (mNetworkQualityLock) {
-            mRttListenerList.removeObserver(listener);
-            if (mRttListenerList.isEmpty()) {
-                synchronized (mLock) {
-                    checkHaveAdapter();
-                    nativeProvideRTTObservations(mUrlRequestContextAdapter, false);
+            if (mRttListenerList.removeObserver(listener)) {
+                mRttListenerMap.remove(listener);
+                if (mRttListenerList.isEmpty()) {
+                    synchronized (mLock) {
+                        checkHaveAdapter();
+                        nativeProvideRTTObservations(mUrlRequestContextAdapter, false);
+                    }
                 }
             }
         }
@@ -445,6 +464,8 @@ public class CronetUrlRequestContext extends CronetEngineBase {
                 }
             }
             mThroughputListenerList.addObserver(listener);
+            mThroughputListenerMap.put(listener,
+                    new VersionSafeCallbacks.NetworkQualityThroughputListenerWrapper(listener));
         }
     }
 
@@ -454,11 +475,13 @@ public class CronetUrlRequestContext extends CronetEngineBase {
             throw new IllegalStateException("Network quality estimator must be enabled");
         }
         synchronized (mNetworkQualityLock) {
-            mThroughputListenerList.removeObserver(listener);
-            if (mThroughputListenerList.isEmpty()) {
-                synchronized (mLock) {
-                    checkHaveAdapter();
-                    nativeProvideThroughputObservations(mUrlRequestContextAdapter, false);
+            if (mThroughputListenerList.removeObserver(listener)) {
+                mThroughputListenerMap.remove(listener);
+                if (mThroughputListenerList.isEmpty()) {
+                    synchronized (mLock) {
+                        checkHaveAdapter();
+                        nativeProvideThroughputObservations(mUrlRequestContextAdapter, false);
+                    }
                 }
             }
         }
@@ -467,20 +490,21 @@ public class CronetUrlRequestContext extends CronetEngineBase {
     @Override
     public void addRequestFinishedListener(RequestFinishedInfo.Listener listener) {
         synchronized (mFinishedListenerLock) {
-            mFinishedListenerList.add(listener);
+            mFinishedListenerMap.put(
+                    listener, new VersionSafeCallbacks.RequestFinishedInfoListener(listener));
         }
     }
 
     @Override
     public void removeRequestFinishedListener(RequestFinishedInfo.Listener listener) {
         synchronized (mFinishedListenerLock) {
-            mFinishedListenerList.remove(listener);
+            mFinishedListenerMap.remove(listener);
         }
     }
 
     boolean hasRequestFinishedListener() {
         synchronized (mFinishedListenerLock) {
-            return !mFinishedListenerList.isEmpty();
+            return !mFinishedListenerMap.isEmpty();
         }
     }
 
@@ -613,7 +637,9 @@ public class CronetUrlRequestContext extends CronetEngineBase {
     @CalledByNative
     private void onRttObservation(final int rttMs, final long whenMs, final int source) {
         synchronized (mNetworkQualityLock) {
-            for (final NetworkQualityRttListener listener : mRttListenerList) {
+            for (NetworkQualityRttListener unsafeListener : mRttListenerList) {
+                final VersionSafeCallbacks.NetworkQualityRttListenerWrapper listener =
+                        mRttListenerMap.get(unsafeListener);
                 Runnable task = new Runnable() {
                     @Override
                     public void run() {
@@ -630,7 +656,9 @@ public class CronetUrlRequestContext extends CronetEngineBase {
     private void onThroughputObservation(
             final int throughputKbps, final long whenMs, final int source) {
         synchronized (mNetworkQualityLock) {
-            for (final NetworkQualityThroughputListener listener : mThroughputListenerList) {
+            for (NetworkQualityThroughputListener unsafeListener : mThroughputListenerList) {
+                final VersionSafeCallbacks.NetworkQualityThroughputListenerWrapper listener =
+                        mThroughputListenerMap.get(unsafeListener);
                 Runnable task = new Runnable() {
                     @Override
                     public void run() {
@@ -650,11 +678,12 @@ public class CronetUrlRequestContext extends CronetEngineBase {
     }
 
     void reportFinished(final RequestFinishedInfo requestInfo) {
-        ArrayList<RequestFinishedInfo.Listener> currentListeners;
+        ArrayList<VersionSafeCallbacks.RequestFinishedInfoListener> currentListeners;
         synchronized (mFinishedListenerLock) {
-            currentListeners = new ArrayList<RequestFinishedInfo.Listener>(mFinishedListenerList);
+            currentListeners = new ArrayList<VersionSafeCallbacks.RequestFinishedInfoListener>(
+                    mFinishedListenerMap.values());
         }
-        for (final RequestFinishedInfo.Listener listener : currentListeners) {
+        for (final VersionSafeCallbacks.RequestFinishedInfoListener listener : currentListeners) {
             Runnable task = new Runnable() {
                 @Override
                 public void run() {