Adding version info to the client config request

Adding version info to the client config request

This information will easily allow the server to use different
experiments in different channels and different versions.

BUG=622374
Committed: https://crrev.com/19f51e45ebccead8ad7df4b0bbaa66f783b506c8
Cr-Commit-Position: refs/heads/master@{#403207}

[RyanSturm, 2016-06-28 20:15:05]

ryansturm@chromium.org changed reviewers:
+ buettner@chromium.org, jpfeiff@chromium.org, tbansal@chromium.org

[RyanSturm, 2016-06-28 20:15:06]

tbansal: PTAL, let me know if you think I should test this code path (it seemed
unnecessary).

buettner, jpfeiff: PTAL @ the protobuf change

[buettner, 2016-06-28 20:52:01]

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
File components/data_reduction_proxy/proto/client_config.proto (right):

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/proto/client_config.proto:137: optional string
version_number = 2;
I think channel/build/patch is enough. And client_type defined here:
https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro...

wdyt?

[tbansal1, 2016-06-28 21:32:15]

https://codereview.chromium.org/2103223002/diff/60001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/2103223002/diff/60001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:21: #include
"components/version_info/version_info.h"
why is this include needed?

https://codereview.chromium.org/2103223002/diff/60001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:84:
data_reduction_proxy_io_data->set_channel(chrome::GetChannel());
Why not pass in the constructor? I think you can also change it to a string
because chrome::GetChannel() enum  is Chrome specific. Other embedders (e.g.,
Cronet) may use different enums, or may not use enums at all. String will
provide the flexibility to match up with all the embedders.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
File components/data_reduction_proxy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/BUILD.gn:64:
"//components/version_info",
Similar change in gyp files?

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:182:
version_info::Channel channel() { return channel_; }
const function

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:265:
version_info::Channel channel_;
set as const variable, and set it in costructor.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc
(right):

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:107:
return VersionInfo_Channel_UNKNOWN;
Add NOTREACHED() in default.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:124:
return version_info::Channel::UNKNOWN;
Add NOTREACHED() in default.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
File components/data_reduction_proxy/proto/client_config.proto (right):

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/proto/client_config.proto:121: // The channel is
unspecififed.
s/unspecififed/unspecified/

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/proto/client_config.proto:137: optional string
version_number = 2;
On 2016/06/28 20:52:01, buettner wrote:
> I think channel/build/patch is enough. And client_type defined here:
>
https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro...
> 
> wdyt?

OS may be important since some experiments may be OS specific.
product name and official build  are not required now.

[RyanSturm, 2016-06-28 22:22:14]

https://codereview.chromium.org/2103223002/diff/60001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/2103223002/diff/60001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:21: #include
"components/version_info/version_info.h"
On 2016/06/28 21:32:15, tbansal1 wrote:
> why is this include needed?

Done.

https://codereview.chromium.org/2103223002/diff/60001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:84:
data_reduction_proxy_io_data->set_channel(chrome::GetChannel());
On 2016/06/28 21:32:15, tbansal1 wrote:
> Why not pass in the constructor? I think you can also change it to a string
> because chrome::GetChannel() enum  is Chrome specific. Other embedders (e.g.,
> Cronet) may use different enums, or may not use enums at all. String will
> provide the flexibility to match up with all the embedders.

Done.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
File components/data_reduction_proxy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/BUILD.gn:64:
"//components/version_info",
On 2016/06/28 21:32:15, tbansal1 wrote:
> Similar change in gyp files?

Acknowledged.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:182:
version_info::Channel channel() { return channel_; }
On 2016/06/28 21:32:15, tbansal1 wrote:
> const function

Done.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:265:
version_info::Channel channel_;
On 2016/06/28 21:32:15, tbansal1 wrote:
> set as const variable, and set it in costructor.

Done.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc
(right):

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:107:
return VersionInfo_Channel_UNKNOWN;
On 2016/06/28 21:32:15, tbansal1 wrote:
> Add NOTREACHED() in default.

Acknowledged.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:124:
return version_info::Channel::UNKNOWN;
On 2016/06/28 21:32:15, tbansal1 wrote:
> Add NOTREACHED() in default.

Acknowledged.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
File components/data_reduction_proxy/proto/client_config.proto (right):

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/proto/client_config.proto:121: // The channel is
unspecififed.
On 2016/06/28 21:32:15, tbansal1 wrote:
> s/unspecififed/unspecified/

Acknowledged.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/proto/client_config.proto:137: optional string
version_number = 2;
On 2016/06/28 20:52:01, buettner wrote:
> I think channel/build/patch is enough. And client_type defined here:
>
https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro...
> 
> wdyt?

Done.

https://codereview.chromium.org/2103223002/diff/60001/components/data_reducti...
components/data_reduction_proxy/proto/client_config.proto:137: optional string
version_number = 2;
On 2016/06/28 21:32:15, tbansal1 wrote:
> On 2016/06/28 20:52:01, buettner wrote:
> > I think channel/build/patch is enough. And client_type defined here:
> >
>
https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro...
> > 
> > wdyt?
> 
> OS may be important since some experiments may be OS specific.
> product name and official build  are not required now.

Done.

[buettner, 2016-06-28 22:39:38]

https://codereview.chromium.org/2103223002/diff/80001/components/data_reducti...
File components/data_reduction_proxy/proto/client_config.proto (right):

https://codereview.chromium.org/2103223002/diff/80001/components/data_reducti...
components/data_reduction_proxy/proto/client_config.proto:119: // The client's
platform type.
// The client type.

Not sure if this is Chromium style, but can you add a comment pointing to the
list of client strings and mention that it should be one of those? Ideally that
would be an enforced constraint, but a comment is better than nothing.

[RyanSturm, 2016-06-28 22:40:25]

https://codereview.chromium.org/2103223002/diff/80001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.h
(right):

https://codereview.chromium.org/2103223002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.h:55: void
GetChromiumBuildAndPatchAsInts(const std::string& version_string,
Add comment

[RyanSturm, 2016-06-28 22:53:35]

On 2016/06/28 22:40:25, RyanSturm wrote:
>
https://codereview.chromium.org/2103223002/diff/80001/components/data_reducti...
> File components/data_reduction_proxy/core/common/data_reduction_proxy_util.h
> (right):
> 
>
https://codereview.chromium.org/2103223002/diff/80001/components/data_reducti...
> components/data_reduction_proxy/core/common/data_reduction_proxy_util.h:55:
void
> GetChromiumBuildAndPatchAsInts(const std::string& version_string,
> Add comment

Done.

[RyanSturm, 2016-06-28 22:56:43]

On 2016/06/28 22:39:38, buettner wrote:
>
https://codereview.chromium.org/2103223002/diff/80001/components/data_reducti...
> File components/data_reduction_proxy/proto/client_config.proto (right):
> 
>
https://codereview.chromium.org/2103223002/diff/80001/components/data_reducti...
> components/data_reduction_proxy/proto/client_config.proto:119: // The client's
> platform type.
> // The client type.
> 
> Not sure if this is Chromium style, but can you add a comment pointing to the
> list of client strings and mention that it should be one of those? Ideally
that
> would be an enforced constraint, but a comment is better than nothing.

Sounds like it should be fine to do that.

[tbansal1, 2016-06-29 17:58:26]

https://codereview.chromium.org/2103223002/diff/80001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/2103223002/diff/80001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:216: false /* enable */,
GetUserAgent(), std::string()));
Fix cronet too :)

