Change RequestFinishedListener to provide executor

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

 // Copyright 2014 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.
 
 package org.chromium.net.impl;
 
 import android.content.Context;
 import android.os.Build;
 import android.os.ConditionVariable;
 import android.os.Handler;
 import android.os.Looper;
 import android.os.Process;
 import android.util.Log;
 
 import org.chromium.base.ObserverList;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.NativeClassQualifiedName;
 import org.chromium.base.annotations.UsedByReflection;
 import org.chromium.net.BidirectionalStream;
 import org.chromium.net.CronetEngine;
 import org.chromium.net.NetworkQualityRttListener;
 import org.chromium.net.NetworkQualityThroughputListener;
+import org.chromium.net.RequestFinishedListener;
 import org.chromium.net.UrlRequest;
 import org.chromium.net.urlconnection.CronetHttpURLConnection;
 import org.chromium.net.urlconnection.CronetURLStreamHandlerFactory;
 
 import java.net.Proxy;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.URLStreamHandlerFactory;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import javax.annotation.concurrent.GuardedBy;
 
 /**
@@ skipping 19 matching lines @@
     private Thread mNetworkThread;
 
     private Executor mNetworkQualityExecutor;
     private boolean mNetworkQualityEstimatorEnabled;
 
     /** Locks operations on network quality listeners, because listener
      * addition and removal may occur on a different thread from notification.
      */
     private final Object mNetworkQualityLock = new Object();
 
+    /** Locks operations on the list of RequestFinishedListeners, because operations can happen on

[xunjieli, 2016/07/29 16:35:25]

The previous example is a bad example :( 
The first line needs be to moved one line down.
/**
 * first line...
 * second line.
 */

[mgersh, 2016/07/29 18:21:52]

Done.

+     * any thread.
+     */
+    private final Object mFinishedListenerLock = new Object();
+
     @GuardedBy("mNetworkQualityLock")
     private final ObserverList<NetworkQualityRttListener> mRttListenerList =
             new ObserverList<NetworkQualityRttListener>();
 
     @GuardedBy("mNetworkQualityLock")
     private final ObserverList<NetworkQualityThroughputListener> mThroughputListenerList =
             new ObserverList<NetworkQualityThroughputListener>();
 
-    @GuardedBy("mNetworkQualityLock")
-    private final ObserverList<RequestFinishedListener> mFinishedListenerList =
-            new ObserverList<RequestFinishedListener>();
+    @GuardedBy("mFinishedListenerLock")
+    private final List<RequestFinishedListener> mFinishedListenerList =
+            new ArrayList<RequestFinishedListener>();
 
     /**
      * Synchronize access to mCertVerifierData.
      */
     private ConditionVariable mWaitGetCertVerifierDataComplete = new ConditionVariable();
 
     /** Holds CertVerifier data. */
     private String mCertVerifierData;
 
     @UsedByReflection("CronetEngine.java")
