Update ResponseInfo to UrlResponseInfo with API review comments.

components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.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;
 
 import android.util.Log;
-import android.util.Pair;
 
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNIAdditionalImport;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.NativeClassQualifiedName;
 
 import java.nio.ByteBuffer;
+import java.util.AbstractMap;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.TreeMap;
 import java.util.concurrent.Executor;
 import java.util.concurrent.RejectedExecutionException;
 
 /**
  * UrlRequest using Chromium HTTP stack implementation. Could be accessed from
  * any thread on Executor. Cancel can be called from any thread.
  * All @CallByNative methods are called on native network thread
  * and post tasks with listener calls onto Executor. Upon return from listener
  * callback native request adapter is called on executive thread and posts
  * native tasks to native network thread. Because Cancel could be called from
@@ skipping 34 matching lines @@
     private final List<String> mUrlChain = new ArrayList<String>();
 
     private final UrlRequestListener mListener;
     private final String mInitialUrl;
     private final int mPriority;
     private String mInitialMethod;
     private final HeadersList mRequestHeaders = new HeadersList();
 
     private CronetUploadDataStream mUploadDataStream;
 
-    private NativeResponseInfo mResponseInfo;
+    private UrlResponseInfo mResponseInfo;
 
     /*
      * Listener callback is repeatedly called when each read is completed, so it
      * is cached as a member variable.
      */
     private OnReadCompletedRunnable mOnReadCompletedTask;
 
     private Runnable mOnDestroyedCallbackForTests;
 
-    private static final class HeadersList extends ArrayList<Pair<String, String>> {}
+    private static final class HeadersList extends ArrayList<Map.Entry<String, String>> {}
 
     private final class OnReadCompletedRunnable implements Runnable {
         ByteBuffer mByteBuffer;
 
         @Override
         public void run() {
             if (isDone()) {
                 return;
             }
             try {
                 synchronized (mUrlRequestAdapterLock) {
                     if (mUrlRequestAdapter == 0) {
                         return;
                     }
                     mWaitingOnRead = true;
                 }
                 // Null out mByteBuffer, out of paranoia. Has to be done before
                 // mListener call, to avoid any race when there are multiple
                 // executor threads.
                 ByteBuffer buffer = mByteBuffer;
                 mByteBuffer = null;
                 mListener.onReadCompleted(CronetUrlRequest.this,
                         mResponseInfo, buffer);
             } catch (Exception e) {
                 onListenerException(e);
             }
         }
     }
 
