Add SetSupportsQuic method to explicitly specify server that supports QUIC.

Add SetSupportsQuic method to explicitly specify server that supports QUIC.

BUG=411010
TEST=build/android/test_runner.py instrumentation --test-apk=CronetSampleTest -f *Quic*

Committed: https://crrev.com/c71361c6408da79dfd759b1f74ba5b43638cb113
Cr-Commit-Position: refs/heads/master@{#295063}

[mef, 2014-09-05 16:18:32]

mef@chromium.org changed reviewers:
+ clm@google.com, fnk@google.com, mmenke@chromium.org, rch@chromium.org

[mef, 2014-09-05 16:21:02]

Please take a look to confirm that I'm going in the right direction. :)

I'm going to add a HttpUrlRequest.wasFetchedOverQUIC() method to verify whether
QUIC is used for particular request...

[mmenke, 2014-09-05 17:20:34]

I think rather than WasFetchedOverQuic, we should add a way to get the protocol,
which returns one of (HTTP, SPDY, QUIC), and maybe includes a version with
minor/major version numbers as well.

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:95:
* @param probability that server supports QUIC (0..1)
This seems like a bad idea to me.

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:97:
public void setSupportsQuic(String server,
Suggest calling this setQuicHint, which leaves a bit more room for non-QUIC
fallback.

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:97:
public void setSupportsQuic(String server,
I strongly suggest we throw an exception if |server| is not a URL.  Think it
would be easy to accidentally do "setSupportsQuic("www.google.com", ...)". 
Could make nativeSetSupportsQuic return false on invalid URLs (And maybe
non-http/https ones as well), and throw on that - think that's simpler than
validating in Java.

[Ryan Hamilton, 2014-09-05 18:03:17]

On 2014/09/05 17:20:34, mmenke wrote:
> I think rather than WasFetchedOverQuic, we should add a way to get the
protocol,
> which returns one of (HTTP, SPDY, QUIC), and maybe includes a version with
> minor/major version numbers as well.

HttpResponseInfo::ConnectionInfo was designed specifically to address the needs
of this CL. I recommend plumbing this through where needed and tweaking the
values if they are not high fidelity enough.

[mmenke, 2014-09-05 18:07:47]

On 2014/09/05 18:03:17, Ryan Hamilton wrote:
> On 2014/09/05 17:20:34, mmenke wrote:
> > I think rather than WasFetchedOverQuic, we should add a way to get the
> protocol,
> > which returns one of (HTTP, SPDY, QUIC), and maybe includes a version with
> > minor/major version numbers as well.
> 
> HttpResponseInfo::ConnectionInfo was designed specifically to address the
needs
> of this CL. I recommend plumbing this through where needed and tweaking the
> values if they are not high fidelity enough.

My general feeling is that enums that include protocol versions are a pain -
we're stuck keeping SPDY2 around forever, for example, unless we want to
potentially break embedders, and have to update public Java files whenever a new
version of SPDY or QUIC is introduced.  And is HTTP/0.9 HTTP1 or UNKNOWN?

[mef, 2014-09-05 18:15:43]

Would it make sense to expose this to Java:

  // Protocol negotiated with the server.
  std::string npn_negotiated_protocol;

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:95:
* @param probability that server supports QUIC (0..1)
On 2014/09/05 17:20:34, mmenke wrote:
> This seems like a bad idea to me.

Done.

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:97:
public void setSupportsQuic(String server,
On 2014/09/05 17:20:33, mmenke wrote:
> Suggest calling this setQuicHint, which leaves a bit more room for non-QUIC
> fallback.

Done.

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:97:
public void setSupportsQuic(String server,
On 2014/09/05 17:20:34, mmenke wrote:
> I strongly suggest we throw an exception if |server| is not a URL.  Think it
> would be easy to accidentally do "setSupportsQuic("www.google.com", ...)". 
> Could make nativeSetSupportsQuic return false on invalid URLs (And maybe
> non-http/https ones as well), and throw on that - think that's simpler than
> validating in Java.

Done.

[Charles, 2014-09-05 18:16:22]

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:97:
public void setSupportsQuic(String server,
This isn't very useful, as API goes - a normal user has no access to the
ChromiumUrlRequestContext object. You should put this configuration in the
HttpUrlRequestFactoryConfig.

[Ryan Hamilton, 2014-09-05 18:17:56]

On 2014/09/05 18:07:47, mmenke wrote:
> On 2014/09/05 18:03:17, Ryan Hamilton wrote:
> > On 2014/09/05 17:20:34, mmenke wrote:
> > > I think rather than WasFetchedOverQuic, we should add a way to get the
> > protocol,
> > > which returns one of (HTTP, SPDY, QUIC), and maybe includes a version with
> > > minor/major version numbers as well.
> > 
> > HttpResponseInfo::ConnectionInfo was designed specifically to address the
> needs
> > of this CL. I recommend plumbing this through where needed and tweaking the
> > values if they are not high fidelity enough.
> 
> My general feeling is that enums that include protocol versions are a pain -
> we're stuck keeping SPDY2 around forever, for example, unless we want to
> potentially break embedders, and have to update public Java files whenever a
new
> version of SPDY or QUIC is introduced.  And is HTTP/0.9 HTTP1 or UNKNOWN?

Why do we have to keep SPDY2 around forever? Because some embedder might refer
to that value and their code will no longer compile? Hm. Interesting. Would you
prefer a string? For HTTP/2 (and even SPDY, for that matter) the version numbers
can become quite funky 3 vs 3.1 vs 4a2.  *boggle*

That being said, if you have a recommendation for what you'd like to see, go for
it!

[Ryan Hamilton, 2014-09-05 18:19:59]

On 2014/09/05 18:15:43, mef wrote:
> Would it make sense to expose this to Java:
> 
>   // Protocol negotiated with the server.
>   std::string npn_negotiated_protocol;

QUIC does not use NPN, so it would be better to pick a different name. (And for
that matter, SPDY and HTTP/2 are transitioning from NPN -> ALPN)

[mmenke, 2014-09-05 18:27:11]

On 2014/09/05 18:17:56, Ryan Hamilton wrote:
> On 2014/09/05 18:07:47, mmenke wrote:
> > On 2014/09/05 18:03:17, Ryan Hamilton wrote:
> > > On 2014/09/05 17:20:34, mmenke wrote:
> > > > I think rather than WasFetchedOverQuic, we should add a way to get the
> > > protocol,
> > > > which returns one of (HTTP, SPDY, QUIC), and maybe includes a version
with
> > > > minor/major version numbers as well.
> > > 
> > > HttpResponseInfo::ConnectionInfo was designed specifically to address the
> > needs
> > > of this CL. I recommend plumbing this through where needed and tweaking
the
> > > values if they are not high fidelity enough.
> > 
> > My general feeling is that enums that include protocol versions are a pain -
> > we're stuck keeping SPDY2 around forever, for example, unless we want to
> > potentially break embedders, and have to update public Java files whenever a
> new
> > version of SPDY or QUIC is introduced.  And is HTTP/0.9 HTTP1 or UNKNOWN?
> 
> Why do we have to keep SPDY2 around forever? Because some embedder might refer
> to that value and their code will no longer compile? Hm. Interesting. Would
you
> prefer a string? For HTTP/2 (and even SPDY, for that matter) the version
numbers
> can become quite funky 3 vs 3.1 vs 4a2.  *boggle*
> 
> That being said, if you have a recommendation for what you'd like to see, go
for
> it!


That's exactly the reason.  Suppose an embedder wanted to know if spdy was used.
 Then they'd use type >= SPDY2 && type <= SPDY4...but that breaks if SPDY5 is
added, and it fails to compile it SPDY2 is removed.

I also think passing a single string, like "SPDY2" or "QUIC/1", etc, forces the
embedder into writing their own version parsing code (If, for instance, they
just want to know if SPDY was used, but don't care about version, as in the
above example), which is why I was suggesting separating it out into multiple
values.  That having been said, I do think a single string is an improvement.

[mef, 2014-09-05 18:27:39]

On 2014/09/05 18:19:59, Ryan Hamilton wrote:
> On 2014/09/05 18:15:43, mef wrote:
> > Would it make sense to expose this to Java:
> > 
> >   // Protocol negotiated with the server.
> >   std::string npn_negotiated_protocol;
> 
> QUIC does not use NPN, so it would be better to pick a different name. (And
for
> that matter, SPDY and HTTP/2 are transitioning from NPN -> ALPN)

Ignoring the name, would this value be meaningful for QUIC, HTTP/2, etc?
What would be a better name? How about getProtocolInfoString()?

[mef, 2014-09-05 19:07:28]

I've added getNegotiatedProtocol method to get npn_negotiated_protocol string,
which appears to be set to "quic/1+spdy/3" for QUIC.

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:97:
public void setSupportsQuic(String server,
On 2014/09/05 18:16:22, Charles wrote:
> This isn't very useful, as API goes - a normal user has no access to the
> ChromiumUrlRequestContext object. You should put this configuration in the
> HttpUrlRequestFactoryConfig.

Acknowledged. This is geared towards advanced user. :) ChromiumUrlRequestContext
is accessible using ChromiumUrlRequestFactory.getRequestContext() method.

I have no objections to putting this into HttpUrlRequestFactoryConfig, and will
happily do so if it makes sense.

Beauty of this method is that it could actually be called after instantiation of
factory/context.

[Charles, 2014-09-05 19:24:53]

On 2014/09/05 19:07:28, mef wrote:
> I've added getNegotiatedProtocol method to get npn_negotiated_protocol string,
> which appears to be set to "quic/1+spdy/3" for QUIC.
> 
>
https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
> File
>
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
> (right):
> 
>
https://codereview.chromium.org/544223003/diff/1/components/cronet/android/ja...
>
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:97:
> public void setSupportsQuic(String server,
> On 2014/09/05 18:16:22, Charles wrote:
> > This isn't very useful, as API goes - a normal user has no access to the
> > ChromiumUrlRequestContext object. You should put this configuration in the
> > HttpUrlRequestFactoryConfig.
> 
> Acknowledged. This is geared towards advanced user. :)
ChromiumUrlRequestContext
> is accessible using ChromiumUrlRequestFactory.getRequestContext() method.
That's still not useful, because it requires casting from
HttpUrlRequestFactory.createFactory(). We want everyone using createFactory,
right?
> 
> I have no objections to putting this into HttpUrlRequestFactoryConfig, and
will
> happily do so if it makes sense.
I think it makes sense. I'd like all configuration to go there.
> 
> Beauty of this method is that it could actually be called after instantiation
of
> factory/context.
Yes, but it's not particularly expensive, so there's no real benefit. There is,
however, complication added by requiring the caller to figure out when they
ought to call this method.

[mef, 2014-09-05 22:09:22]

PTAL, I've added QUIC hints to HttpUrlRequestFactoryConfig.

[mef, 2014-09-08 20:38:46]

ping?

[mmenke, 2014-09-08 20:46:35]

On 2014/09/08 20:38:46, mef wrote:
> ping?

I'll get to this tomorrow...  I'm experimenting with a "No codereviews that
require thought after 2:00, when I spent the morning on reviews" policy. 
Otherwise, at the rate I'm going, I may never get to the upload stuff.

[mmenke, 2014-09-09 14:53:36]

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/chromium_url_request.cc (right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/chromium_url_request.cc:395: if (request == NULL) {
Looks like we're pretty inconsistent about checking this.  (See, for example,
AppendChunk, GetErrorString, and GetHttpStatusCode).  If it can't happen, we
shouldn't be checking for it.  If it can, we should be more consistent (And have
tests).

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/chromium_url_request.cc:398: std::string
negotiated_protocol = request->GetNegotiatedProtocol();
If we haven't negotiated a protocol yet, I assume this is an empty string?  If
so, think we should also be returning that in the request==NULL case, if that
code is worth having (I'm skeptical that it is, unless it can actually happen -
and if it can, we should probably be throwing exceptions, actually).

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/chromium_url_request_context.cc (right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/chromium_url_request_context.cc:165: 0.5f);
Any reason for the 0.5?

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java:105: *
Returns protocol (e.g. "quic/1+spdy/3") negotiated with server.
Maybe add "Returns empty string if no protocol was negotiated, or the protocol
not known."?

Also, what do we return when using SPDY proxies?  What about CONNECT requests
over SPDY/HTTP proxies?

I don't think it's clear that we don't return HTTP versions (I'm assuming we
don't, at least?)

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:83:
* String, path to directory for HTTP Cache and Cookie Storage.
Cleanup:  A lot of these should be prefixed with @param name

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:91:
* Note that enableHttpCache(DISK) is needed to take advantage or 0-RTT.
nit:  or -> of.

Also, maybe add "0-RTT connection establishment between sessions."

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:94:
* @param alternatePort to use for QUIC
nit:  End comments with periods.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:25:
config.addQuicHint("google.com", 0);
0 uses the default port?  Should update addQuicHint description to reflect that.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/url_request_adapter.h (right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/url_request_adapter.h:113: std::string
GetNegotiatedProtocol() const;
We don't return HTTP versions?  Should we?

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/url_request_context_adapter.cc:229: void
URLRequestContextAdapter::SetQuicHint(const net::HostPortPair& server,
Do we really need multiple ways of setting per-server QUIC information?

[fnk, 2014-09-09 15:03:12]

On 2014/09/09 14:53:36, mmenke wrote:
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> File components/cronet/android/chromium_url_request.cc (right):
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> components/cronet/android/chromium_url_request.cc:395: if (request == NULL) {
> Looks like we're pretty inconsistent about checking this.  (See, for example,
> AppendChunk, GetErrorString, and GetHttpStatusCode).  If it can't happen, we
> shouldn't be checking for it.  If it can, we should be more consistent (And
have
> tests).
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> components/cronet/android/chromium_url_request.cc:398: std::string
> negotiated_protocol = request->GetNegotiatedProtocol();
> If we haven't negotiated a protocol yet, I assume this is an empty string?  If
> so, think we should also be returning that in the request==NULL case, if that
> code is worth having (I'm skeptical that it is, unless it can actually happen
-
> and if it can, we should probably be throwing exceptions, actually).
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> File components/cronet/android/chromium_url_request_context.cc (right):
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> components/cronet/android/chromium_url_request_context.cc:165: 0.5f);
> Any reason for the 0.5?
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> File components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java
> (right):
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java:105: *
> Returns protocol (e.g. "quic/1+spdy/3") negotiated with server.
> Maybe add "Returns empty string if no protocol was negotiated, or the protocol
> not known."?
> 
> Also, what do we return when using SPDY proxies?  What about CONNECT requests
> over SPDY/HTTP proxies?
> 
> I don't think it's clear that we don't return HTTP versions (I'm assuming we
> don't, at least?)
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> File
>
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java
> (right):
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
>
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:83:
> * String, path to directory for HTTP Cache and Cookie Storage.
> Cleanup:  A lot of these should be prefixed with @param name
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
>
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:91:
> * Note that enableHttpCache(DISK) is needed to take advantage or 0-RTT.
> nit:  or -> of.
> 
> Also, maybe add "0-RTT connection establishment between sessions."
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
>
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:94:
> * @param alternatePort to use for QUIC
> nit:  End comments with periods.
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> File
>
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
> (right):
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
>
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:25:
> config.addQuicHint("google.com", 0);
> 0 uses the default port?  Should update addQuicHint description to reflect
that.
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> File components/cronet/android/url_request_adapter.h (right):
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> components/cronet/android/url_request_adapter.h:113: std::string
> GetNegotiatedProtocol() const;
> We don't return HTTP versions?  Should we?
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> File components/cronet/android/url_request_context_adapter.cc (right):
> 
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> components/cronet/android/url_request_context_adapter.cc:229: void
> URLRequestContextAdapter::SetQuicHint(const net::HostPortPair& server,
> Do we really need multiple ways of setting per-server QUIC information?

It just occured to me that this will not work for clients that have a lot of
distinct hosts (i.e., me). Can we look into hard-enabling QUIC for everything
instead?

[Charles, 2014-09-09 16:40:06]

On 2014/09/09 15:03:12, fnk wrote:
> On 2014/09/09 14:53:36, mmenke wrote:
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > File components/cronet/android/chromium_url_request.cc (right):
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > components/cronet/android/chromium_url_request.cc:395: if (request == NULL)
{
> > Looks like we're pretty inconsistent about checking this.  (See, for
example,
> > AppendChunk, GetErrorString, and GetHttpStatusCode).  If it can't happen, we
> > shouldn't be checking for it.  If it can, we should be more consistent (And
> have
> > tests).
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > components/cronet/android/chromium_url_request.cc:398: std::string
> > negotiated_protocol = request->GetNegotiatedProtocol();
> > If we haven't negotiated a protocol yet, I assume this is an empty string? 
If
> > so, think we should also be returning that in the request==NULL case, if
that
> > code is worth having (I'm skeptical that it is, unless it can actually
happen
> -
> > and if it can, we should probably be throwing exceptions, actually).
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > File components/cronet/android/chromium_url_request_context.cc (right):
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > components/cronet/android/chromium_url_request_context.cc:165: 0.5f);
> > Any reason for the 0.5?
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > File components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java:105:
*
> > Returns protocol (e.g. "quic/1+spdy/3") negotiated with server.
> > Maybe add "Returns empty string if no protocol was negotiated, or the
protocol
> > not known."?
> > 
> > Also, what do we return when using SPDY proxies?  What about CONNECT
requests
> > over SPDY/HTTP proxies?
> > 
> > I don't think it's clear that we don't return HTTP versions (I'm assuming we
> > don't, at least?)
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > File
> >
>
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> >
>
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:83:
> > * String, path to directory for HTTP Cache and Cookie Storage.
> > Cleanup:  A lot of these should be prefixed with @param name
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> >
>
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:91:
> > * Note that enableHttpCache(DISK) is needed to take advantage or 0-RTT.
> > nit:  or -> of.
> > 
> > Also, maybe add "0-RTT connection establishment between sessions."
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> >
>
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:94:
> > * @param alternatePort to use for QUIC
> > nit:  End comments with periods.
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > File
> >
>
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> >
>
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:25:
> > config.addQuicHint("google.com", 0);
> > 0 uses the default port?  Should update addQuicHint description to reflect
> that.
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > File components/cronet/android/url_request_adapter.h (right):
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > components/cronet/android/url_request_adapter.h:113: std::string
> > GetNegotiatedProtocol() const;
> > We don't return HTTP versions?  Should we?
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > File components/cronet/android/url_request_context_adapter.cc (right):
> > 
> >
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > components/cronet/android/url_request_context_adapter.cc:229: void
> > URLRequestContextAdapter::SetQuicHint(const net::HostPortPair& server,
> > Do we really need multiple ways of setting per-server QUIC information?
> 
> It just occured to me that this will not work for clients that have a lot of
> distinct hosts (i.e., me). Can we look into hard-enabling QUIC for everything
> instead?

A global switch would be fine for us too, but it seems like specified host
method is easier to implement.

[Ryan Hamilton, 2014-09-09 17:02:55]

On 2014/09/09 15:03:12, fnk wrote:
> On 2014/09/09 14:53:36, mmenke wrote:
>
https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
> > components/cronet/android/url_request_context_adapter.cc:229: void
> > URLRequestContextAdapter::SetQuicHint(const net::HostPortPair& server,
> > Do we really need multiple ways of setting per-server QUIC information?
> 
> It just occured to me that this will not work for clients that have a lot of
> distinct hosts (i.e., me). Can we look into hard-enabling QUIC for everything
> instead?

Remember that QUIC supports the idea of canonical hosts:

  crypto_config_.AddCanonicalSuffix(".c.youtube.com");
  crypto_config_.AddCanonicalSuffix(".googlevideo.com");

  canoncial_suffixes_.push_back(".c.youtube.com");
  canoncial_suffixes_.push_back(".googlevideo.com");
  canoncial_suffixes_.push_back(".googleusercontent.com");

This means that if we think we can speak QUIC to any host matching those
suffixes, then we this all such hosts can speak QUIC.

Perhaps this helps your use case.

[Charles, 2014-09-09 17:03:56]

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:394:
public String getNegotiatedProtocol() {
Check to make sure we've received headers first, and that we're not recycled.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java:426:
return "";
http/1.1?

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java:105: *
Returns protocol (e.g. "quic/1+spdy/3") negotiated with server.
Must be called after onResponseStarted?

[mef, 2014-09-09 21:30:49]

I haven't addressed any of the comments :( but I gout this to work. :)

Few notes:

- I ForceAlternateProtocol(QUIC) in
URLRequestContextBuilder::SetSpdyAndQuicEnabled if QUIC is enabled. This affects
*ALL* hosts, but it is good for experiment. 
- I couldn't get http_server_properties()->SetAlternateProtocol() to do the
right thing so far. 

- I've only got test to succeed using LTE network.

Feel free to patch and play, I'll try to clean it up tomorrow.

[mef, 2014-09-10 16:48:18]

PTAL, I've cleaned it up and addressed review comments.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/chromium_url_request.cc (right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/chromium_url_request.cc:395: if (request == NULL) {
On 2014/09/09 14:53:35, mmenke wrote:
> Looks like we're pretty inconsistent about checking this.  (See, for example,
> AppendChunk, GetErrorString, and GetHttpStatusCode).  If it can't happen, we
> shouldn't be checking for it.  If it can, we should be more consistent (And
have
> tests).

Acknowledged. I think we should have these checks everywhere, but I'm not sure
whether that belongs to this CL.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/chromium_url_request.cc:398: std::string
negotiated_protocol = request->GetNegotiatedProtocol();
On 2014/09/09 14:53:35, mmenke wrote:
> If we haven't negotiated a protocol yet, I assume this is an empty string?  If
> so, think we should also be returning that in the request==NULL case, if that
> code is worth having (I'm skeptical that it is, unless it can actually happen
-
> and if it can, we should probably be throwing exceptions, actually).

Done.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/chromium_url_request_context.cc (right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/chromium_url_request_context.cc:165: 0.5f);
On 2014/09/09 14:53:35, mmenke wrote:
> Any reason for the 0.5?

Done.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:394:
public String getNegotiatedProtocol() {
On 2014/09/09 17:03:56, Charles wrote:
> Check to make sure we've received headers first, and that we're not recycled.

Done.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java:426:
return "";
On 2014/09/09 17:03:56, Charles wrote:
> http/1.1?
From what I see Chromium stack returns empty string if it has not negotiated any
NPN protocol.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java:105: *
Returns protocol (e.g. "quic/1+spdy/3") negotiated with server.
On 2014/09/09 17:03:56, Charles wrote:
> Must be called after onResponseStarted?

Done.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java:105: *
Returns protocol (e.g. "quic/1+spdy/3") negotiated with server.
On 2014/09/09 14:53:35, mmenke wrote:
> Maybe add "Returns empty string if no protocol was negotiated, or the protocol
> not known."?
Done.
> 
> Also, what do we return when using SPDY proxies?  What about CONNECT requests
> over SPDY/HTTP proxies?
I have no idea. :( Ryan, do you?
> 
> I don't think it's clear that we don't return HTTP versions (I'm assuming we
> don't, at least?)
Done.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:83:
* String, path to directory for HTTP Cache and Cookie Storage.
On 2014/09/09 14:53:35, mmenke wrote:
> Cleanup:  A lot of these should be prefixed with @param name

Acknowledged.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:91:
* Note that enableHttpCache(DISK) is needed to take advantage or 0-RTT.
On 2014/09/09 14:53:35, mmenke wrote:
> nit:  or -> of.
> 
> Also, maybe add "0-RTT connection establishment between sessions."

Done.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:94:
* @param alternatePort to use for QUIC
On 2014/09/09 14:53:35, mmenke wrote:
> nit:  End comments with periods.

Done.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:25:
config.addQuicHint("google.com", 0);
On 2014/09/09 14:53:35, mmenke wrote:
> 0 uses the default port?  Should update addQuicHint description to reflect
that.
It was my feeble attempt at guessing correct params, I don't think 0 is valid
here.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/url_request_adapter.h (right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/url_request_adapter.h:113: std::string
GetNegotiatedProtocol() const;
On 2014/09/09 14:53:35, mmenke wrote:
> We don't return HTTP versions? Should we?
I don't think so.

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/60001/components/cronet/androi...
components/cronet/android/url_request_context_adapter.cc:229: void
URLRequestContextAdapter::SetQuicHint(const net::HostPortPair& server,
On 2014/09/09 14:53:35, mmenke wrote:
> Do we really need multiple ways of setting per-server QUIC information?
Probably not, I'll be happy to remove it.

[mmenke, 2014-09-10 18:21:27]

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
File components/cronet/android/chromium_url_request.cc (right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/chromium_url_request.cc:397: }
nit:  Think the preference is not to use braces on single-line ifs.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java
(right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java:107: *
Does not return HTTP versions. Must be called after onResponseStarted
Maybe "Does not return HTTP versions" -> "Returns empty when using plain http or
https."

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:192:
String quicURL = "https://www.google.com:443";
Per discussion, we shouldn't be testing with a remote server.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
File components/cronet/android/url_request_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/url_request_adapter.cc:105: if (url_request_ == NULL)
The general chrome philosophy is to not both have a DCHECK (As they should never
happen), and handle its failure.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:147: // Currently
(circa M39) enabling QUIC requires setting probability threshold.
On 2014/09/10 16:48:18, mef wrote:
> On 2014/09/09 14:53:35, mmenke wrote:
> > Do we really need multiple ways of setting per-server QUIC information?
> Probably not, I'll be happy to remove it.

Great!  I suggest keeping this one and getting rid of the other, though I'm
fine, either way.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:157:
quic_hint.alternate_port < std::numeric_limits<uint16>::min() ||
port 0 is also invalid, no?

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:165:
DCHECK(!quic_hint_host.IsEmpty());
Why DCHECK on empty hosts, but LOG(ERROR) on missing ports?  We should have a
consistent philosophy about error handling.  Since this is designed to allow
loading from a file, I guess handling errors makes more sense?

https://codereview.chromium.org/544223003/diff/100001/components/cronet/url_r...
File components/cronet/url_request_context_config.h (right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/url_r...
components/cronet/url_request_context_config.h:62: ScopedVector<QuicHint>
quic_hints;
Should probably include the header for this

https://codereview.chromium.org/544223003/diff/100001/components/cronet/url_r...
components/cronet/url_request_context_config.h:65:
DISALLOW_COPY_AND_ASSIGN(URLRequestContextConfig);
include macros.h

[mef, 2014-09-10 20:18:49]

Thanks! I'll work on removing testing against google.com.
I'll need to pull in an inproc QUIC server, which requires refactoring the
library to build one with and without test utilities.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
File components/cronet/android/chromium_url_request.cc (right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/chromium_url_request.cc:397: }
On 2014/09/10 18:21:27, mmenke wrote:
> nit:  Think the preference is not to use braces on single-line ifs.

Done.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java
(right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequest.java:107: *
Does not return HTTP versions. Must be called after onResponseStarted
On 2014/09/10 18:21:27, mmenke wrote:
> Maybe "Does not return HTTP versions" -> "Returns empty when using plain http
or
> https."

Done.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:192:
String quicURL = "https://www.google.com:443";
On 2014/09/10 18:21:27, mmenke wrote:
> Per discussion, we shouldn't be testing with a remote server.

Acknowledged. This will require Helen's refactoring to create libcronettest.so
with inproc quic server.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
File components/cronet/android/url_request_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/url_request_adapter.cc:105: if (url_request_ == NULL)
On 2014/09/10 18:21:27, mmenke wrote:
> The general chrome philosophy is to not both have a DCHECK (As they should
never
> happen), and handle its failure.

Done. It was my test aid. :)

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:147: // Currently
(circa M39) enabling QUIC requires setting probability threshold.
On 2014/09/10 18:21:27, mmenke wrote:
> On 2014/09/10 16:48:18, mef wrote:
> > On 2014/09/09 14:53:35, mmenke wrote:
> > > Do we really need multiple ways of setting per-server QUIC information?
> > Probably not, I'll be happy to remove it.
> 
> Great!  I suggest keeping this one and getting rid of the other, though I'm
> fine, either way.

Acknowledged.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:157:
quic_hint.alternate_port < std::numeric_limits<uint16>::min() ||
On 2014/09/10 18:21:27, mmenke wrote:
> port 0 is also invalid, no?

Done.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:165:
DCHECK(!quic_hint_host.IsEmpty());
On 2014/09/10 18:21:27, mmenke wrote:
> Why DCHECK on empty hosts, but LOG(ERROR) on missing ports?  We should have a
> consistent philosophy about error handling.  Since this is designed to allow
> loading from a file, I guess handling errors makes more sense?

Done.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/url_r...
File components/cronet/url_request_context_config.h (right):

https://codereview.chromium.org/544223003/diff/100001/components/cronet/url_r...
components/cronet/url_request_context_config.h:62: ScopedVector<QuicHint>
quic_hints;
On 2014/09/10 18:21:27, mmenke wrote:
> Should probably include the header for this

Done.

https://codereview.chromium.org/544223003/diff/100001/components/cronet/url_r...
components/cronet/url_request_context_config.h:65:
DISALLOW_COPY_AND_ASSIGN(URLRequestContextConfig);
On 2014/09/10 18:21:27, mmenke wrote:
> include macros.h

Done.

[mmenke, 2014-09-10 20:22:01]

I don't think this needs another pass until you have tests working.  If you feel
otherwise, please say so.

[Ryan Hamilton, 2014-09-10 20:58:19]

Just a few comments.

https://codereview.chromium.org/544223003/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/544223003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_context_builder.cc:245:
HttpServerPropertiesImpl::ForceAlternateProtocol(pair);
Please do not actually land this :>

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:151: if
(!config->quic_hints.empty()) {
I'm not sure that this guard saves you anything... You might as well just hit
the for() loop, which will terminate once it checks the size of hints. (And it
lets you outdent the code one level.

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:157:
quic_hint_server.EffectiveIntPort());
If you change the types of the QuicHint members, I think you can simplify this
code to some extent.

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:164: }
Does SetAlternateProtocol perform similar checks? If so, perhaps you can skip
this check.

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.h (right):

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.h:69: void
SetQuicHint(const net::HostPortPair& server,
Please add a comment for this method.

https://codereview.chromium.org/544223003/diff/120001/components/cronet/url_r...
File components/cronet/url_request_context_config.h (right):

https://codereview.chromium.org/544223003/diff/120001/components/cronet/url_r...
components/cronet/url_request_context_config.h:33: std::string server;
If this is a URL it seems like the type should be GURL, shouldn't it? If it's
not a GURL, it seems like it should be a HostPortPair (or two a std::string
host; uint16 port fields). Since it seems that you're going to set the hit at
the host:port level, that's probably the right type for this member.

https://codereview.chromium.org/544223003/diff/120001/net/url_request/url_req...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/544223003/diff/120001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:357:
network_session_params.enable_quic = http_network_session_params_.enable_quic;
Do you need to deal with alternate_protocol_probability_threshold?

[mmenke, 2014-09-10 21:10:18]

https://codereview.chromium.org/544223003/diff/120001/components/cronet/url_r...
File components/cronet/url_request_context_config.h (right):

https://codereview.chromium.org/544223003/diff/120001/components/cronet/url_r...
components/cronet/url_request_context_config.h:33: std::string server;
On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> If this is a URL it seems like the type should be GURL, shouldn't it? If it's
> not a GURL, it seems like it should be a HostPortPair (or two a std::string
> host; uint16 port fields). Since it seems that you're going to set the hit at
> the host:port level, that's probably the right type for this member.

This is created automatically from a JSON-formatted string (Which ultimately
comes from Java), hence the matching what one can JSON-encode.  We'll ultimately
need some sort of validation somewhere.

[mef, 2014-09-12 19:44:34]

PTAL. Per conversation with Matt I've disabled testQuicLoadUrl until I can use
local Quic server.

That turned out to be challenging as Quic end-to-end test is currently
unavailable on Android, so I'll try to add that in the follow-up CL.

https://codereview.chromium.org/544223003/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/544223003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_context_builder.cc:245:
HttpServerPropertiesImpl::ForceAlternateProtocol(pair);
On 2014/09/10 20:58:18, Ryan Hamilton wrote:
> Please do not actually land this :>

Done. I mean undone.

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:151: if
(!config->quic_hints.empty()) {
On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> I'm not sure that this guard saves you anything... You might as well just hit
> the for() loop, which will terminate once it checks the size of hints. (And it
> lets you outdent the code one level.

Done.

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:157:
quic_hint_server.EffectiveIntPort());
On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> If you change the types of the QuicHint members, I think you can simplify this
> code to some extent.
As Matt has mention this is de-serialized from JSON strings from Java side.

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:164: }
On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> Does SetAlternateProtocol perform similar checks? If so, perhaps you can skip
> this check.
Umm, SetAlternateProtocol just uses host_port_pair as a key to the map. I've
added this to debug my quic hint insertion issues.

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.h (right):

https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.h:69: void
SetQuicHint(const net::HostPortPair& server,
On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> Please add a comment for this method.

Acknowledged. Per discussion with mmenke@ I'm going to remove it.

https://codereview.chromium.org/544223003/diff/120001/net/url_request/url_req...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/544223003/diff/120001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:357:
network_session_params.enable_quic = http_network_session_params_.enable_quic;
On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> Do you need to deal with alternate_protocol_probability_threshold?
Yes, that's done in URLRequestContextAdapter::InitializeURLRequestContext

[mef, 2014-09-15 13:30:33]

On 2014/09/12 19:44:34, mef wrote:
> PTAL. Per conversation with Matt I've disabled testQuicLoadUrl until I can use
> local Quic server.
> 
> That turned out to be challenging as Quic end-to-end test is currently
> unavailable on Android, so I'll try to add that in the follow-up CL.
> 
>
https://codereview.chromium.org/544223003/diff/80001/net/url_request/url_requ...
> File net/url_request/url_request_context_builder.cc (right):
> 
>
https://codereview.chromium.org/544223003/diff/80001/net/url_request/url_requ...
> net/url_request/url_request_context_builder.cc:245:
> HttpServerPropertiesImpl::ForceAlternateProtocol(pair);
> On 2014/09/10 20:58:18, Ryan Hamilton wrote:
> > Please do not actually land this :>
> 
> Done. I mean undone.
> 
>
https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
> File components/cronet/android/url_request_context_adapter.cc (right):
> 
>
https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
> components/cronet/android/url_request_context_adapter.cc:151: if
> (!config->quic_hints.empty()) {
> On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> > I'm not sure that this guard saves you anything... You might as well just
hit
> > the for() loop, which will terminate once it checks the size of hints. (And
it
> > lets you outdent the code one level.
> 
> Done.
> 
>
https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
> components/cronet/android/url_request_context_adapter.cc:157:
> quic_hint_server.EffectiveIntPort());
> On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> > If you change the types of the QuicHint members, I think you can simplify
this
> > code to some extent.
> As Matt has mention this is de-serialized from JSON strings from Java side.
> 
>
https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
> components/cronet/android/url_request_context_adapter.cc:164: }
> On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> > Does SetAlternateProtocol perform similar checks? If so, perhaps you can
skip
> > this check.
> Umm, SetAlternateProtocol just uses host_port_pair as a key to the map. I've
> added this to debug my quic hint insertion issues.
> 
>
https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
> File components/cronet/android/url_request_context_adapter.h (right):
> 
>
https://codereview.chromium.org/544223003/diff/120001/components/cronet/andro...
> components/cronet/android/url_request_context_adapter.h:69: void
> SetQuicHint(const net::HostPortPair& server,
> On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> > Please add a comment for this method.
> 
> Acknowledged. Per discussion with mmenke@ I'm going to remove it.
> 
>
https://codereview.chromium.org/544223003/diff/120001/net/url_request/url_req...
> File net/url_request/url_request_context_builder.cc (right):
> 
>
https://codereview.chromium.org/544223003/diff/120001/net/url_request/url_req...
> net/url_request/url_request_context_builder.cc:357:
> network_session_params.enable_quic = http_network_session_params_.enable_quic;
> On 2014/09/10 20:58:19, Ryan Hamilton wrote:
> > Do you need to deal with alternate_protocol_probability_threshold?
> Yes, that's done in URLRequestContextAdapter::InitializeURLRequestContext

ping

[mmenke, 2014-09-15 13:34:59]

rch:  I'm waiting on you to chime in about the updated QUIC setup, before
doing another pass.

On Mon, Sep 15, 2014 at 9:30 AM, <mef@chromium.org> wrote:

> On 2014/09/12 19:44:34, mef wrote:
>
>> PTAL. Per conversation with Matt I've disabled testQuicLoadUrl until I
>> can use
>> local Quic server.
>>
>
>  That turned out to be challenging as Quic end-to-end test is currently
>> unavailable on Android, so I'll try to add that in the follow-up CL.
>>
>
>
> https://codereview.chromium.org/544223003/diff/80001/net/
> url_request/url_request_context_builder.cc
>
>> File net/url_request/url_request_context_builder.cc (right):
>>
>
>
> https://codereview.chromium.org/544223003/diff/80001/net/
> url_request/url_request_context_builder.cc#newcode245
>
>> net/url_request/url_request_context_builder.cc:245:
>> HttpServerPropertiesImpl::ForceAlternateProtocol(pair);
>> On 2014/09/10 20:58:18, Ryan Hamilton wrote:
>> > Please do not actually land this :>
>>
>
>  Done. I mean undone.
>>
>
>
> https://codereview.chromium.org/544223003/diff/120001/
> components/cronet/android/url_request_context_adapter.cc
>
>> File components/cronet/android/url_request_context_adapter.cc (right):
>>
>
>
> https://codereview.chromium.org/544223003/diff/120001/
> components/cronet/android/url_request_context_adapter.cc#newcode151
>
>> components/cronet/android/url_request_context_adapter.cc:151: if
>> (!config->quic_hints.empty()) {
>> On 2014/09/10 20:58:19, Ryan Hamilton wrote:
>> > I'm not sure that this guard saves you anything... You might as well
>> just
>>
> hit
>
>> > the for() loop, which will terminate once it checks the size of hints.
>> (And
>>
> it
>
>> > lets you outdent the code one level.
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.org/544223003/diff/120001/
> components/cronet/android/url_request_context_adapter.cc#newcode157
>
>> components/cronet/android/url_request_context_adapter.cc:157:
>> quic_hint_server.EffectiveIntPort());
>> On 2014/09/10 20:58:19, Ryan Hamilton wrote:
>> > If you change the types of the QuicHint members, I think you can
>> simplify
>>
> this
>
>> > code to some extent.
>> As Matt has mention this is de-serialized from JSON strings from Java
>> side.
>>
>
>
> https://codereview.chromium.org/544223003/diff/120001/
> components/cronet/android/url_request_context_adapter.cc#newcode164
>
>> components/cronet/android/url_request_context_adapter.cc:164: }
>> On 2014/09/10 20:58:19, Ryan Hamilton wrote:
>> > Does SetAlternateProtocol perform similar checks? If so, perhaps you can
>>
> skip
>
>> > this check.
>> Umm, SetAlternateProtocol just uses host_port_pair as a key to the map.
>> I've
>> added this to debug my quic hint insertion issues.
>>
>
>
> https://codereview.chromium.org/544223003/diff/120001/
> components/cronet/android/url_request_context_adapter.h
>
>> File components/cronet/android/url_request_context_adapter.h (right):
>>
>
>
> https://codereview.chromium.org/544223003/diff/120001/
> components/cronet/android/url_request_context_adapter.h#newcode69
>
>> components/cronet/android/url_request_context_adapter.h:69: void
>> SetQuicHint(const net::HostPortPair& server,
>> On 2014/09/10 20:58:19, Ryan Hamilton wrote:
>> > Please add a comment for this method.
>>
>
>  Acknowledged. Per discussion with mmenke@ I'm going to remove it.
>>
>
>
> https://codereview.chromium.org/544223003/diff/120001/net/
> url_request/url_request_context_builder.cc
>
>> File net/url_request/url_request_context_builder.cc (right):
>>
>
>
> https://codereview.chromium.org/544223003/diff/120001/net/
> url_request/url_request_context_builder.cc#newcode357
>
>> net/url_request/url_request_context_builder.cc:357:
>> network_session_params.enable_quic = http_network_session_params_.
>> enable_quic;
>> On 2014/09/10 20:58:19, Ryan Hamilton wrote:
>> > Do you need to deal with alternate_protocol_probability_threshold?
>> Yes, that's done in URLRequestContextAdapter::InitializeURLRequestContext
>>
>
> ping
>
> https://codereview.chromium.org/544223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Hamilton, 2014-09-15 18:29:45]

Overall, I think this looks good. One comment about the format of the hint, and
a couple of small comments.

https://codereview.chromium.org/544223003/diff/220001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java
(right):

https://codereview.chromium.org/544223003/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:97:
public HttpUrlRequestFactoryConfig addQuicHint(String server,
Do you expect callers will effectively hard-code calls to this method into their
code? If so, I think that this method should take 3 arguments:

 String host,
 int port
 int alternate_port

Then I would say:

addQuicHint("www.google.com", 443, 443);

And change the QuicHint to have these three fields, instead of single field that
is a URL in which the path (and query and anchor) are ignored.

https://codereview.chromium.org/544223003/diff/220001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/220001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:151: if
(!config->quic_hints.empty()) {
Consider breaking this new chunk of code (lines 147-174) into a method, and
using early returns instead of nesting. Perhaps this will end up too
complicated, so ignore this if so.

Or, just remove this check, and let the guard on the for loop take care of this.
 (I think I suggested that in a previous patch, but perhaps that was at a
different place).

https://codereview.chromium.org/544223003/diff/220001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:162: LOG(ERROR) <<
"Invalid QUIC hint: " << quic_hint.server;
Since there are actually two different potential problems with the QUIC hint
(bad server or bad alt-port) perhaps this should be split into two checks so
that the LOG(ERROR) prints out the part of the hint that is problematic?

[mef, 2014-09-15 21:01:01]

Ryan, thanks, PTAL. Matt, WDYT?

https://codereview.chromium.org/544223003/diff/220001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java
(right):

https://codereview.chromium.org/544223003/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:97:
public HttpUrlRequestFactoryConfig addQuicHint(String server,
On 2014/09/15 18:29:44, Ryan Hamilton wrote:
> Do you expect callers will effectively hard-code calls to this method into
their
> code? If so, I think that this method should take 3 arguments:
> 
>  String host,
>  int port
>  int alternate_port
> 
> Then I would say:
> 
> addQuicHint("www.google.com", 443, 443);
> 
> And change the QuicHint to have these three fields, instead of single field
that
> is a URL in which the path (and query and anchor) are ignored. 

Done.

https://codereview.chromium.org/544223003/diff/220001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/220001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:151: if
(!config->quic_hints.empty()) {
On 2014/09/15 18:29:44, Ryan Hamilton wrote:
> Consider breaking this new chunk of code (lines 147-174) into a method, and
> using early returns instead of nesting. Perhaps this will end up too
> complicated, so ignore this if so.
> 
> Or, just remove this check, and let the guard on the for loop take care of
this.
>  (I think I suggested that in a previous patch, but perhaps that was at a
> different place).

Done.

https://codereview.chromium.org/544223003/diff/220001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:162: LOG(ERROR) <<
"Invalid QUIC hint: " << quic_hint.server;
On 2014/09/15 18:29:44, Ryan Hamilton wrote:
> Since there are actually two different potential problems with the QUIC hint
> (bad server or bad alt-port) perhaps this should be split into two checks so
> that the LOG(ERROR) prints out the part of the hint that is problematic?

Done.

[Ryan Hamilton, 2014-09-15 21:19:40]

the QUIC-specific pieces LGTM, but mmenke understands the cronet pieces better.

[mmenke, 2014-09-16 01:51:39]

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java
(right):

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:95:
* @param port port of the server that supports QUIC.
nit:  Port

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:96:
* @param alternatePort to use for QUIC.
Should probably be consistent here:  Either use the variable name as part of the
string, or don't.  As I have no idea hos JavaDoc formats comments, I suggest not
doing it.

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java
(right):

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:216:
netLogFile);
Do we really need to much with NetLog?

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:220:
// Try several times as first request may not use QUIC.
Add a TODO about getting rid of this as well.

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:154: net::HostPortPair
quic_hint_host(quic_hint.host, quic_hint.port);
optional nit:  Suggest calling this quic_hint_host_port_pair.  Wordier, but we
call them host_port_pair almost everywhere.

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:156: LOG(ERROR) <<
"Invalid QUIC hint host: " << quic_hint.host;
This line seems a bit silly, seeing as IsEmpty only returns true if both the
host is the empty string and the port is valid.

[mef, 2014-09-16 08:44:15]

Matt, thanks, ptal!

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java
(right):

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:95:
* @param port port of the server that supports QUIC.
On 2014/09/16 01:51:38, mmenke wrote:
> nit:  Port

Done.

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactoryConfig.java:96:
* @param alternatePort to use for QUIC.
On 2014/09/16 01:51:38, mmenke wrote:
> Should probably be consistent here:  Either use the variable name as part of
the
> string, or don't.  As I have no idea hos JavaDoc formats comments, I suggest
not
> doing it.

Done.

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java
(right):

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:216:
netLogFile);
On 2014/09/16 01:51:38, mmenke wrote:
> Do we really need to much with NetLog?

Done.

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:220:
// Try several times as first request may not use QUIC.
On 2014/09/16 01:51:38, mmenke wrote:
> Add a TODO about getting rid of this as well.

Done.

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:154: net::HostPortPair
quic_hint_host(quic_hint.host, quic_hint.port);
On 2014/09/16 01:51:38, mmenke wrote:
> optional nit:  Suggest calling this quic_hint_host_port_pair.  Wordier, but we
> call them host_port_pair almost everywhere.

Done.

https://codereview.chromium.org/544223003/diff/280001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:156: LOG(ERROR) <<
"Invalid QUIC hint host: " << quic_hint.host;
On 2014/09/16 01:51:38, mmenke wrote:
> This line seems a bit silly, seeing as IsEmpty only returns true if both the
> host is the empty string and the port is valid.

Done.

[mmenke, 2014-09-16 14:21:03]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-09-16 14:21:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/544223003/320001

[commit-bot: I haz the power, 2014-09-16 14:50:40]

Committed patchset #17 (id:320001) as 37e8d813e4de50f1e86849606b7bad085f0b25a6

[commit-bot: I haz the power, 2014-09-16 14:53:21]

Patchset 17 (id:??) landed as
https://crrev.com/c71361c6408da79dfd759b1f74ba5b43638cb113
Cr-Commit-Position: refs/heads/master@{#295063}

[Ryan Hamilton, 2014-09-17 22:07:26]

https://codereview.chromium.org/544223003/diff/320001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/544223003/diff/320001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:150:
->SetAlternateProtocolProbabilityThreshold(1.0f);
Doh! I should have noticed this before. This should be 0.0f not 1.0f.  Sorry!

[mmenke, 2014-09-17 22:13:49]

On 2014/09/17 22:07:26, Ryan Hamilton wrote:
>
https://codereview.chromium.org/544223003/diff/320001/components/cronet/andro...
> File components/cronet/android/url_request_context_adapter.cc (right):
> 
>
https://codereview.chromium.org/544223003/diff/320001/components/cronet/andro...
> components/cronet/android/url_request_context_adapter.cc:150:
> ->SetAlternateProtocolProbabilityThreshold(1.0f);
> Doh! I should have noticed this before. This should be 0.0f not 1.0f.  Sorry!

Hrm...this should probably be documented somewhere - at least neither
HttpServerProperties nor HttpServerPropertiesImpl provide any documentation for
it.  HttpServerPropertiesImpl::HasAlternateProtocol does make it clear that this
is the threshold above which we use alternate protocols, I suppose, but that's a
bit deeper than one should have to go.

[Ryan Hamilton, 2014-09-17 22:16:51]

On 2014/09/17 22:13:49, mmenke wrote:
> On 2014/09/17 22:07:26, Ryan Hamilton wrote:
> >
>
https://codereview.chromium.org/544223003/diff/320001/components/cronet/andro...
> > File components/cronet/android/url_request_context_adapter.cc (right):
> > 
> >
>
https://codereview.chromium.org/544223003/diff/320001/components/cronet/andro...
> > components/cronet/android/url_request_context_adapter.cc:150:
> > ->SetAlternateProtocolProbabilityThreshold(1.0f);
> > Doh! I should have noticed this before. This should be 0.0f not 1.0f. 
Sorry!
> 
> Hrm...this should probably be documented somewhere - at least neither
> HttpServerProperties nor HttpServerPropertiesImpl provide any documentation
for
> it.  HttpServerPropertiesImpl::HasAlternateProtocol does make it clear that
this
> is the threshold above which we use alternate protocols, I suppose, but that's
a
> bit deeper than one should have to go.

Yup, you're right. I'll add a comment to the relevant methods in
HttpServerProperties.

[chromium-reviews, 2014-09-17 22:17:46]

I'll send a CL.

On Wed, Sep 17, 2014 at 3:13 PM, <mmenke@chromium.org> wrote:

> On 2014/09/17 22:07:26, Ryan Hamilton wrote:
>
> https://codereview.chromium.org/544223003/diff/320001/
> components/cronet/android/url_request_context_adapter.cc
>
>> File components/cronet/android/url_request_context_adapter.cc (right):
>>
>
>
> https://codereview.chromium.org/544223003/diff/320001/
> components/cronet/android/url_request_context_adapter.cc#newcode150
>
>> components/cronet/android/url_request_context_adapter.cc:150:
>> ->SetAlternateProtocolProbabilityThreshold(1.0f);
>> Doh! I should have noticed this before. This should be 0.0f not 1.0f.
>> Sorry!
>>
>
> Hrm...this should probably be documented somewhere - at least neither
> HttpServerProperties nor HttpServerPropertiesImpl provide any
> documentation for
> it.  HttpServerPropertiesImpl::HasAlternateProtocol does make it clear
> that this
> is the threshold above which we use alternate protocols, I suppose, but
> that's a
> bit deeper than one should have to go.
>
> https://codereview.chromium.org/544223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.