@@ skipping 53 matching lines @@
         }
         return urlRequestContextConfig;
     }
 
     @Override
     public UrlRequest createRequest(String url, UrlRequest.Callback callback, Executor executor,
             int priority, Collection<Object> requestAnnotations, boolean disableCache,
             boolean disableConnectionMigration) {
         synchronized (mLock) {
             checkHaveAdapter();
-            boolean metricsCollectionEnabled = mNetworkQualityEstimatorEnabled;
-            if (metricsCollectionEnabled) { // Collect metrics only if someone is listening.
-                synchronized (mNetworkQualityLock) {
-                    metricsCollectionEnabled = !mFinishedListenerList.isEmpty();
-                }
+            boolean metricsCollectionEnabled = false;
+            synchronized (mFinishedListenerLock) {
+                metricsCollectionEnabled = !mFinishedListenerList.isEmpty();
             }
             return new CronetUrlRequest(this, url, priority, callback, executor, requestAnnotations,
                     metricsCollectionEnabled, disableCache, disableConnectionMigration);
         }
     }
 
     @Override
     public BidirectionalStream createBidirectionalStream(String url,
             BidirectionalStream.Callback callback, Executor executor, String httpMethod,
             List<Map.Entry<String, String>> requestHeaders,
@@ skipping 201 matching lines @@
         }
     }
 
     /**
      * TODO(tbansal):  http://crbug.com/618034 Remove this API once all
      * embedders have switched to using a request finished listener that
      * provides its own executor.
      */
     @Override
     public void addRequestFinishedListener(RequestFinishedListener listener) {
-        if (!mNetworkQualityEstimatorEnabled) {
-            throw new IllegalStateException("Network quality estimator must be enabled");
-        }
-        // RequestFinishedListener does not provide its own executor.
-        if (mNetworkQualityExecutor == null) {
-            throw new IllegalStateException("Executor must not be null");
-        }
-        synchronized (mNetworkQualityLock) {
-            mFinishedListenerList.addObserver(listener);
+        synchronized (mFinishedListenerLock) {
+            mFinishedListenerList.add(listener);
         }
     }
 
     /**
      * TODO(tbansal):  http://crbug.com/618034 Remove this API.
      */
     @Override
     public void removeRequestFinishedListener(RequestFinishedListener listener) {
-        if (!mNetworkQualityEstimatorEnabled) {
-            throw new IllegalStateException("Network quality estimator must be enabled");
-        }
-        synchronized (mNetworkQualityLock) {
-            mFinishedListenerList.removeObserver(listener);
+        synchronized (mFinishedListenerLock) {
+            mFinishedListenerList.remove(listener);
         }
     }
 
     @Override
     public URLConnection openConnection(URL url) {
         return openConnection(url, Proxy.NO_PROXY);
     }
 
     @Override
     public URLConnection openConnection(URL url, Proxy proxy) {
@@ skipping 107 matching lines @@
     }
 
     @SuppressWarnings("unused")
     @CalledByNative
     private void onGetCertVerifierData(String certVerifierData) {
         mCertVerifierData = certVerifierData;
         mWaitGetCertVerifierDataComplete.open();
     }
 
     void reportFinished(final CronetUrlRequest request) {
-        if (!mNetworkQualityEstimatorEnabled) {
-            return;
+        final UrlRequestInfo requestInfo = request.getRequestInfo();
+        ArrayList<RequestFinishedListener> currentListeners;
+        synchronized (mFinishedListenerLock) {
+            currentListeners = new ArrayList<RequestFinishedListener>(mFinishedListenerList);
         }
-        // If no request finished listener has been added, then mNetworkQualityExecutor may be
-        // null. Exit early to avoid posting to a null executor.
-        synchronized (mNetworkQualityLock) {
-            if (mFinishedListenerList.isEmpty()) {
-                return;
-            }
+        for (final RequestFinishedListener listener : currentListeners) {
+            Runnable task = new Runnable() {
+                @Override
+                public void run() {
+                    listener.onRequestFinished(requestInfo);
+                }
+            };
+            postObservationTaskToExecutor(listener.getExecutor(), task);
         }
-        Runnable task = new Runnable() {
-            @Override
-            public void run() {
-                synchronized (mNetworkQualityLock) {
-                    UrlRequestInfo requestInfo = request.getRequestInfo();
-                    for (RequestFinishedListener listener : mFinishedListenerList) {
-                        listener.onRequestFinished(requestInfo);
-                    }
-                }
-            }
-        };
-        // Use {@link mNetworkQualityExecutor} since
-        // RequestFInishedListeners do not provide an executor.
-        postObservationTaskToExecutor(mNetworkQualityExecutor, task);
     }
 
     private static void postObservationTaskToExecutor(Executor executor, Runnable task) {
         try {
             executor.execute(task);
         } catch (RejectedExecutionException failException) {
             Log.e(CronetUrlRequestContext.LOG_TAG, "Exception posting task to executor",
                     failException);
         }
     }
@@ skipping 41 matching lines @@
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeEnableNetworkQualityEstimator(long nativePtr);
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeProvideRTTObservations(long nativePtr, boolean should);
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeProvideThroughputObservations(long nativePtr, boolean should);
 }