-    private static final class NativeResponseInfo implements ResponseInfo {
-        private final String[] mResponseInfoUrlChain;
-        private final int mHttpStatusCode;
-        private final String mHttpStatusText;
-        private final boolean mWasCached;
-        private final String mNegotiatedProtocol;
-        private final String mProxyServer;
-        private final HeadersList mAllHeaders = new HeadersList();
-        private Map<String, List<String>> mResponseHeaders;
-        private List<Pair<String, String>> mUnmodifiableAllHeaders;
+    private static final class ExtendedUrlResponseInfo implements ExtendedResponseInfo {
+        private final UrlResponseInfo mUrlResponseInfo;
 
-        NativeResponseInfo(String[] urlChain, int httpStatusCode,
-                String httpStatusText, boolean wasCached,
-                String negotiatedProtocol, String proxyServer) {
-            mResponseInfoUrlChain = urlChain;
-            mHttpStatusCode = httpStatusCode;
-            mHttpStatusText = httpStatusText;
-            mWasCached = wasCached;
-            mNegotiatedProtocol = negotiatedProtocol;
-            mProxyServer = proxyServer;
-        }
-
-        @Override
-        public String getUrl() {
-            return mResponseInfoUrlChain[mResponseInfoUrlChain.length - 1];
-        }
-
-        @Override
-        public String[] getUrlChain() {
-            return mResponseInfoUrlChain;
-        }
-
-        @Override
-        public int getHttpStatusCode() {
-            return mHttpStatusCode;
-        }
-
-        @Override
-        public String getHttpStatusText() {
-            return mHttpStatusText;
-        }
-
-        @Override
-        public List<Pair<String, String>> getAllHeadersAsList() {
-            if (mUnmodifiableAllHeaders == null) {
-                mUnmodifiableAllHeaders =
-                        Collections.unmodifiableList(mAllHeaders);
-            }
-            return mUnmodifiableAllHeaders;
-        }
-
-        @Override
-        public Map<String, List<String>> getAllHeaders() {
-            if (mResponseHeaders != null) {
-                return mResponseHeaders;
-            }
-            Map<String, List<String>> map = new TreeMap<String, List<String>>(
-                    String.CASE_INSENSITIVE_ORDER);
-            for (Pair<String, String> entry : mAllHeaders) {
-                List<String> values = new ArrayList<String>();
-                if (map.containsKey(entry.first)) {
-                    values.addAll(map.get(entry.first));
-                }
-                values.add(entry.second);
-                map.put(entry.first, Collections.unmodifiableList(values));
-            }
-            mResponseHeaders = Collections.unmodifiableMap(map);
-            return mResponseHeaders;
-        }
-
-        @Override
-        public boolean wasCached() {
-            return mWasCached;
-        }
-
-        @Override
-        public String getNegotiatedProtocol() {
-            return mNegotiatedProtocol;
-        }
-
-        @Override
-        public String getProxyServer() {
-            return mProxyServer;
-        }
-    };
-
-    private static final class NativeExtendedResponseInfo implements ExtendedResponseInfo {
-        private final ResponseInfo mResponseInfo;
-        private final long mTotalReceivedBytes;
-
-        NativeExtendedResponseInfo(ResponseInfo responseInfo,
-                                   long totalReceivedBytes) {
-            mResponseInfo = responseInfo;
-            mTotalReceivedBytes = totalReceivedBytes;
+        ExtendedUrlResponseInfo(UrlResponseInfo urlResponseInfo) {
+            mUrlResponseInfo = urlResponseInfo;
         }
 
         @Override
         public ResponseInfo getResponseInfo() {
-            return mResponseInfo;
+            return mUrlResponseInfo;
         }
 
         @Override
         public long getTotalReceivedBytes() {
-            return mTotalReceivedBytes;
+            return mUrlResponseInfo.getReceivedBytesCount();
         }
     };
 
     CronetUrlRequest(CronetUrlRequestContext requestContext,
             long urlRequestContextAdapter,
             String url,
             int priority,
             UrlRequestListener listener,
             Executor executor) {
         if (url == null) {
@@ skipping 25 matching lines @@
 
     @Override
     public void addHeader(String header, String value) {
         checkNotStarted();
         if (header == null) {
             throw new NullPointerException("Invalid header name.");
         }
         if (value == null) {
             throw new NullPointerException("Invalid header value.");
         }
-        mRequestHeaders.add(Pair.create(header, value));
+        mRequestHeaders.add(new AbstractMap.SimpleImmutableEntry<String, String>(header, value));
     }
 
     @Override
     public void setUploadDataProvider(UploadDataProvider uploadDataProvider, Executor executor) {
         if (uploadDataProvider == null) {
             throw new NullPointerException("Invalid UploadDataProvider.");
         }
         if (mInitialMethod == null) {
             mInitialMethod = "POST";
         }
         mUploadDataStream = new CronetUploadDataStream(uploadDataProvider, executor);
     }
 
     @Override
     public void start() {
         synchronized (mUrlRequestAdapterLock) {
             checkNotStarted();
 
             try {
                 mUrlRequestAdapter = nativeCreateRequestAdapter(
                         mRequestContext.getUrlRequestContextAdapter(), mInitialUrl, mPriority);
                 mRequestContext.onRequestStarted(this);
                 if (mInitialMethod != null) {
                     if (!nativeSetHttpMethod(mUrlRequestAdapter, mInitialMethod)) {
                         throw new IllegalArgumentException("Invalid http method " + mInitialMethod);
                     }
                 }
 
                 boolean hasContentType = false;
-                for (Pair<String, String> header : mRequestHeaders) {
-                    if (header.first.equalsIgnoreCase("Content-Type")
-                            && !header.second.isEmpty()) {
+                for (Map.Entry<String, String> header : mRequestHeaders) {
+                    if (header.getKey().equalsIgnoreCase("Content-Type")
+                            && !header.getValue().isEmpty()) {
                         hasContentType = true;
                     }
-                    if (!nativeAddRequestHeader(mUrlRequestAdapter, header.first, header.second)) {
+                    if (!nativeAddRequestHeader(
+                                mUrlRequestAdapter, header.getKey(), header.getValue())) {

[xunjieli, 2015/09/25 13:54:32]

The more I looked at this, the more I think this is inefficient. If we have a
lot of headers, there are multiple JNI calls. Maybe we can pass a jobjectArray
and use base::android::AppendJavaStringArrayToStringVector directly in one
single JNI call. 
Similarly, we can get rid of onAppendResponseHeader, and do the reverse.

Paul, what do you think? 

[mef, 2015/09/25 15:58:27]

Do you mean make 2 string arrays for keys and values? That might work.
IIUIC JNI calls up from C++ to Java are more expensive than down from Java to
C++, so getting rid of onAppendResponseHeader sounds like a noble goal. 

[xunjieli, 2015/09/25 16:16:34]

Since key and values are strings, We can use even indices to hold keys and odd
indices values, or we can do two arrays. Either way should work to save the Java
<-> C++ round trips :)

[pauljensen, 2015/09/25 18:30:16]

So here's a great resource:
https://docs.google.com/spreadsheets/d/1ulggKOZ7VQ8IytP1hKr-1wvBu70qG2qxKT4vR...
That's on a Nexus 4, which is probably 2x slower than a more modern equivalent.
JNI is very fast in general.  String conversions are slower but still pretty
fast.
I'm not too worried about the overhead here right now as generally there aren't
too many HTTP headers and generally they're not too big.
Anyhow this CL isn't changing the number or type of JNI here so lets move this
discussion to a separate bug.

[xunjieli, 2015/09/25 18:39:13]

Cool, thanks for the pointer.
Yea, I just thought it worth bringing up while we are here. I mentioned this a
while ago, but it got lost. I will file a bug to track it.

                         destroyRequestAdapter();
                         throw new IllegalArgumentException(
-                                "Invalid header " + header.first + "=" + header.second);
+                                "Invalid header " + header.getKey() + "=" + header.getValue());
                     }
                 }
                 if (mUploadDataStream != null) {
                     if (!hasContentType) {
                         throw new IllegalArgumentException(
                                 "Requests with upload data must have a Content-Type.");
                     }
                     mUploadDataStream.attachToRequest(this, mUrlRequestAdapter);
                 }
             } catch (RuntimeException e) {
@@ skipping 160 matching lines @@
                 return RequestPriority.LOW;
             case REQUEST_PRIORITY_MEDIUM:
                 return RequestPriority.MEDIUM;
             case REQUEST_PRIORITY_HIGHEST:
                 return RequestPriority.HIGHEST;
             default:
                 return RequestPriority.MEDIUM;
         }
     }
 
-    private NativeResponseInfo prepareResponseInfoOnNetworkThread(
-            int httpStatusCode) {
+    private UrlResponseInfo prepareResponseInfoOnNetworkThread(int httpStatusCode) {
         long urlRequestAdapter;
         synchronized (mUrlRequestAdapterLock) {
             if (mUrlRequestAdapter == 0) {
                 return null;
             }
             // This method is running on network thread, so even if
             // mUrlRequestAdapter is set to 0 from another thread the actual
             // deletion of the adapter is posted to network thread, so it is
             // safe to preserve and use urlRequestAdapter outside the lock.
             urlRequestAdapter = mUrlRequestAdapter;
         }
-        NativeResponseInfo responseInfo = new NativeResponseInfo(
-                mUrlChain.toArray(new String[mUrlChain.size()]),
-                httpStatusCode,
-                nativeGetHttpStatusText(urlRequestAdapter),
+        HeadersList headersList = new HeadersList();
+        nativePopulateResponseHeaders(urlRequestAdapter, headersList);
+
+        UrlResponseInfo responseInfo = new UrlResponseInfo(new ArrayList<String>(mUrlChain),
+                httpStatusCode, nativeGetHttpStatusText(urlRequestAdapter), headersList,
                 nativeGetWasCached(urlRequestAdapter),
                 nativeGetNegotiatedProtocol(urlRequestAdapter),
                 nativeGetProxyServer(urlRequestAdapter));
-        nativePopulateResponseHeaders(urlRequestAdapter,
-                                      responseInfo.mAllHeaders);
         return responseInfo;
     }
 
     private void checkNotStarted() {
         synchronized (mUrlRequestAdapterLock) {
             if (mStarted || isDone()) {
                 throw new IllegalStateException("Request is already started.");
             }
         }
     }
@@ skipping 83 matching lines @@
      * callback. If the redirect response has a body, it will be ignored.
      * This will only be called between start and onResponseStarted.
      *
      * @param newLocation Location where request is redirected.
      * @param httpStatusCode from redirect response
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private void onReceivedRedirect(final String newLocation,
             int httpStatusCode) {
-        final NativeResponseInfo responseInfo =
-                prepareResponseInfoOnNetworkThread(httpStatusCode);
+        final UrlResponseInfo responseInfo = prepareResponseInfoOnNetworkThread(httpStatusCode);
         // Have to do this after creating responseInfo.
         mUrlChain.add(newLocation);
 
         Runnable task = new Runnable() {
             public void run() {
                 synchronized (mUrlRequestAdapterLock) {
                     if (isDone()) {
                         return;
                     }
                     mWaitingOnRedirect = true;
@@ skipping 73 matching lines @@
         postTaskToExecutor(mOnReadCompletedTask);
     }
 
     /**
      * Called when request is completed successfully, no callbacks will be
      * called afterwards.
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private void onSucceeded(long totalReceivedBytes) {
-        final NativeExtendedResponseInfo extendedResponseInfo =
-                new NativeExtendedResponseInfo(mResponseInfo,
-                        totalReceivedBytes);
+        mResponseInfo.updateReceivedBytesCount(totalReceivedBytes);
+        final ExtendedUrlResponseInfo extendedResponseInfo =
+                new ExtendedUrlResponseInfo(mResponseInfo);
         Runnable task = new Runnable() {
             public void run() {
                 synchronized (mUrlRequestAdapterLock) {
                     if (isDone()) {
                         return;
                     }
                     // Destroy adapter first, so request context could be shut
                     // down from the listener.
                     destroyRequestAdapter();
                 }
@@ skipping 24 matching lines @@
         failWithException(requestError);
     }
 
     /**
      * Appends header |name| with value |value| to |headersList|.
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private void onAppendResponseHeader(HeadersList headersList,
             String name, String value) {
-        headersList.add(Pair.create(name, value));
+        headersList.add(new AbstractMap.SimpleImmutableEntry<String, String>(name, value));
     }
 
     /**
      * Called by the native code when request status is fetched from the
      * native stack.
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private void onStatus(final UrlRequest.StatusListener listener, final int loadState) {
         Runnable task = new Runnable() {
@@ skipping 43 matching lines @@
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native String nativeGetProxyServer(long nativePtr);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native void nativeGetStatus(long nativePtr, UrlRequest.StatusListener listener);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native boolean nativeGetWasCached(long nativePtr);
 }