https://codereview.chromium.org/2103223002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
(right):

https://codereview.chromium.org/2103223002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:381:
version_info->set_channel(io_data_->channel());
What are these variables set to in tests? Is it possible to write any test
around these?

https://codereview.chromium.org/2103223002/diff/120001/components/data_reduct...
File components/data_reduction_proxy/proto/client_config.proto (right):

https://codereview.chromium.org/2103223002/diff/120001/components/data_reduct...
components/data_reduction_proxy/proto/client_config.proto:124: // The version
number, e.g. "6.0.490.1".
I do not understand this comment. From the example, the version number seems
like a string, but it is actually an int32.

[buettner, 2016-06-29 18:04:44]

https://codereview.chromium.org/2103223002/diff/120001/components/data_reduct...
File components/data_reduction_proxy/proto/client_config.proto (right):

https://codereview.chromium.org/2103223002/diff/120001/components/data_reduct...
components/data_reduction_proxy/proto/client_config.proto:130: // The production
channel.
Maybe it's clear enough to Chromium developers, but you may want to add, "E.g.,
Dev, Beta, Stable, etc."

[RyanSturm, 2016-06-29 19:02:36]

https://codereview.chromium.org/2103223002/diff/80001/components/data_reducti...
File components/data_reduction_proxy/proto/client_config.proto (right):

https://codereview.chromium.org/2103223002/diff/80001/components/data_reducti...
components/data_reduction_proxy/proto/client_config.proto:119: // The client's
platform type.
On 2016/06/28 22:39:38, buettner wrote:
> // The client type.
> 
> Not sure if this is Chromium style, but can you add a comment pointing to the
> list of client strings and mention that it should be one of those? Ideally
that
> would be an enforced constraint, but a comment is better than nothing.

Done.

https://codereview.chromium.org/2103223002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
(right):

https://codereview.chromium.org/2103223002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:381:
version_info->set_channel(io_data_->channel());
On 2016/06/29 17:58:26, tbansal1 wrote:
> What are these variables set to in tests? Is it possible to write any test
> around these?

I didn't think it was worth testing these. If you have a particularly good
testing strategy let me know. (By default, channel is std::string() and client
is UNKNOWN).

https://codereview.chromium.org/2103223002/diff/120001/components/data_reduct...
File components/data_reduction_proxy/proto/client_config.proto (right):

https://codereview.chromium.org/2103223002/diff/120001/components/data_reduct...
components/data_reduction_proxy/proto/client_config.proto:124: // The version
number, e.g. "6.0.490.1".
On 2016/06/29 17:58:26, tbansal1 wrote:
> I do not understand this comment. From the example, the version number seems
> like a string, but it is actually an int32.

Done.

