Add build and patch to Chrome-Proxy header

Chrome-Proxy will now have three directives relevant to versioning:
"b=" -- the build number
"p=" -- the patch number
"c=" -- the client type (current values are "ios", "android", and "webview")

Currently, b and p only make sense for chrome types and are only being implemented for ios and android.

BUG=409934
Committed: https://crrev.com/12a31fdc4f1d7abebb7845628dcd86a79d01e9b2
Cr-Commit-Position: refs/heads/master@{#293591}

[megjablon, 2014-09-02 20:35:07]

megjablon@chromium.org changed reviewers:
+ bengr@chromium.org

[bengr, 2014-09-02 22:57:53]

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
(right):

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:68: return
"";
return std::string();

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h
(right):

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:57: static
std::string GetBuildAndPatchNumberFromString(std::string version);
This probably shouldn't be public. Make private and friend the test.

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc
(right):

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc:17:
TEST_F(DataReductionProxyChromeSettingsTest, TestGetBuildAndPatchNumber) {
For completeness add a test that verifies that a bogus version returns
std::string();

[megjablon, 2014-09-02 23:11:28]

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
(right):

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:68: return
"";
On 2014/09/02 22:57:53, bengr1 wrote:
> return std::string();

Done.

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h
(right):

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:57: static
std::string GetBuildAndPatchNumberFromString(std::string version);
On 2014/09/02 22:57:53, bengr1 wrote:
> This probably shouldn't be public. Make private and friend the test.

Done.

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc
(right):

https://codereview.chromium.org/530153002/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc:17:
TEST_F(DataReductionProxyChromeSettingsTest, TestGetBuildAndPatchNumber) {
On 2014/09/02 22:57:53, bengr1 wrote:
> For completeness add a test that verifies that a bogus version returns
> std::string();

Done.

[bengr, 2014-09-02 23:16:14]

lgtm, with nit.

https://codereview.chromium.org/530153002/diff/20001/chrome/browser/net/spdyp...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc
(right):

https://codereview.chromium.org/530153002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc:25:
"0.0");
nit: define kBogusVersion[] = "0.0"

[megjablon, 2014-09-03 03:42:44]

Patchset #5 (id:80001) has been deleted

[megjablon, 2014-09-03 17:00:30]

Patchset #5 (id:100001) has been deleted

[megjablon, 2014-09-03 17:32:55]

Patchset #6 (id:140001) has been deleted

[megjablon, 2014-09-03 17:39:27]

Patchset #5 (id:120001) has been deleted

[bengr, 2014-09-03 22:28:39]

NOT lgtm

https://codereview.chromium.org/530153002/diff/180001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
(right):

https://codereview.chromium.org/530153002/diff/180001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:63:
std::string DataReductionProxyChromeSettings::GetBuildNumberFromString(
How often is this called? If once, I'm ok with the structure. If it's on every
request, then to avoid copies, do bool GetBuildNumberFromString(std::string*
build_number);

https://codereview.chromium.org/530153002/diff/180001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:73:
std::string DataReductionProxyChromeSettings::GetPatchNumber() {
To avoid repetition, I'd have one function called void
GetChromiumVersionComponent(const std::string& version, size_t component_index);

Then have GetBuildNumberFromString call
GetChromiumVersionComponent(version_info.Version(), 2) and have
GetPatchNumberFromString call
GetChromiumVersionComponent(version_info.Version(), 3)

https://codereview.chromium.org/530153002/diff/180001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc
(right):

https://codereview.chromium.org/530153002/diff/180001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc:31:
DataReductionProxyChromeSettings::GetPatchNumberFromString(kVersion);
Fix these tests to reflect the new impl.

https://codereview.chromium.org/530153002/diff/180001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h
(right):

https://codereview.chromium.org/530153002/diff/180001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:43:
const std::string& build_number,
Why can't this just take a version and do the splitting internally?

[megjablon, 2014-09-04 00:41:07]

https://codereview.chromium.org/530153002/diff/180001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
(right):

https://codereview.chromium.org/530153002/diff/180001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:63:
std::string DataReductionProxyChromeSettings::GetBuildNumberFromString(
On 2014/09/03 22:28:38, bengr1 wrote:
> How often is this called? If once, I'm ok with the structure. If it's on every
> request, then to avoid copies, do bool GetBuildNumberFromString(std::string*
> build_number);

Only once

[megjablon, 2014-09-04 19:06:37]

megjablon@chromium.org changed reviewers:
+ mmenke@chromium.org

[megjablon, 2014-09-04 19:06:38]

Adding mmenke for iothread.cc and profile_impl_io_data.cc

[mmenke, 2014-09-04 19:12:10]

profile_impl_io_data, io_thread LGTM

https://codereview.chromium.org/530153002/diff/200001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/530153002/diff/200001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:664: version_info.Version(),
nit:  You can just use "chrome::VersionInfo().Version()"

https://codereview.chromium.org/530153002/diff/200001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/530153002/diff/200001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:438: version_info.Version(),
Per comment in other file, you don't need the local.

[megjablon, 2014-09-04 19:16:36]

https://codereview.chromium.org/530153002/diff/200001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/530153002/diff/200001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:664: version_info.Version(),
On 2014/09/04 19:12:10, mmenke wrote:
> nit:  You can just use "chrome::VersionInfo().Version()"

Done.

https://codereview.chromium.org/530153002/diff/200001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/530153002/diff/200001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:438: version_info.Version(),
On 2014/09/04 19:12:10, mmenke wrote:
> Per comment in other file, you don't need the local.

Done.

[bengr, 2014-09-04 19:22:52]

https://codereview.chromium.org/530153002/diff/200001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc
(right):

https://codereview.chromium.org/530153002/diff/200001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:29:
const char kAndroidWebViewProtocolVersion[] = "";
Add:
// TODO(bengr): Remove when this class determines the version.

https://codereview.chromium.org/530153002/diff/200001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:53:
patch_number_ = GetChromiumVersionComponent(version, 3);
Since you are doing the same work twice, why not have this function:

void GetChromiumBuildAndPatchNumbers(const std::string& version,
                                     std::string* build,
                                     std::string* patch) {
  // ... split the version, check that build and patch are non NULL
  *build = version_parts[2];
  *patch = version_parts[3];
}

https://codereview.chromium.org/530153002/diff/200001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:58:
std::string version, int index) {
use const std::string version&

https://codereview.chromium.org/530153002/diff/200001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:128:
header_value += ", b=" + build_number_ + ", p=" + patch_number_;
indent only 2 from the 'if'

[megjablon, 2014-09-04 20:30:35]

https://codereview.chromium.org/530153002/diff/200001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc
(right):

https://codereview.chromium.org/530153002/diff/200001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:53:
patch_number_ = GetChromiumVersionComponent(version, 3);
On 2014/09/04 19:22:52, bengr1 wrote:
> Since you are doing the same work twice, why not have this function:
> 
> void GetChromiumBuildAndPatchNumbers(const std::string& version,
>                                      std::string* build,
>                                      std::string* patch) {
>   // ... split the version, check that build and patch are non NULL
>   *build = version_parts[2];
>   *patch = version_parts[3];
> }

Done.

https://codereview.chromium.org/530153002/diff/200001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:58:
std::string version, int index) {
On 2014/09/04 19:22:52, bengr1 wrote:
> use const std::string version&

Done.

[bengr, 2014-09-05 03:31:20]

Lgtm with two nits.  Change GetChromiumVersionComponent to
GetChromiumBuildAndPatch and update the CL title to say "Add build and Patch to
Chrome-Proxy header" and then describe only what's implemented not what's
planned.

[megjablon, 2014-09-05 17:16:06]

Patchset #10 (id:260001) has been deleted

[megjablon, 2014-09-05 20:19:29]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2014-09-05 20:47:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/megjablon@chromium.org/530153002/280001

[commit-bot: I haz the power, 2014-09-05 23:08:24]

Committed patchset #10 (id:280001) as 799837530e6e9e9e0b2e7e2a4d40de1d683b5229

[commit-bot: I haz the power, 2014-09-10 03:41:51]

Patchset 10 (id:??) landed as
https://crrev.com/12a31fdc4f1d7abebb7845628dcd86a79d01e9b2
Cr-Commit-Position: refs/heads/master@{#293591}