Update ResponseInfo to UrlResponseInfo with API review comments.

components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java

Index: components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
index 9381f0a93f3078b6079aeef9cb618ec5e0223b4a..ce7f8cececce3c5ada68312755e392dace10fd28 100644
--- a/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
@@ -5,7 +5,6 @@
 package org.chromium.net;
 
 import android.util.Log;
-import android.util.Pair;
 
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
@@ -14,11 +13,10 @@ 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;
 
@@ -73,7 +71,7 @@ final class CronetUrlRequest implements UrlRequest {
 
     private CronetUploadDataStream mUploadDataStream;
 
-    private NativeResponseInfo mResponseInfo;
+    private UrlResponseInfo mResponseInfo;
 
     /*
      * Listener callback is repeatedly called when each read is completed, so it
@@ -83,7 +81,7 @@ final class CronetUrlRequest implements UrlRequest {
 
     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;
@@ -113,110 +111,21 @@ final class CronetUrlRequest implements UrlRequest {
         }
     }
 
-    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;
-
-        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;
-        }
+    private static final class ExtendedUrlResponseInfo implements ExtendedResponseInfo {
+        private final UrlResponseInfo mUrlResponseInfo;
 
-        @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();
         }
     };
 
@@ -262,7 +171,7 @@ final class CronetUrlRequest implements UrlRequest {
         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
@@ -292,15 +201,16 @@ final class CronetUrlRequest implements UrlRequest {
                 }
 
                 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) {
@@ -481,8 +391,7 @@ final class CronetUrlRequest implements UrlRequest {
         }
     }
 
-    private NativeResponseInfo prepareResponseInfoOnNetworkThread(
-            int httpStatusCode) {
+    private UrlResponseInfo prepareResponseInfoOnNetworkThread(int httpStatusCode) {
         long urlRequestAdapter;
         synchronized (mUrlRequestAdapterLock) {
             if (mUrlRequestAdapter == 0) {
@@ -494,15 +403,14 @@ final class CronetUrlRequest implements UrlRequest {
             // 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;
     }
 
@@ -606,8 +514,7 @@ final class CronetUrlRequest implements UrlRequest {
     @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);
 
@@ -701,9 +608,9 @@ final class CronetUrlRequest implements UrlRequest {
     @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) {
@@ -748,7 +655,7 @@ final class CronetUrlRequest implements UrlRequest {
     @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));
     }
 
     /**