[Cronet] Create Builders, rename UrlRequestContext to CronetEngine

[Cronet] Create Builders, rename UrlRequestContext to CronetEngine

Non-functional refactoring of Cronet Java API to better match
common Java API patterns.

1. Rename UrlRequestContext to CronetEngine
2. Rename UrlRequestContextConfig to CronetEngine.Builder
3. Break UrlRequest into:
     a. UrlRequest.Builder to allow configuring a request
     b. UrlRequest to control
4. CronetEngines are created with a new function called
   CronetEngine.Builder.build().  All occurrences of
   createContext have been changed to use this.
5. UrlRequests are created with a new function called
   UrlRequest.Builder.build().  All occurrences of
   createRequest have been changed to use this.

All old APIs are still available to ease migration to the
new APIs, though the renames (#1 and #2 above) are required.

Also included is a fix to CronetBufferedOutputStream's
UploadDataProvider.getLength() implementation where it
didn't work after setConnected() was called. This fix is
necessary to keep tests passing with this change.

BUG=531538
Committed: https://crrev.com/6182d654df33116d7c4e4616c7b51d4e8b28d77f
Cr-Commit-Position: refs/heads/master@{#352986}

[pauljensen, 2015-09-24 23:26:12]

pauljensen@chromium.org changed reviewers:
+ mef@chromium.org

[pauljensen, 2015-09-24 23:26:13]

Misha, PTAL.  Feel free to defer the review to someone else if you like.  Thank
you!

[mef, 2015-09-25 21:32:19]

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
2015?

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:124: *
@return the config to facilitate chaining.
Here and elsewhere:

@return the builder to facilitate chaining.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:289: * Get
JSON string representation of the config.
builder

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:351: *
Creates a {@link UrlRequest} given a {@link Builder}, and starts
I think this should link to UrlRequest.Builder.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:363: for
(Pair<String, String> header : builder.mRequestHeaders) {
should we switch from Pair<String, String> to Map.Entry<String, String> for
consistency?

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:18: public
interface UrlRequest {
should this be an abstract class?

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:23: public
static class Builder {
should it also be final?

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:36: final
ArrayList<Pair<String, String>> mRequestHeaders =
I think java people have prefer Map.Entry<String, String>.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:149:
return mConnected ? mBuffer.limit() : mBuffer.position();
Um, can you elaborate on this? Seems a bit magical.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java:50:
* @param builder the {@link CronetEngine} builder to be used.
Maybe {@link CronetEngine.Builder}?

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:23:
* Test CronetUrlRequestContext.
CronetEngine

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:98:
mActivity.mCronetEngine.executeRequest(builder);
I wonder if anybody is using a pattern where they createRequest in one place,
where they have the UrlRequestContext, and start in another, where they don't.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:581:
public void testInitContextAndStartRequest() {
testInitEngineAndStartRequest

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:596:
public void testInitContextStartTwoRequests() throws Exception {
testInitEngineAndStartTwoRequests

Context => Engine here and elsewhere.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:653:
public void testInitDifferentContexts() throws Exception {
Should this be testInitDifferentEngines?

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:831:
final UrlRequest urlRequest = future.get();
why do you need it?

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:117:
CronetUrlRequestContext newContext =
newEngine?

[pauljensen, 2015-09-28 14:18:13]

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/09/25 21:32:19, mef wrote:
> 2015?

Done.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:124: *
@return the config to facilitate chaining.
On 2015/09/25 21:32:19, mef wrote:
> Here and elsewhere:
> 
> @return the builder to facilitate chaining.

Done.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:289: * Get
JSON string representation of the config.
On 2015/09/25 21:32:18, mef wrote:
> builder

Done.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:351: *
Creates a {@link UrlRequest} given a {@link Builder}, and starts
On 2015/09/25 21:32:19, mef wrote:
> I think this should link to UrlRequest.Builder.

It seems to link properly as "Builder"; the link appears as
"UrlRequest.Builder".

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:363: for
(Pair<String, String> header : builder.mRequestHeaders) {
On 2015/09/25 21:32:19, mef wrote:
> should we switch from Pair<String, String> to Map.Entry<String, String> for
> consistency?

CronetUrlRequest uses an ArrayList<Pair<String, String>> so I think my code is
consistent.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:18: public
interface UrlRequest {
On 2015/09/25 21:32:19, mef wrote:
> should this be an abstract class?

Perhaps, but let's leave that for another CL.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:23: public
static class Builder {
On 2015/09/25 21:32:19, mef wrote:
> should it also be final?

Done.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:36: final
ArrayList<Pair<String, String>> mRequestHeaders =
On 2015/09/25 21:32:19, mef wrote:
> I think java people have prefer Map.Entry<String, String>.

From the API review feedback, I think that is only for public APIs so as to
facilitate adding to a Map, and for possible non-Android usage which isn't
really an issue in Cronet.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:149:
return mConnected ? mBuffer.limit() : mBuffer.position();
On 2015/09/25 21:32:19, mef wrote:
> Um, can you elaborate on this? Seems a bit magical.

Done.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java:50:
* @param builder the {@link CronetEngine} builder to be used.
On 2015/09/25 21:32:19, mef wrote:
> Maybe {@link CronetEngine.Builder}?

Done.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:23:
* Test CronetUrlRequestContext.
On 2015/09/25 21:32:19, mef wrote:
> CronetEngine

Done.  I had left this as CronetUrlRequestContext because there were a few tests
that explicitly held pointers to CronetUrlRequestContext so I figured they were
using CronetUrlRequestContext-specific behavior, but they weren't.  I changed
them to CronetEngine pointers and changed this comment.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:98:
mActivity.mCronetEngine.executeRequest(builder);
On 2015/09/25 21:32:19, mef wrote:
> I wonder if anybody is using a pattern where they createRequest in one place,
> where they have the UrlRequestContext, and start in another, where they don't.

Potentially but they can now create the UrlRequest.Builder in one place and
executeRequest() in the other.  Basically we've split the lifetime of a
UrlRequest into the pre-start() phase where it's a UrlRequest.Builder and
UrlRequest afterwards.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:581:
public void testInitContextAndStartRequest() {
On 2015/09/25 21:32:19, mef wrote:
> testInitEngineAndStartRequest

Done.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:596:
public void testInitContextStartTwoRequests() throws Exception {
On 2015/09/25 21:32:19, mef wrote:
> testInitEngineAndStartTwoRequests
> 
> Context => Engine here and elsewhere.

Done.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:653:
public void testInitDifferentContexts() throws Exception {
On 2015/09/25 21:32:19, mef wrote:
> Should this be testInitDifferentEngines?

Done.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:831:
final UrlRequest urlRequest = future.get();
On 2015/09/25 21:32:19, mef wrote:
> why do you need it?

line 838, 840, 856, and 871 read it

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:117:
CronetUrlRequestContext newContext =
On 2015/09/25 21:32:19, mef wrote:
> newEngine?

Done.  I thought because it was a CronetUrlRequestContext that it was using
CronetUrlRequestContext-specific behavior.  It was not, so I changed it to a
CronetEngine and renamed the variable.

[xunjieli, 2015-09-28 14:57:04]

xunjieli@chromium.org changed reviewers:
+ xunjieli@chromium.org

[xunjieli, 2015-09-28 14:57:05]

driving by, just to make sure I understand the big picture.

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/READ...
File components/cronet/README.md (right):

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/READ...
components/cronet/README.md:72: CronetEngine engine =
engineBuilder.build(getContext());
I usually see build method with 0 argument. Should we move the app context
parameter to the builder's constructor or use a setter?

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/READ...
components/cronet/README.md:76: "https://www.example.com", listener, executor);
Why doesn't the UrlRequest.Builder have a build method? It seems unintuitive
that we have a builder but don't build it.

I would expect executeRequest takes in a request or requestBuilder.build():

UrlRequest.Builder requestBuilder = new UrlRequest.Builder();
UrlRequest request = requestBuilder.build();
engine.executeRequest(request);

[mef, 2015-09-28 16:31:38]

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/READ...
File components/cronet/README.md (right):

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/READ...
components/cronet/README.md:72: CronetEngine engine =
engineBuilder.build(getContext());
On 2015/09/28 14:57:05, xunjieli wrote:
> I usually see build method with 0 argument. Should we move the app context
> parameter to the builder's constructor or use a setter?

My concern with having setter is that it is not apparent that it must be set. I
have no problem with passing it to builder's constructor.

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/READ...
components/cronet/README.md:76: "https://www.example.com", listener, executor);
On 2015/09/28 14:57:05, xunjieli wrote:
> Why doesn't the UrlRequest.Builder have a build method? It seems unintuitive
> that we have a builder but don't build it.
> 
> I would expect executeRequest takes in a request or requestBuilder.build():
> 
> UrlRequest.Builder requestBuilder = new UrlRequest.Builder();
> UrlRequest request = requestBuilder.build();
> engine.executeRequest(request);
> 
> 

I see your point, but what would somebody do with built, but not executing
request?

[mef, 2015-09-28 17:13:37]

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:363: for
(Pair<String, String> header : builder.mRequestHeaders) {
On 2015/09/28 14:18:12, pauljensen wrote:
> On 2015/09/25 21:32:19, mef wrote:
> > should we switch from Pair<String, String> to Map.Entry<String, String> for
> > consistency?
> 
> CronetUrlRequest uses an ArrayList<Pair<String, String>> so I think my code is
> consistent.

Ack. That will change once https://codereview.chromium.org/1359343005/ lands,
but I suppose currently it is consistent.

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/1363723002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:831:
final UrlRequest urlRequest = future.get();
On 2015/09/28 14:18:12, pauljensen wrote:
> On 2015/09/25 21:32:19, mef wrote:
> > why do you need it?
> 
> line 838, 840, 856, and 871 read it

Ah, that's the pattern change, as we don't have request until executeRequest is
called.
I think this is another reason to follow Helen's suggestion and have an explicit
UrlRequest.Builder.build() method.

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/READ...
File components/cronet/README.md (right):

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/READ...
components/cronet/README.md:76: "https://www.example.com", listener, executor);
On 2015/09/28 16:31:38, mef wrote:
> On 2015/09/28 14:57:05, xunjieli wrote:
> > Why doesn't the UrlRequest.Builder have a build method? It seems unintuitive
> > that we have a builder but don't build it.
> > 
> > I would expect executeRequest takes in a request or requestBuilder.build():
> > 
> > UrlRequest.Builder requestBuilder = new UrlRequest.Builder();
> > UrlRequest request = requestBuilder.build();
> > engine.executeRequest(request);
> > 
> > 
> 
> I see your point, but what would somebody do with built, but not executing
> request?

I take it back. Looking at
http://cr.openjdk.java.net/~michaelm/httpclient.old/CCCv1.1/javadoc/ it seems to
make sense to separate building from execution.

[mef, 2015-09-28 17:25:57]

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:39: boolean
mDisableCache;
should this have an explicit default?

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:58: public
Builder(String url, UrlRequestListener listener, Executor executor) {
given that all arguments are required, should we add null checks and throw
NullPointerException if they are not specified?

[xunjieli, 2015-09-28 18:17:57]

xunjieli@chromium.org changed reviewers:
- xunjieli@chromium.org

[pauljensen, 2015-09-30 15:41:33]

I updated UrlRequest.Builder and CronetEngine.Builder to both have build()
functions with no arguments.  This was a pretty large change.  PTAL.

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:39: boolean
mDisableCache;
On 2015/09/28 17:25:57, mef wrote:
> should this have an explicit default?

Java initializes boolean's to false.  I think that should be enough, but I don't
feel strongly.  Do you think I should explicitly add " = false"?

https://codereview.chromium.org/1363723002/diff/140001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:58: public
Builder(String url, UrlRequestListener listener, Executor executor) {
On 2015/09/28 17:25:57, mef wrote:
> given that all arguments are required, should we add null checks and throw
> NullPointerException if they are not specified?

Done.

[xunjieli, 2015-10-01 19:00:09]

xunjieli@chromium.org changed reviewers:
+ xunjieli@chromium.org

[xunjieli, 2015-10-01 19:00:09]

Thanks. I like this change!

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:33: *
Default config enables SPDY, disables QUIC, SDCH and http cache.
nit: s/http/HTTP.

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java
(right):

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java:50:
public CronetURLStreamHandlerFactory(CronetEngine.Builder builder) {
I think the client should be the one who call CronetEngine.Build(). Can we make
this constructor take in an Engine instead?

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
File
components/cronet/android/test/javaperftests/src/org/chromium/net/CronetPerfTestActivity.java
(right):

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
components/cronet/android/test/javaperftests/src/org/chromium/net/CronetPerfTestActivity.java:352:
mCronetEngine.executeRequest(builder);
need to update tests?

[pauljensen, 2015-10-01 23:42:59]

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:33: *
Default config enables SPDY, disables QUIC, SDCH and http cache.
On 2015/10/01 19:00:09, xunjieli wrote:
> nit: s/http/HTTP.

Done.

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java
(right):

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java:50:
public CronetURLStreamHandlerFactory(CronetEngine.Builder builder) {
On 2015/10/01 19:00:09, xunjieli wrote:
> I think the client should be the one who call CronetEngine.Build(). Can we
make
> this constructor take in an Engine instead?

Done.

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
File
components/cronet/android/test/javaperftests/src/org/chromium/net/CronetPerfTestActivity.java
(right):

https://codereview.chromium.org/1363723002/diff/200001/components/cronet/andr...
components/cronet/android/test/javaperftests/src/org/chromium/net/CronetPerfTestActivity.java:352:
mCronetEngine.executeRequest(builder);
On 2015/10/01 19:00:09, xunjieli wrote:
> need to update tests?

Done.

[xunjieli, 2015-10-02 15:19:33]

LGTM. thanks!

[mef, 2015-10-02 16:02:27]

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:378: *
@deprecated Use {@link #executeRequest}.
This says 'use executeRequest', but there is no executeRequest method?

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java
(right):

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java:24:
public HttpUrlConnectionUrlRequestFactory(CronetEngine.Builder builder) {
Per conversation with Paul this changes legacy API that is spread *everywhere*.
Boo.

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:89: public
void setHttpMethod(String method) {
should [set|add]Xyz methods return |this| to facilitate chaining?

[mef, 2015-10-02 16:46:58]

Looks great! Changes to callers are simple and logical.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:215: *
@param maxSize maximum size used to cache data (advisory and maybe
should we spell out units of maxSize?

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:357:
return createContext(this);
Should it use createCronetEngine?

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:21:
public HttpUrlRequestFactoryConfig(Context context) {
this needs default constructor.

Can we do super(null) here?

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:255:
final UrlRequest.Builder requestBuilder = new UrlRequest.Builder(
I wonder if request requestBuilder should be mRequestBuilder created in
constructor. 
At least it will get the check that url is not null?

Is it possible for url to change between construction and start?

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:161:
.enableHTTP2(true)
nit: alignment.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:219:
builder.build().start();
FWIW this looks pretty and clean.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:110:
new
CronetEngine.Builder(mActivity).setLibraryName("cronet_tests").build().shutdown();
is mActivity a correct target context here? Should we replace
getInstrumentation().getTargetContext() with mActivity everywhere?

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:157:
UrlRequest.Builder builder = new UrlRequest.Builder(
minor nit: I'd call it urlRequestBuilder as 'builder' is a bit generic, but I
can be convinced otherwise.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:64:
new CronetEngine.Builder(context).setLibraryName("cronet_tests"));
will this work?

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:137:
HttpUrlRequestFactoryConfig config = new HttpUrlRequestFactoryConfig(null);
Can we get rid of null?

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/GetStatusTest.java
(left):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/GetStatusTest.java:62:
TestStatusListener statusListener0 = new TestStatusListener();
Can't do that, nice!

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/HttpUrlRequestFactoryTest.java:26:
HttpUrlRequestFactoryConfig config = new HttpUrlRequestFactoryConfig(null);
Is it possible to get these tests to build an pass without any changes?

[pauljensen, 2015-10-02 18:23:15]

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:378: *
@deprecated Use {@link #executeRequest}.
On 2015/10/02 16:02:27, mef wrote:
> This says 'use executeRequest', but there is no executeRequest method?

Done.

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java
(right):

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java:24:
public HttpUrlConnectionUrlRequestFactory(CronetEngine.Builder builder) {
On 2015/10/02 16:02:27, mef wrote:
> Per conversation with Paul this changes legacy API that is spread
*everywhere*.
> Boo.

Done.

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/1363723002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:89: public
void setHttpMethod(String method) {
On 2015/10/02 16:02:27, mef wrote:
> should [set|add]Xyz methods return |this| to facilitate chaining?

SGTM, done.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:215: *
@param maxSize maximum size used to cache data (advisory and maybe
On 2015/10/02 16:46:58, mef wrote:
> should we spell out units of maxSize?

Done.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:357:
return createContext(this);
On 2015/10/02 16:46:58, mef wrote:
> Should it use createCronetEngine?

For now I think createContext is better because it adds the UserAgent.  In the
future we can consider combining them perhaps.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:21:
public HttpUrlRequestFactoryConfig(Context context) {
On 2015/10/02 16:46:58, mef wrote:
> this needs default constructor.
> 
> Can we do super(null) here?

Done.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:255:
final UrlRequest.Builder requestBuilder = new UrlRequest.Builder(
On 2015/10/02 16:46:58, mef wrote:
> I wonder if request requestBuilder should be mRequestBuilder created in
> constructor. 
> At least it will get the check that url is not null?
> 
> Is it possible for url to change between construction and start?

I like the shorter scope here.  Seems more memory efficient (might go on the
stack rather than get GC'ed).  It also prevents accidental use of
mRequestBuilder after the UrlRequest is constructed.
The URL cannot change between construction and startRequest().

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:161:
.enableHTTP2(true)
On 2015/10/02 16:46:58, mef wrote:
> nit: alignment.

coding style is 8 character indent on wrapped lines.  "git cl format" enforces
that.  It just so happened "myConfig" was 8 characters previously.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:219:
builder.build().start();
On 2015/10/02 16:46:58, mef wrote:
> FWIW this looks pretty and clean.

Indeed, I just worry that someone might forget the "start()" call.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:110:
new
CronetEngine.Builder(mActivity).setLibraryName("cronet_tests").build().shutdown();
On 2015/10/02 16:46:58, mef wrote:
> is mActivity a correct target context here? Should we replace
> getInstrumentation().getTargetContext() with mActivity everywhere?

I should have left this one as getInstrumentation().getTargetContext(), setting
back.  I don't think we should replace getInstrumentation().getTargetContext()
with mActivity everywhere.  I don't think it matters too much.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:157:
UrlRequest.Builder builder = new UrlRequest.Builder(
On 2015/10/02 16:46:58, mef wrote:
> minor nit: I'd call it urlRequestBuilder as 'builder' is a bit generic, but I
> can be convinced otherwise.

Done.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:64:
new CronetEngine.Builder(context).setLibraryName("cronet_tests"));
On 2015/10/02 16:46:58, mef wrote:
> will this work?

I reverted this file.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:137:
HttpUrlRequestFactoryConfig config = new HttpUrlRequestFactoryConfig(null);
On 2015/10/02 16:46:58, mef wrote:
> Can we get rid of null?

I reverted this file.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/GetStatusTest.java
(left):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/GetStatusTest.java:62:
TestStatusListener statusListener0 = new TestStatusListener();
On 2015/10/02 16:46:58, mef wrote:
> Can't do that, nice! 

Oops, actually you can now that we added back start(), reverted back to old
test.

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/HttpUrlRequestFactoryTest.java:26:
HttpUrlRequestFactoryConfig config = new HttpUrlRequestFactoryConfig(null);
On 2015/10/02 16:46:58, mef wrote:
> Is it possible to get these tests to build an pass without any changes?

Done.

[pauljensen, 2015-10-02 23:12:00]

Misha, awaiting your review.

[mef, 2015-10-02 23:54:19]

On 2015/10/02 23:12:00, pauljensen wrote:
> Misha, awaiting your review.

Sorry, I ran out to meeting and than to the Martian. 
If you don't mind I'll take a look on Monday, I'd rather have Ben's NQE landed
first.

[pauljensen, 2015-10-03 00:45:32]

On 2015/10/02 23:54:19, mef wrote:
> On 2015/10/02 23:12:00, pauljensen wrote:
> > Misha, awaiting your review.
> 
> Sorry, I ran out to meeting and than to the Martian. 
> If you don't mind I'll take a look on Monday, I'd rather have Ben's NQE landed
> first.

No rush; Monday is just fine; I just wanted to make sure you knew it was pending
your review.

[mef, 2015-10-05 16:41:52]

Looks good, but per our discussion, could we hold off on this until Ben lands
his CL that customers are waiting for?

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1363723002/diff/260001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetEngine.java:357:
return createContext(this);
On 2015/10/02 18:23:15, pauljensen wrote:
> On 2015/10/02 16:46:58, mef wrote:
> > Should it use createCronetEngine?
> 
> For now I think createContext is better because it adds the UserAgent.  In the
> future we can consider combining them perhaps.

Acknowledged.

https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java
(right):

https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:14:
public class UrlRequestContextConfig extends CronetEngine.Builder {
I'm confused by Rietveld. Is UrlRequestContextConfig being deleted or modified?

[pauljensen, 2015-10-05 16:59:40]

https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java
(right):

https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:14:
public class UrlRequestContextConfig extends CronetEngine.Builder {
On 2015/10/05 16:41:52, mef wrote:
> I'm confused by Rietveld. Is UrlRequestContextConfig being deleted or
modified?

Modified, or more accurately, deprecated, so as to still be available for
embedders of the old API.

[mef, 2015-10-05 17:18:48]

https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java
(right):

https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:14:
public class UrlRequestContextConfig extends CronetEngine.Builder {
On 2015/10/05 16:59:40, pauljensen wrote:
> On 2015/10/05 16:41:52, mef wrote:
> > I'm confused by Rietveld. Is UrlRequestContextConfig being deleted or
> modified?
> 
> Modified, or more accurately, deprecated, so as to still be available for
> embedders of the old API.

In this case, do we still need HttpUrl* changes?

[pauljensen, 2015-10-05 18:09:56]

On 2015/10/05 17:18:48, mef wrote:
>
https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
> File
>
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java
> (right):
> 
>
https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
>
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:14:
> public class UrlRequestContextConfig extends CronetEngine.Builder {
> On 2015/10/05 16:59:40, pauljensen wrote:
> > On 2015/10/05 16:41:52, mef wrote:
> > > I'm confused by Rietveld. Is UrlRequestContextConfig being deleted or
> > modified?
> > 
> > Modified, or more accurately, deprecated, so as to still be available for
> > embedders of the old API.
> 
> In this case, do we still need HttpUrl* changes?

It's quite confusing, but yes.  Since it will be hard to migrate users of the
old API off of UrlRequestContextConfigs easily, I made UrlRequestContextConfig
simply inherit from CronetEngine.Builder.  So in our testing code we have shared
code to generate CronetEngine.Builders/UrlRequestContextConfigs.  Since the
"context" part of a CronetEngine.Builder is final, and is null in
UrlRequestContextConfig and non-null in CronetEngine.Builder, there is no easy
way to get a UrlRequestContextConfig from a CronetEngine.Builder.  So I had to
make the testing code generate either a CronetEngine.Builder or a
UrlRequestContextConfig.  I chose a CronetEngine.Builder as it's easy to
transition from a UrlRequestContextConfig to a CronetEngine.Builder, i.e. they
implicitly upcast.  Anyhow, for these reasons I modified the HttpUrl* code to
accept CronetEngine.Builder as it allows accepting either a CronetEngine.Builder
or a UrlRequestContextCongig.

[mef, 2015-10-05 19:52:43]

https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactoryTest.java
(right):

https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactoryTest.java:20:
CronetTestActivity activity = launchCronetTestApp();
FindBugs reported the following issues:
DLS_DEAD_LOCAL_STORE: Dead store to local variable
  In class org.chromium.net.urlconnection.CronetURLStreamHandlerFactoryTest
  In method
org.chromium.net.urlconnection.CronetURLStreamHandlerFactoryTest.testRequireConfig()
  At CronetURLStreamHandlerFactoryTest.java:[line 20]

[pauljensen, 2015-10-06 13:07:09]

https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactoryTest.java
(right):

https://codereview.chromium.org/1363723002/diff/300001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactoryTest.java:20:
CronetTestActivity activity = launchCronetTestApp();
On 2015/10/05 19:52:43, mef wrote:
> FindBugs reported the following issues:
> DLS_DEAD_LOCAL_STORE: Dead store to local variable
>   In class org.chromium.net.urlconnection.CronetURLStreamHandlerFactoryTest
>   In method
>
org.chromium.net.urlconnection.CronetURLStreamHandlerFactoryTest.testRequireConfig()
>   At CronetURLStreamHandlerFactoryTest.java:[line 20]

Done.

[pauljensen, 2015-10-07 13:46:27]

Misha, awaiting your review.  I'd like to land this sooner than later, now that
Ben's NQE API change is in.

[mef, 2015-10-07 15:15:13]

On 2015/10/07 13:46:27, pauljensen wrote:
> Misha, awaiting your review.  I'd like to land this sooner than later, now
that
> Ben's NQE API change is in.

Will do, although I suspect that you'll have to rebase it on top of Ben's
changes.

[mef, 2015-10-07 15:15:16]

On 2015/10/07 13:46:27, pauljensen wrote:
> Misha, awaiting your review.  I'd like to land this sooner than later, now
that
> Ben's NQE API change is in.

Will do, although I suspect that you'll have to rebase it on top of Ben's
changes.

[pauljensen, 2015-10-07 16:46:23]

On 2015/10/07 15:15:16, mef wrote:
> On 2015/10/07 13:46:27, pauljensen wrote:
> > Misha, awaiting your review.  I'd like to land this sooner than later, now
> that
> > Ben's NQE API change is in.
> 
> Will do, although I suspect that you'll have to rebase it on top of Ben's
> changes.

It's rebased and ready to go.

[pauljensen, 2015-10-07 17:34:14]

On 2015/10/07 16:46:23, pauljensen wrote:
> On 2015/10/07 15:15:16, mef wrote:
> > On 2015/10/07 13:46:27, pauljensen wrote:
> > > Misha, awaiting your review.  I'd like to land this sooner than later, now
> > that
> > > Ben's NQE API change is in.
> > 
> > Will do, although I suspect that you'll have to rebase it on top of Ben's
> > changes.
> 
> It's rebased and ready to go.

Erm, *now* it's rebased and ready to go.

[pauljensen, 2015-10-07 17:34:49]

On 2015/10/07 17:34:14, pauljensen wrote:
> Erm, *now* it's rebased and ready to go.
(I had to fix up Ben's tests to work with my renames).

[mef, 2015-10-07 20:30:13]

lgtm

[pauljensen, 2015-10-07 22:49:38]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2015-10-07 22:49:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/1363723002/#ps360001
(title: "update Ben's tests")

[commit-bot: I haz the power, 2015-10-07 22:51:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363723002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363723002/360001

[commit-bot: I haz the power, 2015-10-08 00:25:12]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-10-08 00:26:14]

Patchset 19 (id:??) landed as
https://crrev.com/6182d654df33116d7c4e4616c7b51d4e8b28d77f
Cr-Commit-Position: refs/heads/master@{#352986}