[Sync] Finch Experiment : Enable compression between sync client and server

Enable compress HTTP call's content between sync client and
sync server.

BUG=509728
Committed: https://crrev.com/fbf1a591aa9653333a748764c1465344b7782fc8
Cr-Commit-Position: refs/heads/master@{#342213}

[Gang Wu, 2015-07-17 23:32:51]

Patchset #1 (id:1) has been deleted

[Gang Wu, 2015-07-24 06:14:54]

Patchset #8 (id:160001) has been deleted

[Gang Wu, 2015-07-24 06:15:34]

Patchset #6 (id:120001) has been deleted

[Gang Wu, 2015-07-24 22:58:35]

Patchset #7 (id:180001) has been deleted

[Gang Wu, 2015-07-24 22:58:43]

Patchset #7 (id:200001) has been deleted

[Gang Wu, 2015-07-24 22:58:56]

Patchset #7 (id:220001) has been deleted

[Gang Wu, 2015-07-24 23:34:03]

gangwu@chromium.org changed reviewers:
+ asvitkine@chromium.org, zea@chromium.org

[Gang Wu, 2015-07-24 23:34:47]

Patchset #6 (id:140001) has been deleted

[Gang Wu, 2015-07-24 23:37:40]

Hi Nicolas/Alexei,
Please take a look.

Alexei,
Please review both histogram.xml and testing/variations/*.

[Gang Wu, 2015-07-25 05:46:14]

Patchset #7 (id:260001) has been deleted

[Alexei Svitkine (slow), 2015-07-25 15:50:36]

https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:47: bool GZipCompress(std::string source,
std::string* dest) {
FYI: We have something similar here:

https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

I wonder if we could re-use it somehow. Or at the very least to add a TODO to do
it at the time when this stops being an experiment.

https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:378: compressed_content_length =
fetch_state_.response_headers->GetContentLength();
Nit: Line length

https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
File sync/internal_api/http_bridge_unittest.cc (right):

https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:29: bool GzipUncompress(const char*
input, int length, std::string* output) {
Ditto here:

https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

[Gang Wu, 2015-07-27 17:52:30]

Patchset #9 (id:320001) has been deleted

[Gang Wu, 2015-07-27 21:07:08]

Patchset #10 (id:360001) has been deleted

[Gang Wu, 2015-07-27 22:35:06]

https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:47: bool GZipCompress(std::string source,
std::string* dest) {
On 2015/07/25 15:50:36, Alexei Svitkine wrote:
> FYI: We have something similar here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...
> 
> I wonder if we could re-use it somehow. Or at the very least to add a TODO to
do
> it at the time when this stops being an experiment.

Thanks for info, I tried change to use metrics::GzipCompress, but got warning at
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...


Cycle: /b/build/slave/ios_rel_device_ninja/build/src/components/components.gyp
-> /b/build/slave/ios_rel_device_ninja/build/src/sync/sync.gyp ->
/b/build/slave/ios_rel_device_ninja/build/src/components/components.gyp

so I write our own function, actually, I like to use metrics's function, but do
you know how to avoid this cycle dependency?

https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:378: compressed_content_length =
fetch_state_.response_headers->GetContentLength();
On 2015/07/25 15:50:36, Alexei Svitkine wrote:
> Nit: Line length

Done.

https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
File sync/internal_api/http_bridge_unittest.cc (right):

https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:29: bool GzipUncompress(const char*
input, int length, std::string* output) {
On 2015/07/25 15:50:36, Alexei Svitkine wrote:
> Ditto here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

same as previous one.

[Nicolas Zea, 2015-07-28 00:09:00]

On 2015/07/27 22:35:06, Gang Wu wrote:
>
https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
> File sync/internal_api/http_bridge.cc (right):
> 
>
https://codereview.chromium.org/1246523003/diff/280001/sync/internal_api/http...
> sync/internal_api/http_bridge.cc:47: bool GZipCompress(std::string source,
> std::string* dest) {
> On 2015/07/25 15:50:36, Alexei Svitkine wrote:
> > FYI: We have something similar here:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...
> > 
> > I wonder if we could re-use it somehow. Or at the very least to add a TODO
to
> do
> > it at the time when this stops being an experiment.
> 
> Thanks for info, I tried change to use metrics::GzipCompress, but got warning
at
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...
> 
> 
> Cycle: /b/build/slave/ios_rel_device_ninja/build/src/components/components.gyp
> -> /b/build/slave/ios_rel_device_ninja/build/src/sync/sync.gyp ->
> /b/build/slave/ios_rel_device_ninja/build/src/components/components.gyp
> 
> so I write our own function, actually, I like to use metrics's function, but
do
> you know how to avoid this cycle dependency?

The compression code would need to live outside of components/ to be usable by
sync/. There's possibly an argument to be made that it should live in base/ or
more likely net/. That said, that might take some time to get right and convince
others. In the meantime I'd suggest adding a comment explaining where the code
comes from (with TODO) and filing a bug to share the code.

[Nicolas Zea, 2015-07-28 00:27:19]

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:76: void
RecordSyncContentLengthHistograms(int64 this_content_length,
nit: I think these variables could be named clearer. How about
compressed_content_length and original_content_length?

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:78: // Upload UMA data
no need for this comment (all UMA histograms result in uploading data
eventually. And to be accurate, this doesn't actually immediately result in the
upload, but simply records the data for later upload).

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:257: std::string content_to_be_sent =
request_content_;
I'd rather not pay the cost of a copy on all requests, even when we don't have
the feature enabled. How about pulling the SetUploadData call into the if
statements, and only declaring this variable within the if (IsSync...) body?

It would be something like:
int compressed_content_size = 0;
if (IsSyncHttp...) {
  std::string compress_content;
  GzipCompress(...);
  compressed_content_size = ...
  ...AddExtraRequestHeader...
  ...SetUploadData...)
} else {
  ...
}
RecordSyncContentLengthHistogram(...);

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:381:
RecordSyncContentLengthHistograms(compressed_content_length,
Does it make sense to record the request separately from the response (in
different histograms)?

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
File sync/internal_api/http_bridge_unittest.cc (right):

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:29: bool GzipUncompress(const char*
input, int length, std::string* output) {
Comment where this code comes from?

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:272: // Full round-trip test of the
HttpBridge with compress data, using default
nit: compress -> compressed

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:274: TEST_F(SyncHttpBridgeTest,
TestMakeSynchronousPostLiveWithCompressedPayload) {
nit: I think it would make a bit more sense to order these two tests below
TestMakeSynchronousPostLiveComprehensive, as these both seemed to be based on
that test.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:296: std::string
uncompressed_payload(payload.size() * 10, ' ');
Why initialize it to so many spaces?

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:300: EXPECT_EQ(payload.length() + 1,
uncompressed_payload.size());
This check is redundant given the one below.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:301: EXPECT_EQ(payload + '\0',
uncompressed_payload);
Do all http responses include the null character at the end, and we just hadn't
been checking for that?

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:306:
TestMakeSynchronousPostLiveComprehensiveWithCompression) {
How is this test different from the one above? The comment seems to imply
they're basically the same?

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/publ...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:25: #include "third_party/zlib/zlib.h"
move to .cc file?

[Alexei Svitkine (slow), 2015-07-28 15:20:18]

If we go with the TODO route for now for the duplicated code, I'd at least make
the two functions identical to the ones in components/metrics, so it's easier to
de-dupe.

[Gang Wu, 2015-07-28 19:56:07]

Patchset #11 (id:400001) has been deleted

[Gang Wu, 2015-07-28 22:11:07]

Patchset #12 (id:440001) has been deleted

[Gang Wu, 2015-07-28 22:29:37]

Patchset #12 (id:180002) has been deleted

[Gang Wu, 2015-07-28 22:46:03]

Patchset #12 (id:470001) has been deleted

[Gang Wu, 2015-07-28 23:12:04]

Patchset #12 (id:480001) has been deleted

[Gang Wu, 2015-07-28 23:58:26]

Patchset #12 (id:500001) has been deleted

[Gang Wu, 2015-07-29 21:09:16]

updated, please take a look.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:76: void
RecordSyncContentLengthHistograms(int64 this_content_length,
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> nit: I think these variables could be named clearer. How about
> compressed_content_length and original_content_length?

Done.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:78: // Upload UMA data
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> no need for this comment (all UMA histograms result in uploading data
> eventually. And to be accurate, this doesn't actually immediately result in
the
> upload, but simply records the data for later upload).

Done.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:257: std::string content_to_be_sent =
request_content_;
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> I'd rather not pay the cost of a copy on all requests, even when we don't have
> the feature enabled. How about pulling the SetUploadData call into the if
> statements, and only declaring this variable within the if (IsSync...) body?
> 
> It would be something like:
> int compressed_content_size = 0;
> if (IsSyncHttp...) {
>   std::string compress_content;
>   GzipCompress(...);
>   compressed_content_size = ...
>   ...AddExtraRequestHeader...
>   ...SetUploadData...)
> } else {
>   ...
> }
> RecordSyncContentLengthHistogram(...);

Done.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:381:
RecordSyncContentLengthHistograms(compressed_content_length,
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> Does it make sense to record the request separately from the response (in
> different histograms)?

Yes, since request is smaller than response, so separate then is better.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
File sync/internal_api/http_bridge_unittest.cc (right):

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:29: bool GzipUncompress(const char*
input, int length, std::string* output) {
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> Comment where this code comes from?

Done.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:272: // Full round-trip test of the
HttpBridge with compress data, using default
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> nit: compress -> compressed

Done.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:274: TEST_F(SyncHttpBridgeTest,
TestMakeSynchronousPostLiveWithCompressedPayload) {
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> nit: I think it would make a bit more sense to order these two tests below
> TestMakeSynchronousPostLiveComprehensive, as these both seemed to be based on
> that test.

rename two tests to make it easier to understand, one test is for payload check,
another is for header check.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:296: std::string
uncompressed_payload(payload.size() * 10, ' ');
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> Why initialize it to so many spaces?

fixed.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:300: EXPECT_EQ(payload.length() + 1,
uncompressed_payload.size());
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> This check is redundant given the one below.

Done.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:301: EXPECT_EQ(payload + '\0',
uncompressed_payload);
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> Do all http responses include the null character at the end, and we just
hadn't
> been checking for that?

it is because in lines above, sent payload by
http_bridge->SetPostPayload("application/x-www-form-urlencoded",
                             payload.length() + 1, payload.c_str());
so here is an extra null at the end.
removed "+ 1" for extra null.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:306:
TestMakeSynchronousPostLiveComprehensiveWithCompression) {
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> How is this test different from the one above? The comment seems to imply
> they're basically the same?

rename both test name.

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/publ...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:25: #include "third_party/zlib/zlib.h"
On 2015/07/28 00:27:19, Nicolas Zea wrote:
> move to .cc file?

When I put them in .cc file, I got dependecy err.

Dependency chain (there may also be others):
  //sync:sync_unit_tests -->
  //net:net --[private]-->
  //third_party/zlib:zlib
on http_bridge_unittest.cc file, so moved to here.

[Gang Wu, 2015-07-29 21:09:26]

Patchset #12 (id:520001) has been deleted

[Nicolas Zea, 2015-07-30 19:59:44]

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/publ...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:25: #include "third_party/zlib/zlib.h"
On 2015/07/29 21:09:16, Gang Wu wrote:
> On 2015/07/28 00:27:19, Nicolas Zea wrote:
> > move to .cc file?
> 
> When I put them in .cc file, I got dependecy err.
> 
> Dependency chain (there may also be others):
>   //sync:sync_unit_tests -->
>   //net:net --[private]-->
>   //third_party/zlib:zlib
> on http_bridge_unittest.cc file, so moved to here.

Does that just mean you need to also include it in the unit test cc? This
doesn't need to be included in this header.

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:45: // The difference in bytes between a zlib
header and a gzip header.
Add a comment that explains that makes it clear which of these are copied from
compression_utils.cc. To do that, it might be useful to move the
RecordXXHistogram methods here, and then say something like:
-----------
The rest of the code in the anon namespace is copied from
components/metrics/compression_utils.cc
-----------

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:373: int compressed_content_size = 0;
You're using an int here, but the RequestXXXMethods take int64s, and std::string
actually returns a size_t. Which is correct?

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:494: int64 compressed_content_length =
fetch_state_.response_content.size();
Same comment above about type consistency/correctness.

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
File sync/internal_api/http_bridge_unittest.cc (right):

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:241: // UA string and no request
cookies.
Do the UA string or request cookies actually matter to this test?

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:271: // Full round-trip test of the
HttpBridge with compression.
Update comment to explain what is being tested w.r.t. to the header?

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/publ...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:43: // TODO: This two functions are
copied from
TODO's should have an owner, and ideally reference a bug. So: TODO(gangwu):
crbug.com/XXXX ....

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:45: // reference them, we will get cycle
dependency warning. so once the functions
nit: ". so once the" -> ". Once the"

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:51: SYNC_EXPORT_PRIVATE bool
GzipUncompress(const std::string& input,
Given this isn't actually used in HttpBridge, and only in tests, I'd rather it
live in the test itself (with the same comment about the TODO above)

https://codereview.chromium.org/1246523003/diff/540001/sync/tools/testserver/...
File sync/tools/testserver/sync_testserver.py (right):

https://codereview.chromium.org/1246523003/diff/540001/sync/tools/testserver/...
sync/tools/testserver/sync_testserver.py:202: enocde =
self.headers.getheader('Content-Encoding')
enocde -> encode

[Gang Wu, 2015-07-31 00:39:45]

updated base on comments

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/publ...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/1246523003/diff/380001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:25: #include "third_party/zlib/zlib.h"
On 2015/07/30 19:59:44, Nicolas Zea wrote:
> On 2015/07/29 21:09:16, Gang Wu wrote:
> > On 2015/07/28 00:27:19, Nicolas Zea wrote:
> > > move to .cc file?
> > 
> > When I put them in .cc file, I got dependecy err.
> > 
> > Dependency chain (there may also be others):
> >   //sync:sync_unit_tests -->
> >   //net:net --[private]-->
> >   //third_party/zlib:zlib
> > on http_bridge_unittest.cc file, so moved to here.
> 
> Does that just mean you need to also include it in the unit test cc? This
> doesn't need to be included in this header.

Yes, since I remove unzip function from unit test cc file, now I can move it to
http_bridge.cc file.

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:45: // The difference in bytes between a zlib
header and a gzip header.
On 2015/07/30 19:59:44, Nicolas Zea wrote:
> Add a comment that explains that makes it clear which of these are copied from
> compression_utils.cc. To do that, it might be useful to move the
> RecordXXHistogram methods here, and then say something like:
> -----------
> The rest of the code in the anon namespace is copied from
> components/metrics/compression_utils.cc
> -----------

Done.

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:373: int compressed_content_size = 0;
On 2015/07/30 19:59:44, Nicolas Zea wrote:
> You're using an int here, but the RequestXXXMethods take int64s, and
std::string
> actually returns a size_t. Which is correct?

int64, since GetContentLength() return int64 it might return -1.
it might have problem from size_t -> int64, but since
HttpBridge::GetResponseContentLength did same thing(size_t -> int), and HTTP
will not send such a long content(will chunk it), so there is no risk to convert
from size_t to int64.

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:494: int64 compressed_content_length =
fetch_state_.response_content.size();
On 2015/07/30 19:59:44, Nicolas Zea wrote:
> Same comment above about type consistency/correctness.

Done.

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
File sync/internal_api/http_bridge_unittest.cc (right):

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:241: // UA string and no request
cookies.
On 2015/07/30 19:59:44, Nicolas Zea wrote:
> Do the UA string or request cookies actually matter to this test?

comment updated.

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:271: // Full round-trip test of the
HttpBridge with compression.
On 2015/07/30 19:59:44, Nicolas Zea wrote:
> Update comment to explain what is being tested w.r.t. to the header?

Done.

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/publ...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:43: // TODO: This two functions are
copied from
On 2015/07/30 19:59:44, Nicolas Zea wrote:
> TODO's should have an owner, and ideally reference a bug. So: TODO(gangwu):
> crbug.com/XXXX ....

Done.

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:45: // reference them, we will get cycle
dependency warning. so once the functions
On 2015/07/30 19:59:44, Nicolas Zea wrote:
> nit: ". so once the" -> ". Once the"

Done.

https://codereview.chromium.org/1246523003/diff/540001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:51: SYNC_EXPORT_PRIVATE bool
GzipUncompress(const std::string& input,
On 2015/07/30 19:59:44, Nicolas Zea wrote:
> Given this isn't actually used in HttpBridge, and only in tests, I'd rather it
> live in the test itself (with the same comment about the TODO above)

Done.

https://codereview.chromium.org/1246523003/diff/540001/sync/tools/testserver/...
File sync/tools/testserver/sync_testserver.py (right):

https://codereview.chromium.org/1246523003/diff/540001/sync/tools/testserver/...
sync/tools/testserver/sync_testserver.py:202: enocde =
self.headers.getheader('Content-Encoding')
On 2015/07/30 19:59:44, Nicolas Zea wrote:
> enocde -> encode

Done.

[Gang Wu, 2015-08-03 16:41:55]

Patchset #15 (id:600001) has been deleted

[Alexei Svitkine (slow), 2015-08-03 18:12:30]

Looks good, just a few more suggestions.

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:43: return group_name == "Enabled";
Nit: It's better the use StartsWith() when using the field trial API and
checking for the group name. This allows you to have multiple experiment groups
with the Enabled behavior, which may be desirable later on when you make your
experiment configs for different channels.

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:63: // he rest of the code in the anon
namespace is copied from
Nit: he -> The

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
File sync/internal_api/http_bridge_unittest.cc (right):

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:22: namespace {
Nit: Add an empty line after this.

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:102: }
Nit: Add an empty line after this.

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:103: }
Nit:  // namespace

https://codereview.chromium.org/1246523003/diff/580001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1246523003/diff/580001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44685: +<histogram
name="Sync.RequestContentLengthCompressed" units="byte">
Nit: For readability, suggest slight name tweak to:

Sync.RequestContentLength.Compressed
and:
Sync.RequestContentLength.Original

Also, units is usually plural, so byte -> bytes.

Same for the other pair.

[Nicolas Zea, 2015-08-03 20:14:01]

LGTM with some nits

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:66: // TODO(gangwu): crbug.com/515695. The
following codes are copied from
nit: "following codes are" -> "following code is"

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:67: // components/metrics/compression_utils.cc,
we copied them because if we
nit: "compression_utils.cc, we" -> "compression_utils.cc. We"

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
File sync/internal_api/http_bridge_unittest.cc (right):

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:29: // he rest of the code in the anon
namespace is copied from
nit: "he rest" -> "The rest". Also, the same grammar nits about this comment
from the other file apply here.

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:379: EXPECT_NE(std::string::npos,
response.find("Accept-Encoding: gzip, deflate"));
nit: use StringPrintf and the accept encoding request header here too?

[Nicolas Zea, 2015-08-03 20:14:54]

Also, is the plan to add the kitchen sync tests separately?

[Gang Wu, 2015-08-04 00:01:48]

Patchset #16 (id:640001) has been deleted

[Gang Wu, 2015-08-04 16:35:24]

Patchset #16 (id:660001) has been deleted

[Gang Wu, 2015-08-04 22:14:00]

update UMA names and typos

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:43: return group_name == "Enabled";
On 2015/08/03 18:12:30, Alexei Svitkine wrote:
> Nit: It's better the use StartsWith() when using the field trial API and
> checking for the group name. This allows you to have multiple experiment
groups
> with the Enabled behavior, which may be desirable later on when you make your
> experiment configs for different channels.

Done.

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:63: // he rest of the code in the anon
namespace is copied from
On 2015/08/03 18:12:30, Alexei Svitkine wrote:
> Nit: he -> The

Done.

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
File sync/internal_api/http_bridge_unittest.cc (right):

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:22: namespace {
On 2015/08/03 18:12:30, Alexei Svitkine wrote:
> Nit: Add an empty line after this.

Done.

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:102: }
On 2015/08/03 18:12:30, Alexei Svitkine wrote:
> Nit: Add an empty line after this.

Done.

https://codereview.chromium.org/1246523003/diff/580001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:103: }
On 2015/08/03 18:12:30, Alexei Svitkine wrote:
> Nit:  // namespace

Done.

https://codereview.chromium.org/1246523003/diff/580001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1246523003/diff/580001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44685: +<histogram
name="Sync.RequestContentLengthCompressed" units="byte">
On 2015/08/03 18:12:30, Alexei Svitkine wrote:
> Nit: For readability, suggest slight name tweak to:
> 
> Sync.RequestContentLength.Compressed
> and:
> Sync.RequestContentLength.Original
> 
> Also, units is usually plural, so byte -> bytes.
> 
> Same for the other pair.

Done.

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:66: // TODO(gangwu): crbug.com/515695. The
following codes are copied from
On 2015/08/03 20:14:01, Nicolas Zea wrote:
> nit: "following codes are" -> "following code is"

Done.

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
sync/internal_api/http_bridge.cc:67: // components/metrics/compression_utils.cc,
we copied them because if we
On 2015/08/03 20:14:01, Nicolas Zea wrote:
> nit: "compression_utils.cc, we" -> "compression_utils.cc. We"

Done.

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
File sync/internal_api/http_bridge_unittest.cc (right):

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:29: // he rest of the code in the anon
namespace is copied from
On 2015/08/03 20:14:01, Nicolas Zea wrote:
> nit: "he rest" -> "The rest". Also, the same grammar nits about this comment
> from the other file apply here.

Done.

https://codereview.chromium.org/1246523003/diff/620001/sync/internal_api/http...
sync/internal_api/http_bridge_unittest.cc:379: EXPECT_NE(std::string::npos,
response.find("Accept-Encoding: gzip, deflate"));
On 2015/08/03 20:14:01, Nicolas Zea wrote:
> nit: use StringPrintf and the accept encoding request header here too?

Done.

[Gang Wu, 2015-08-04 22:14:39]

On 2015/08/03 20:14:54, Nicolas Zea wrote:
> Also, is the plan to add the kitchen sync tests separately?

Yes, kitchen sync tests will check in separately.

[Alexei Svitkine (slow), 2015-08-05 15:19:32]

lgtm

[Alexei Svitkine (slow), 2015-08-05 20:02:11]

Nicolas, WDYT of making compression_utils its own component? Given that it would
have very lightweight dependency, would it then be possible to have sync/ depend
on it?

[Nicolas Zea, 2015-08-05 23:39:15]

On 2015/08/05 20:02:11, Alexei Svitkine wrote:
> Nicolas, WDYT of making compression_utils its own component? Given that it
would
> have very lightweight dependency, would it then be possible to have sync/
depend
> on it?

I think that seems reasonable actually. It's likely a better option than base/,
as long as compression_utils is never expected to depend on something that
depends on sync (it should be basically standalone aside from base/ and net/)

[Gang Wu, 2015-08-06 22:08:11]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2015-08-06 22:08:11]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1246523003/#ps700001
(title: "merge conlict")

[commit-bot: I haz the power, 2015-08-06 22:09:03]

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

[commit-bot: I haz the power, 2015-08-06 22:15:50]

Committed patchset #17 (id:700001)

[commit-bot: I haz the power, 2015-08-06 22:16:33]

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