https://codereview.chromium.org/2103223002/diff/120001/components/data_reduct...
components/data_reduction_proxy/proto/client_config.proto:130: // The production
channel.
On 2016/06/29 18:04:44, buettner wrote:
> Maybe it's clear enough to Chromium developers, but you may want to add,
"E.g.,
> Dev, Beta, Stable, etc."

Done.

[RyanSturm, 2016-06-29 23:52:59]

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

[RyanSturm, 2016-06-29 23:53:00]

xunjieli: PTAL @ cronet file

[tbansal1, 2016-06-29 23:56:38]

lgtm % nits below.

https://codereview.chromium.org/2103223002/diff/160001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/2103223002/diff/160001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:216:
Client client_;
change it to const.

https://codereview.chromium.org/2103223002/diff/160001/components/data_reduct...
File components/data_reduction_proxy/proto/client_config.proto (right):

https://codereview.chromium.org/2103223002/diff/160001/components/data_reduct...
components/data_reduction_proxy/proto/client_config.proto:124: // The version
number, e.g. 50
This comment is slightly confusing. From the value "50", it seems you are
returning milestone, but you are returning the build number, which is generally
a 4-digit number. 
s/version/build/
s/50/2171/

https://codereview.chromium.org/2103223002/diff/160001/components/data_reduct...
components/data_reduction_proxy/proto/client_config.proto:130: // The production
channel, e.g. Canary, Dev, Beta, Stable.
s/Canary/canary/
s/Dev/dev/
....
assuming I am reading
https://cs.chromium.org/chromium/src/components/version_info/version_info.cc?...
correctly.

[RyanSturm, 2016-06-30 00:02:22]

https://codereview.chromium.org/2103223002/diff/160001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/2103223002/diff/160001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:216:
Client client_;
On 2016/06/29 23:56:38, tbansal1 wrote:
> change it to const.

Done.

https://codereview.chromium.org/2103223002/diff/160001/components/data_reduct...
File components/data_reduction_proxy/proto/client_config.proto (right):

https://codereview.chromium.org/2103223002/diff/160001/components/data_reduct...
components/data_reduction_proxy/proto/client_config.proto:124: // The version
number, e.g. 50
On 2016/06/29 23:56:38, tbansal1 wrote:
> This comment is slightly confusing. From the value "50", it seems you are
> returning milestone, but you are returning the build number, which is
generally
> a 4-digit number. 
> s/version/build/
> s/50/2171/

Done.

https://codereview.chromium.org/2103223002/diff/160001/components/data_reduct...
components/data_reduction_proxy/proto/client_config.proto:130: // The production
channel, e.g. Canary, Dev, Beta, Stable.
On 2016/06/29 23:56:38, tbansal1 wrote:
> s/Canary/canary/
> s/Dev/dev/
> ....
> assuming I am reading
>
https://cs.chromium.org/chromium/src/components/version_info/version_info.cc?...
> correctly.

Done.

[xunjieli, 2016-06-30 13:55:37]

https://codereview.chromium.org/2103223002/diff/200001/components/cronet/andr...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/2103223002/diff/200001/components/cronet/andr...
components/cronet/android/cronet_data_reduction_proxy.cc:95: net_log,
task_runner, task_runner, false, user_agent, std::string()));
You can get a Chromium version string CRONET_VERSION from
components/cronet/version.h. Will that suit this purpose?

[RyanSturm, 2016-06-30 15:09:12]

https://codereview.chromium.org/2103223002/diff/200001/components/cronet/andr...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/2103223002/diff/200001/components/cronet/andr...
components/cronet/android/cronet_data_reduction_proxy.cc:95: net_log,
task_runner, task_runner, false, user_agent, std::string()));
On 2016/06/30 13:55:37, xunjieli wrote:
> You can get a Chromium version string CRONET_VERSION from
> components/cronet/version.h. Will that suit this purpose?

This string is for channel (dev, beta, stable). I don't think cronet has
channels, so an empty string is fine here. The version should match the version
of chrome, which we get via PRODUCT_VERSION which is defined as @VERSION_FULL@.

[xunjieli, 2016-06-30 15:12:54]

components/cronet/android/cronet_data_reduction_proxy.cc lgtm

[buettner, 2016-06-30 17:14:46]

On 2016/06/30 15:12:54, xunjieli wrote:
> components/cronet/android/cronet_data_reduction_proxy.cc lgtm

components/data_reduction_proxy/proto/client_config.proto lgtm

[RyanSturm, 2016-06-30 17:15:59]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-06-30 17:16:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2103223002/#ps200001
(title: "nits")

[commit-bot: I haz the power, 2016-06-30 17:16:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 17:22:35]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-30 17:22:50]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 17:24:24]

Description was changed from

==========
Adding version info to the client config request

This information will easily allow the server to use different
experiments in different channels and different versions.

BUG=622374
==========

to

==========
Adding version info to the client config request

This information will easily allow the server to use different
experiments in different channels and different versions.

BUG=622374

Committed: https://crrev.com/19f51e45ebccead8ad7df4b0bbaa66f783b506c8
Cr-Commit-Position: refs/heads/master@{#403207}
==========

[commit-bot: I haz the power, 2016-06-30 17:24:26]

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