Update ResponseInfo to UrlResponseInfo with API review comments.

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

+// Copyright 2015 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 java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.TreeMap;
+
+/**
+ * Contains basic information about a response. Sent to the embedder whenever
+ * headers are received.
+ */
+// TODO(mef): Remove ResponseInfo after caller updates.

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

This class has pretty crazy mutability requirements.  I think it needs a serious
comment (not a javadoc) describing how almost all the members are strictly
immutable, with the exception of mReceivedBytesCount.  Might also be good to
describe when we generate a new one (e.g. response begins, redirect followed).

+public final class UrlResponseInfo implements ResponseInfo {
+    private final List<String> mResponseInfoUrlChain;
+    private final int mHttpStatusCode;
+    private final String mHttpStatusText;
+    private final boolean mWasCached;
+    private final String mNegotiatedProtocol;
+    private final String mProxyServer;
+    private final List<Map.Entry<String, String>> mAllHeadersList;
+    private Map<String, List<String>> mResponseHeaders;
+    private volatile long mReceivedBytesCount;

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

We are only updating this on the network thread. Our embedders will query new
values every time using getReceivedBytesCount. Why does this need to be
volatile? 

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

Um, what if the app holds on to UrlResponseInfo and accesses it while Cronet is
updating value on network thread?

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

I see. that volatile keyword might help then.

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

volatile is insufficient for items larger than sig_atomic_t, which is 32 bits on
a 32-bit processor.  Remember the classic example:  Cronet increments from
0xffffffff to 0x100000000, Cronet writes top word as 0x1, embedder reads top
word as 0x1, embedder reads bottom word as 0xffffffff, Cronet writes bottom word
as 0x0, then embedder thinks value is 0x1ffffffff which is pretty far off. 
AtomicLong should work.

[mef, 2015/09/25 20:08:50]

Arghh! Thanks for catching this. Done.

+
+    UrlResponseInfo(List<String> urlChain, int httpStatusCode, String httpStatusText,
+            List<Map.Entry<String, String>> allHeadersList, boolean wasCached,
+            String negotiatedProtocol, String proxyServer) {
+        mResponseInfoUrlChain = Collections.unmodifiableList(urlChain);
+        mHttpStatusCode = httpStatusCode;
+        mHttpStatusText = httpStatusText;
+        mAllHeadersList = Collections.unmodifiableList(allHeadersList);
+        mWasCached = wasCached;
+        mNegotiatedProtocol = negotiatedProtocol;
+        mProxyServer = proxyServer;
+        mReceivedBytesCount = 0;
+    }
+
+    /**
+     * Returns the URL the response is for. This is the URL after following
+     * redirects, so it may not be the originally requested URL.
+     * @return the URL the response is for.
+     */
+    @Override
+    public String getUrl() {
+        return mResponseInfoUrlChain.get(mResponseInfoUrlChain.size() - 1);
+    }
+
+    /**
+     * Returns the URL chain. The first entry is the origianlly requested URL;
+     * the following entries are redirects followed.
+     * @return the URL chain.
+     */
+    @Override
+    public List<String> getUrlChain() {
+        return mResponseInfoUrlChain;
+    }
+
+    /**
+     * Returns the HTTP status code. When a resource is retrieved from the cache,
+     * whether it was revalidated or not, the original status code is returned.
+     * @return the HTTP status code.
+     */
+    @Override
+    public int getHttpStatusCode() {
+        return mHttpStatusCode;
+    }
+
+    /**
+     * Returns the HTTP status text of the status line. For example, if the
+     * request has a "HTTP/1.1 200 OK" response, this method returns "OK".
+     * @return the HTTP status text of the status line.
+     */
+    @Override
+    public String getHttpStatusText() {
+        return mHttpStatusText;
+    }
+
+    /**
+     * Returns an unmodifiable list of response header field and value pairs.
+     * The headers are in the same order they are received over the wire.
+     * @return an unmodifiable list of response header field and value pairs.
+     */
+    @Override
+    public List<Map.Entry<String, String>> getAllHeadersAsList() {
+        return mAllHeadersList;
+    }
+
+    /**
+     * Returns an unmodifiable map of the response-header fields and values.
+     * Each list of values for a single header field is in the same order they
+     * were received over the wire.
+     * @return an unmodifiable map of the response-header fields and values.
+     */
+    @Override
+    public Map<String, List<String>> getAllHeaders() {

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

It's sad that even though we have Map.Entry, we still do all these extra work to
construct a Map because there can be multiple headers of the same name. I wonder
how exactly Map.Entry is going to help our embedders.

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

Theoretically it will help if they want to headers into multimap<String, String>
instead of Map<String, List<String>>.

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

I see. That makes sense. Thanks!

+        if (mResponseHeaders != null) {
+            return mResponseHeaders;
+        }
+        Map<String, List<String>> map =
+                new TreeMap<String, List<String>>(String.CASE_INSENSITIVE_ORDER);
+        for (Map.Entry<String, String> entry : mAllHeadersList) {
+            List<String> values = new ArrayList<String>();
+            if (map.containsKey(entry.getKey())) {
+                values.addAll(map.get(entry.getKey()));
+            }
+            values.add(entry.getValue());
+            map.put(entry.getKey(), Collections.unmodifiableList(values));
+        }
+        mResponseHeaders = Collections.unmodifiableMap(map);
+        return mResponseHeaders;
+    }
+
+    /**
+     * Returns {@code true} if the response came from the cache, including
+     * requests that were revalidated over the network before being retrieved
+     * from the cache.
+     * @return {@code true} if the response came from the cache, {@code false}
+     *         otherwise.
+     */
+    @Override
+    public boolean wasCached() {
+        return mWasCached;
+    }
+
+    /**
+     * Returns the protocol (e.g. "quic/1+spdy/3") negotiated with the server.
+     * Returns an empty string if no protocol was negotiated, the protocol is
+     * not known, or when using plain HTTP or HTTPS.
+     * @return the protocol negotiated with the server.
+     */
+    // TODO(mef): Figure out what this returns in the cached case, both with
+    // and without a revalidation request.
+    @Override
+    public String getNegotiatedProtocol() {
+        return mNegotiatedProtocol;
+    }
+
+    /**
+     * Returns the proxy server that was used for the request.
+     * @return the proxy server that was used for the request.
+     */
+    @Override
+    public String getProxyServer() {
+        return mProxyServer;
+    }
+
+    /**
+     * Returns the total amount of data received from network after SSL
+     * decoding and proxy handling but before gzip and SDCH decompression.

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

Though this sentence is very accurate and precise, it's also very complicated. 
I think we need to clarify that this is a minimum bytes-over-the-wire count.  I
think we also need to mention that the count relates just to this request.  Can
we instead say something like:

Returns a minimum count of bytes received from the network to process this
request.
This count may ignore certain overheads (e.g. IP and TCP/UDP framing, SSL
handshake and framing, proxy handling).
This count is taken prior to gzip and SDCH decompression.

[mef, 2015/09/25 20:08:50]

Done.

+     * Available on request completion.

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

I think we need a further comment about how this value can magically change. 
This is extremely magical (comes from a thread embedder doesn't even know
exists) and may come as a big surprise to embedders.

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

I don't think "Available on request completion." is sufficient.  Aren't we going
to update on failure or cancellation too?

[mef, 2015/09/25 20:08:50]

In order to reduce magic nature, would it make sense to keep it 0 until
completion?
Actually, come to think about it, would it make sense to create another copy of
ResponseInfo on success/failure/cancel, with actual value in it?
That way we don't need a setter. The drawback is memory churn.

[pauljensen, 2015/10/06 12:55:41]

So I'd actually like to live in a world where it gets updated even more
frequently, like every time we pass it to the Listener, like for every
onReadCompleted() and all the other Listener methods.  It seems like simple
information that we can cheaply update.  That's why I was pushing for keeping
the description here fairly broad and just indicating that it's a minimum value
which gives us the leeway to improve its accuracy as we see fit.

Anyhow, perhaps we should clarify the "Available on request completion." to
something like "This value may change (even for one {@code UrlResponseInfo}
instance) as the request progresses until completion, when {@link
UrlRequestListener#onSuccess} or {@link UrlRequestListener#onFailure} is
called."

I just don't want to paint us into a corner.  If tomorrow an embedder says
they'd find it useful to get this value updated very frequently, I'd like us to
have a cheap and easy path to enabling that.

Anyhow, what I'm getting at is I'm fine with this class having some funny
mutability if it prevents more copying/allocation of instances of this class,
which in Java means more GC.

Than again, this is just my crazy opinion.  I guess I love my data and want
constant updates :)  I don't feel too strongly on this matter and I'd be fine
with just about any solution to this issue.

+     */
+    public long getReceivedBytesCount() {
+        return mReceivedBytesCount;
+    }
+
+    @Override
+    public String toString() {
+        return String.format(Locale.ROOT, "UrlResponseInfo[%s]: urlChain = %s, "
+                        + "httpStatus = %d %s, headers = %s, wasCached = %b, "
+                        + "negotiatedProtocol = %s, proxyServer= %s, receivedBytesCount = %d",
+                getUrl(), getUrlChain().toString(), getHttpStatusCode(), getHttpStatusText(),
+                getAllHeadersAsList().toString(), wasCached(), getNegotiatedProtocol(),
+                getProxyServer(), getReceivedBytesCount());
+    }
+
+    void updateReceivedBytesCount(long currentReceivedBytesCount) {

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

Maybe rename this method as "setReceivedBytesCount" to make it clearer that this
is setting the variable and not incrementing/decrementing.
Need comments to explain why the visibility is package-protected.

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

Sounds good, will do. I also wonder whether it should be updated for every
onReadCompleted callback.

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

I will vote for that. But could also be in a separate CL if the change is big.

[mef, 2015/09/25 20:08:50]

Acknowledged.

[mef, 2015/09/25 20:53:42]

Done.

+        mReceivedBytesCount = currentReceivedBytesCount;
+    }
+}
+;