[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

- QUIC will soon require that user agent id field is non-empty, but normal User-Agent request header is too rich and verbose to be used for this purpose. The app can override default value using 'user_agent_id' quic experimental option.

- prefer_aes experimental option is needed to experiment with performance of using AES instead of ChaCha20.

BUG=575771
Committed: https://crrev.com/c5da5713830e1c6ad09c75638e06f018396ca40d
Cr-Commit-Position: refs/heads/master@{#374438}

[mef, 2016-02-02 22:01:18]

Description was changed from

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes options.

BUG=575771
==========

to

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes options.

BUG=575771
==========

[mef, 2016-02-02 22:01:28]

Description was changed from

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes options.

BUG=575771
==========

to

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

BUG=575771
==========

[mef, 2016-02-03 15:44:39]

Description was changed from

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

BUG=575771
==========

to

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

- QUIC will soon require that user agent id field is non-empty, but normal
User-Agent request header is too rich and verbose to be used for this purpose.

- prefer_aes experimental option is needed to experiment with performance of
using AES instead of ChaCha20.

BUG=575771
==========

[mef, 2016-02-03 15:44:39]

mef@chromium.org changed reviewers:
+ rch@chromium.org, xunjieli@chromium.org

[mef, 2016-02-03 15:58:02]

PTAL

[Ryan Hamilton, 2016-02-04 01:34:43]

I don't see a test which verifies that the generated QUIC UIAD looks like
"<something> cronet/<version>". Did I miss it?

[xunjieli, 2016-02-04 14:00:06]

https://codereview.chromium.org/1665503002/diff/40001/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/1665503002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:263: public
String getDefaultQuicUserAgentId() {
I feel that since we are in experimental stage here, we should avoid adding more
public APIs, which likely will be removed when QUIC in Cronet matures. Should we
consider passing the values using setExperimentalOptions? That way it is more
flexible in case we want to remove anything. Also will client be setting the UIC
User Agent themselves? If not, we can just use the default string.

[mef, 2016-02-05 17:22:36]

Thanks for your comments! I agree with Helen that UAID doesn't clearly belong to
the public API.

I've made getDefaultQuicUserAgentId a package-private part of
CronetEngine.Builder and added 'user_agent_id' to the set of quic experimental
options.

Please take a look.

https://codereview.chromium.org/1665503002/diff/100001/components/cronet/andr...
File components/cronet/android/test/cronet_url_request_context_config_test.cc
(right):

https://codereview.chromium.org/1665503002/diff/100001/components/cronet/andr...
components/cronet/android/test/cronet_url_request_context_config_test.cc:36:
CHECK_NE(config->quic_user_agent_id.find("Cronet/" CRONET_VERSION),
This now tests that default UAID ends with Cronet/1.2.3.4

[mef, 2016-02-05 17:24:10]

Description was changed from

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

- QUIC will soon require that user agent id field is non-empty, but normal
User-Agent request header is too rich and verbose to be used for this purpose.

- prefer_aes experimental option is needed to experiment with performance of
using AES instead of ChaCha20.

BUG=575771
==========

to

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

- QUIC will soon require that user agent id field is non-empty, but normal
User-Agent request header is too rich and verbose to be used for this purpose.
The app can override default value using 'user_agent_id' quic experimental
option.

- prefer_aes experimental option is needed to experiment with performance of
using AES instead of ChaCha20.

BUG=575771
==========

[Ryan Hamilton, 2016-02-05 17:47:52]

net/ and quic-specific bits LGTM. hopefully xunjieli can reivew the cronet parts
which I'm not terribly familiar with.

https://codereview.chromium.org/1665503002/diff/100001/components/cronet/andr...
File components/cronet/android/test/cronet_url_request_context_config_test.cc
(right):

https://codereview.chromium.org/1665503002/diff/100001/components/cronet/andr...
components/cronet/android/test/cronet_url_request_context_config_test.cc:36:
CHECK_NE(config->quic_user_agent_id.find("Cronet/" CRONET_VERSION),
On 2016/02/05 17:22:36, mef wrote:
> This now tests that default UAID ends with Cronet/1.2.3.4

Great, thanks!

[xunjieli, 2016-02-05 18:55:11]

It feels a bit weird that the Cronet will set a default QUIC UAID in all cases.
I think we should only do so if we are told to by the client, since that does
involve sending information about the client to the server without client
explicitly knows about it. Will the client ever want to set a custom one that is
different from the default? If not, I think we should have a boolean that is
mapped to set_quic_user_agent_id, same as prefer_aes. If that's set, we will set
the QUIC UAID to be the default string of package + cronet version.

[mef, 2016-02-05 19:15:02]

On 2016/02/05 18:55:11, xunjieli wrote:
> It feels a bit weird that the Cronet will set a default QUIC UAID in all
cases.

See https://code.google.com/p/chromium/issues/detail?id=575771#c9
This is expected to be soon a required part of quic handshake.

> I think we should only do so if we are told to by the client, since that does
> involve sending information about the client to the server without client
> explicitly knows about it. 

I think this goes back to your comment on public vs experimental api.

> Will the client ever want to set a custom one that is
> different from the default?

Good question, I suppose they may prefer to use the app version, or somesuch.
IIRC the customization was suggested by somebody on quic team.

> If not, I think we should have a boolean that is
> mapped to set_quic_user_agent_id, same as prefer_aes. If that's set, we will
set
> the QUIC UAID to be the default string of package + cronet version.

Realistically having an experimental option string accomplishes similar goal. 
Setting it to "" would effectively eliminate the id.

[xunjieli, 2016-02-05 19:45:16]

On 2016/02/05 19:15:02, mef wrote:
> On 2016/02/05 18:55:11, xunjieli wrote:
> > It feels a bit weird that the Cronet will set a default QUIC UAID in all
> cases.
> 
> See https://code.google.com/p/chromium/issues/detail?id=575771#c9
> This is expected to be soon a required part of quic handshake.
Thanks for the reference. I think we should probably not do that until it's
required. Or only do it in QUIC's case (when enableQUIC is set).
> 
> > I think we should only do so if we are told to by the client, since that
does
> > involve sending information about the client to the server without client
> > explicitly knows about it. 
> 
> I think this goes back to your comment on public vs experimental api.
> 
> > Will the client ever want to set a custom one that is
> > different from the default?
> 
> Good question, I suppose they may prefer to use the app version, or somesuch.
> IIRC the customization was suggested by somebody on quic team.

Ryan, do you think Cronet clients will ever need a customized UAID? Should it be
just standardized to app package name + cronet version ?

> > If not, I think we should have a boolean that is
> > mapped to set_quic_user_agent_id, same as prefer_aes. If that's set, we will
> set
> > the QUIC UAID to be the default string of package + cronet version.
> 
> Realistically having an experimental option string accomplishes similar goal. 
> Setting it to "" would effectively eliminate the id.

Right, but the experimental option is hidden -- Normal embedders wouldn't know
about it to set the string back to "". Sending UAID (which is almost PII) is not
documented as a part of our public API, so I think we should not do it by
default.

[Ryan Hamilton, 2016-02-05 22:28:17]

On 2016/02/05 19:45:16, xunjieli wrote:
> On 2016/02/05 19:15:02, mef wrote:
> > On 2016/02/05 18:55:11, xunjieli wrote:
> > > It feels a bit weird that the Cronet will set a default QUIC UAID in all
> > cases.
> > 
> > See https://code.google.com/p/chromium/issues/detail?id=575771#c9
> > This is expected to be soon a required part of quic handshake.
> Thanks for the reference. I think we should probably not do that until it's
> required. Or only do it in QUIC's case (when enableQUIC is set).
> > 
> > > I think we should only do so if we are told to by the client, since that
> does
> > > involve sending information about the client to the server without client
> > > explicitly knows about it. 
> > 
> > I think this goes back to your comment on public vs experimental api.
> > 
> > > Will the client ever want to set a custom one that is
> > > different from the default?
> > 
> > Good question, I suppose they may prefer to use the app version, or
somesuch.
> > IIRC the customization was suggested by somebody on quic team.
> 
> Ryan, do you think Cronet clients will ever need a customized UAID? Should it
be
> just standardized to app package name + cronet version ?

I think that default would probably be fine.

> > > If not, I think we should have a boolean that is
> > > mapped to set_quic_user_agent_id, same as prefer_aes. If that's set, we
will
> > set
> > > the QUIC UAID to be the default string of package + cronet version.
> > 
> > Realistically having an experimental option string accomplishes similar
goal. 
> > Setting it to "" would effectively eliminate the id.
> 
> Right, but the experimental option is hidden -- Normal embedders wouldn't know
> about it to set the string back to "". Sending UAID (which is almost PII) is
not
> documented as a part of our public API, so I think we should not do it by
> default.

[mef, 2016-02-08 16:46:01]

On 2016/02/05 22:28:17, Ryan Hamilton wrote:
> On 2016/02/05 19:45:16, xunjieli wrote:
> > On 2016/02/05 19:15:02, mef wrote:
> > > On 2016/02/05 18:55:11, xunjieli wrote:
> > > > It feels a bit weird that the Cronet will set a default QUIC UAID in all
> > > cases.
> > > 
> > > See https://code.google.com/p/chromium/issues/detail?id=575771#c9
> > > This is expected to be soon a required part of quic handshake.
> > Thanks for the reference. I think we should probably not do that until it's
> > required. Or only do it in QUIC's case (when enableQUIC is set).
> > > 
> > > > I think we should only do so if we are told to by the client, since that
> > does
> > > > involve sending information about the client to the server without
client
> > > > explicitly knows about it. 
> > > 
> > > I think this goes back to your comment on public vs experimental api.
> > > 
> > > > Will the client ever want to set a custom one that is
> > > > different from the default?
> > > 
> > > Good question, I suppose they may prefer to use the app version, or
> somesuch.
> > > IIRC the customization was suggested by somebody on quic team.
> > 
> > Ryan, do you think Cronet clients will ever need a customized UAID? Should
it
> be
> > just standardized to app package name + cronet version ?
> 
> I think that default would probably be fine.
> 
> > > > If not, I think we should have a boolean that is
> > > > mapped to set_quic_user_agent_id, same as prefer_aes. If that's set, we
> will
> > > set
> > > > the QUIC UAID to be the default string of package + cronet version.
> > > 
> > > Realistically having an experimental option string accomplishes similar
> goal. 
> > > Setting it to "" would effectively eliminate the id.
> > 
> > Right, but the experimental option is hidden -- Normal embedders wouldn't
know
> > about it to set the string back to "". Sending UAID (which is almost PII) is
> not
> > documented as a part of our public API, so I think we should not do it by
> > default.

I don't have strong preference, but I think that default behavior should require
minimal effort from the client.
Please also note, that Cronet adds User-Agent header (with reasonable, but even
more verbose default) to every request that doesn't set it explicitly.

Setting default UAID with possible override matches that behavior.

Exposing public api 'enableQuicUaId' doesn't make sense if we expect to always
require non-empty UAID.

[xunjieli, 2016-02-08 16:49:31]

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

[xunjieli, 2016-02-08 16:51:09]

Paul, do you mind taking a look? Not sure what is the best action here. (I
prefer not setting the QUIC UAID in experimental options at all if embedder has
not specified to do so.)

[Ryan Hamilton, 2016-02-08 22:12:05]

On 2016/02/08 16:51:09, xunjieli wrote:
> Paul, do you mind taking a look? Not sure what is the best action here. (I
> prefer not setting the QUIC UAID in experimental options at all if embedder
has
> not specified to do so.)

Since QUIC will soon be making the UAID mandatory, what should happen to an
embedder does not explicitly set a UAID? Should cronet "Blow up" in some way
since their QUIC configuration is invalid? Should cronet let them do so, and
rely on the server hanging up on them?

It sure seems to me like cronet should have a reasonable default and that
embedders should not need to think about the QUIC UAID unless they need to.

[xunjieli, 2016-02-08 22:17:16]

On 2016/02/08 22:12:05, Ryan Hamilton wrote:
> On 2016/02/08 16:51:09, xunjieli wrote:
> > Paul, do you mind taking a look? Not sure what is the best action here. (I
> > prefer not setting the QUIC UAID in experimental options at all if embedder
> has
> > not specified to do so.)
> 
> Since QUIC will soon be making the UAID mandatory, what should happen to an
> embedder does not explicitly set a UAID? Should cronet "Blow up" in some way
> since their QUIC configuration is invalid? Should cronet let them do so, and
> rely on the server hanging up on them?

If UAID will be sent regardless of user's consent, I think it should only be
sent when QUIC is enabled. And at least we should also document that fact in
UrlRequest.Builder#enableQUIC.

> It sure seems to me like cronet should have a reasonable default and that
> embedders should not need to think about the QUIC UAID unless they need to.

[Ryan Hamilton, 2016-02-08 22:37:55]

On 2016/02/08 22:17:16, xunjieli wrote:
> On 2016/02/08 22:12:05, Ryan Hamilton wrote:
> > On 2016/02/08 16:51:09, xunjieli wrote:
> > > Paul, do you mind taking a look? Not sure what is the best action here. (I
> > > prefer not setting the QUIC UAID in experimental options at all if
embedder
> > has
> > > not specified to do so.)
> > 
> > Since QUIC will soon be making the UAID mandatory, what should happen to an
> > embedder does not explicitly set a UAID? Should cronet "Blow up" in some way
> > since their QUIC configuration is invalid? Should cronet let them do so, and
> > rely on the server hanging up on them?
> 
> If UAID will be sent regardless of user's consent, I think it should only be
> sent when QUIC is enabled. And at least we should also document that fact in
> UrlRequest.Builder#enableQUIC.

It is sent in the QUIC CHLO message, which is only sent on QUIC connections. So
yes, it's only sent when QUIC is enabled, by definition. :> Documenting this
seems fine. What do you mean by "user's consent"? Does cronet provide a
mechanism for users (not app developers) to opt-out of sending the HTTP
User-Agent header?
  
> > It sure seems to me like cronet should have a reasonable default and that
> > embedders should not need to think about the QUIC UAID unless they need to.

[xunjieli, 2016-02-08 22:47:41]

On 2016/02/08 22:37:55, Ryan Hamilton wrote:
> On 2016/02/08 22:17:16, xunjieli wrote:
> > On 2016/02/08 22:12:05, Ryan Hamilton wrote:
> > > On 2016/02/08 16:51:09, xunjieli wrote:
> > > > Paul, do you mind taking a look? Not sure what is the best action here.
(I
> > > > prefer not setting the QUIC UAID in experimental options at all if
> embedder
> > > has
> > > > not specified to do so.)
> > > 
> > > Since QUIC will soon be making the UAID mandatory, what should happen to
an
> > > embedder does not explicitly set a UAID? Should cronet "Blow up" in some
way
> > > since their QUIC configuration is invalid? Should cronet let them do so,
and
> > > rely on the server hanging up on them?
> > 
> > If UAID will be sent regardless of user's consent, I think it should only be
> > sent when QUIC is enabled. And at least we should also document that fact in
> > UrlRequest.Builder#enableQUIC.
> 
> It is sent in the QUIC CHLO message, which is only sent on QUIC connections.
So
> yes, it's only sent when QUIC is enabled, by definition. :> Documenting this
> seems fine. What do you mean by "user's consent"? Does cronet provide a
> mechanism for users (not app developers) to opt-out of sending the HTTP
> User-Agent header?

I meant Cronet embedders, who are app developers. App developer can opt out of
sending HTTP User-Agent header (We have documentation and public API for it). My
concern is that with the current approach, app developers might not know a way
to opt out QUIC UAID (since it is hidden in undocumented SetExperimentalOptions
API). And it looks like UAID will become a standard, so I am fine with sending a
default value, but I think the fact that Cronet sends "app package name + cronet
version" by default should be documented somehow.

[mef, 2016-02-09 16:29:46]

On 2016/02/08 22:47:41, xunjieli wrote:
> On 2016/02/08 22:37:55, Ryan Hamilton wrote:
> > On 2016/02/08 22:17:16, xunjieli wrote:
> > > On 2016/02/08 22:12:05, Ryan Hamilton wrote:
> > > > On 2016/02/08 16:51:09, xunjieli wrote:
> > > > > Paul, do you mind taking a look? Not sure what is the best action
here.
> (I
> > > > > prefer not setting the QUIC UAID in experimental options at all if
> > embedder
> > > > has
> > > > > not specified to do so.)
> > > > 
> > > > Since QUIC will soon be making the UAID mandatory, what should happen to
> an
> > > > embedder does not explicitly set a UAID? Should cronet "Blow up" in some
> way
> > > > since their QUIC configuration is invalid? Should cronet let them do so,
> and
> > > > rely on the server hanging up on them?
> > > 
> > > If UAID will be sent regardless of user's consent, I think it should only
be
> > > sent when QUIC is enabled. And at least we should also document that fact
in
> > > UrlRequest.Builder#enableQUIC.
> > 
> > It is sent in the QUIC CHLO message, which is only sent on QUIC connections.
> So
> > yes, it's only sent when QUIC is enabled, by definition. :> Documenting this
> > seems fine. What do you mean by "user's consent"? Does cronet provide a
> > mechanism for users (not app developers) to opt-out of sending the HTTP
> > User-Agent header?
> 
> I meant Cronet embedders, who are app developers. App developer can opt out of
> sending HTTP User-Agent header (We have documentation and public API for it).
My
> concern is that with the current approach, app developers might not know a way
> to opt out QUIC UAID (since it is hidden in undocumented
SetExperimentalOptions
> API). And it looks like UAID will become a standard, so I am fine with sending
a
> default value, but I think the fact that Cronet sends "app package name +
cronet
> version" by default should be documented somehow.

PTAL.
We've discussed Helen's concerns offline, I've added some documentation to
enableQUIC() and don't pass down default QuicUserAgent ID unless QUIC is
enabled.

[xunjieli, 2016-02-09 16:52:58]

On 2016/02/09 16:29:46, mef wrote:
> On 2016/02/08 22:47:41, xunjieli wrote:
> > On 2016/02/08 22:37:55, Ryan Hamilton wrote:
> > > On 2016/02/08 22:17:16, xunjieli wrote:
> > > > On 2016/02/08 22:12:05, Ryan Hamilton wrote:
> > > > > On 2016/02/08 16:51:09, xunjieli wrote:
> > > > > > Paul, do you mind taking a look? Not sure what is the best action
> here.
> > (I
> > > > > > prefer not setting the QUIC UAID in experimental options at all if
> > > embedder
> > > > > has
> > > > > > not specified to do so.)
> > > > > 
> > > > > Since QUIC will soon be making the UAID mandatory, what should happen
to
> > an
> > > > > embedder does not explicitly set a UAID? Should cronet "Blow up" in
some
> > way
> > > > > since their QUIC configuration is invalid? Should cronet let them do
so,
> > and
> > > > > rely on the server hanging up on them?
> > > > 
> > > > If UAID will be sent regardless of user's consent, I think it should
only
> be
> > > > sent when QUIC is enabled. And at least we should also document that
fact
> in
> > > > UrlRequest.Builder#enableQUIC.
> > > 
> > > It is sent in the QUIC CHLO message, which is only sent on QUIC
connections.
> > So
> > > yes, it's only sent when QUIC is enabled, by definition. :> Documenting
this
> > > seems fine. What do you mean by "user's consent"? Does cronet provide a
> > > mechanism for users (not app developers) to opt-out of sending the HTTP
> > > User-Agent header?
> > 
> > I meant Cronet embedders, who are app developers. App developer can opt out
of
> > sending HTTP User-Agent header (We have documentation and public API for
it).
> My
> > concern is that with the current approach, app developers might not know a
way
> > to opt out QUIC UAID (since it is hidden in undocumented
> SetExperimentalOptions
> > API). And it looks like UAID will become a standard, so I am fine with
sending
> a
> > default value, but I think the fact that Cronet sends "app package name +
> cronet
> > version" by default should be documented somehow.
> 
> PTAL.
> We've discussed Helen's concerns offline, I've added some documentation to
> enableQUIC() and don't pass down default QuicUserAgent ID unless QUIC is
> enabled.

lgtm. thanks!

[mef, 2016-02-09 19:12:02]

The CQ bit was checked by mef@chromium.org

[mef, 2016-02-09 19:12:03]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1665503002/#ps160001
(title: "Document QUIC UAID string usage, don't pass it down unless QUIC is
enabled.")

[commit-bot: I haz the power, 2016-02-09 19:12:54]

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

[commit-bot: I haz the power, 2016-02-09 20:14:29]

Description was changed from

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

- QUIC will soon require that user agent id field is non-empty, but normal
User-Agent request header is too rich and verbose to be used for this purpose.
The app can override default value using 'user_agent_id' quic experimental
option.

- prefer_aes experimental option is needed to experiment with performance of
using AES instead of ChaCha20.

BUG=575771
==========

to

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

- QUIC will soon require that user agent id field is non-empty, but normal
User-Agent request header is too rich and verbose to be used for this purpose.
The app can override default value using 'user_agent_id' quic experimental
option.

- prefer_aes experimental option is needed to experiment with performance of
using AES instead of ChaCha20.

BUG=575771
==========

[commit-bot: I haz the power, 2016-02-09 20:14:30]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-09 20:15:42]

Description was changed from

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

- QUIC will soon require that user agent id field is non-empty, but normal
User-Agent request header is too rich and verbose to be used for this purpose.
The app can override default value using 'user_agent_id' quic experimental
option.

- prefer_aes experimental option is needed to experiment with performance of
using AES instead of ChaCha20.

BUG=575771
==========

to

==========
[Cronet] Expose quic_user_agent_id and quic_prefer_aes config options.

- QUIC will soon require that user agent id field is non-empty, but normal
User-Agent request header is too rich and verbose to be used for this purpose.
The app can override default value using 'user_agent_id' quic experimental
option.

- prefer_aes experimental option is needed to experiment with performance of
using AES instead of ChaCha20.

BUG=575771

Committed: https://crrev.com/c5da5713830e1c6ad09c75638e06f018396ca40d
Cr-Commit-Position: refs/heads/master@{#374438}
==========

[commit-bot: I haz the power, 2016-02-09 20:15:43]

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