Support gzip-compressed seed data from the variations server.

Support gzip-compressed seed data from the variations server.

Firstly, the client asks for gzip-compressed data by adding "gzip" to
A-IM HTTP request header. Secondly, if "gzip" is present in IM HTTP
response header, the client uncompresses data. Gzip compression could be
used in combination with delta compression.

A-IM and IM headers are defined in RFC 3229:
http://tools.ietf.org/html/rfc3229#page-32

This CL re-adds components/variations/service tests to components_unittests.

BUG=543271
Committed: https://crrev.com/f6acefe76a605a6fc17714306919c56ab3b2e2af
Cr-Commit-Position: refs/heads/master@{#354788}

[veranika, 2015-10-14 21:27:06]

veranika@chromium.org changed reviewers:
+ asvitkine@chromium.org, caitkp@chromium.org

[veranika, 2015-10-15 00:11:14]

Hi,

Cait, could you please review the change in components/BUILD.gn?
Alexei, could you please review everything else?

[Alexei Svitkine (slow), 2015-10-15 15:50:44]

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
components/variations/service/variations_service.cc:675: }
Can you make a new helper for this block of code?

It can return success and have out aprams for is_delta_compressed and
is_gzip_compressed.

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
File components/variations/service/variations_service_unittest.cc (right):

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
components/variations/service/variations_service_unittest.cc:218: const
std::string* header = NULL) {
Default values for params are not allowed by our style guide.

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
components/variations/service/variations_service_unittest.cc:494:
service.GetVariationsServerURL(&prefs, std::string());
Could this line of code be moved into the TestVariationsService ctor, so it
isn't duplicated in all the tests?

https://codereview.chromium.org/1404583004/diff/20001/components/variations/v...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/1404583004/diff/20001/components/variations/v...
components/variations/variations_seed_store.cc:205:
UMA_HISTOGRAM_PERCENTAGE("Variations.StoreSeed.GzipSize.ReductionPercent",
There should be a guard somewhere that ungzipped_data.length() is not 0, else
next line will be a division by zero crash.

https://codereview.chromium.org/1404583004/diff/20001/components/variations/v...
components/variations/variations_seed_store.cc:206: 100 * size_reduction /
ungzipped_data.length());
Nit: Align.

[Cait (Slow), 2015-10-15 17:35:45]

components.gn lgtm

[veranika, 2015-10-15 20:02:57]

PTAL

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
components/variations/service/variations_service.cc:675: }
On 2015/10/15 15:50:43, Alexei Svitkine wrote:
> Can you make a new helper for this block of code?
> 
> It can return success and have out aprams for is_delta_compressed and
> is_gzip_compressed.

Done.

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
File components/variations/service/variations_service_unittest.cc (right):

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
components/variations/service/variations_service_unittest.cc:218: const
std::string* header = NULL) {
On 2015/10/15 15:50:44, Alexei Svitkine wrote:
> Default values for params are not allowed by our style guide.

But this doesn't apply for functions in unnamed namespaces, does it?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
components/variations/service/variations_service_unittest.cc:494:
service.GetVariationsServerURL(&prefs, std::string());
On 2015/10/15 15:50:44, Alexei Svitkine wrote:
> Could this line of code be moved into the TestVariationsService ctor, so it
> isn't duplicated in all the tests?

VariationsService::variations_server_url_ is a private field and
TestVariationsService is in an anonymous namespace, so it's not possible to set
it as a friend of VariationsService. I could make variations_server_url_ a
protected field, but I'm not entirely sure it's the right thing to do.

https://codereview.chromium.org/1404583004/diff/20001/components/variations/v...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/1404583004/diff/20001/components/variations/v...
components/variations/variations_seed_store.cc:205:
UMA_HISTOGRAM_PERCENTAGE("Variations.StoreSeed.GzipSize.ReductionPercent",
On 2015/10/15 15:50:44, Alexei Svitkine wrote:
> There should be a guard somewhere that ungzipped_data.length() is not 0, else
> next line will be a division by zero crash.

Done.

https://codereview.chromium.org/1404583004/diff/20001/components/variations/v...
components/variations/variations_seed_store.cc:206: 100 * size_reduction /
ungzipped_data.length());
On 2015/10/15 15:50:44, Alexei Svitkine wrote:
> Nit: Align.

Done.

[Alexei Svitkine (slow), 2015-10-15 20:30:26]

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
File components/variations/service/variations_service_unittest.cc (right):

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
components/variations/service/variations_service_unittest.cc:218: const
std::string* header = NULL) {
On 2015/10/15 20:02:57, veranika wrote:
> On 2015/10/15 15:50:44, Alexei Svitkine wrote:
> > Default values for params are not allowed by our style guide.
> 
> But this doesn't apply for functions in unnamed namespaces, does it?
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

Looks like you're right, however we don't use it in metrics code and I'd prefer
if we keep it consistent.

Suggest adding an overload:

SimulateServerResponseWithHeader()

Then, the original function can just call the above with nullptr as param for
the header.

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
components/variations/service/variations_service_unittest.cc:494:
service.GetVariationsServerURL(&prefs, std::string());
On 2015/10/15 20:02:57, veranika wrote:
> On 2015/10/15 15:50:44, Alexei Svitkine wrote:
> > Could this line of code be moved into the TestVariationsService ctor, so it
> > isn't duplicated in all the tests?
> 
> VariationsService::variations_server_url_ is a private field and
> TestVariationsService is in an anonymous namespace, so it's not possible to
set
> it as a friend of VariationsService. I could make variations_server_url_ a
> protected field, but I'm not entirely sure it's the right thing to do.

I suggest providing a protected setter for it.

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
components/variations/service/variations_service.cc:245: if
(static_cast<size_t>(*is_delta_compressed + *is_gzip_compressed) !=
Not a fan of adding booleans and casting to size_t.

In fact, I'm not sure adding bools is guaranteed to give 2, if you don't cast
them first.

How about:

int im_count = (*is_delta_compressed ? 1 : 0) + (*is_gzip_compressed ? 1 : 0);
if (im_count != im.size()) {
}

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
components/variations/service/variations_service.cc:543: std::string
supported_im = enable_deltas ? "x-bm,gzip" : "gzip";
Nit: I'd make this var a |const char*| and make it's value include the "A-IM:"
prefix.

Then, just pass it in directly on the line below. This way, you avoid doing
extra string conversions and concatenations.

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
components/variations/service/variations_service.cc:692: return;
Add a comment about why it's OK to return here.

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
File components/variations/service/variations_service_unittest.cc (right):

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
components/variations/service/variations_service_unittest.cc:214: // if it is
not NULL.
Nit: For new code, in code use "nullptr", and in comments use "null".

[veranika, 2015-10-15 22:10:38]

Comments are resolved, PTAL

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
File components/variations/service/variations_service_unittest.cc (right):

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
components/variations/service/variations_service_unittest.cc:218: const
std::string* header = NULL) {
On 2015/10/15 20:30:26, Alexei Svitkine wrote:
> On 2015/10/15 20:02:57, veranika wrote:
> > On 2015/10/15 15:50:44, Alexei Svitkine wrote:
> > > Default values for params are not allowed by our style guide.
> > 
> > But this doesn't apply for functions in unnamed namespaces, does it?
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...
> 
> Looks like you're right, however we don't use it in metrics code and I'd
prefer
> if we keep it consistent.
> 
> Suggest adding an overload:
> 
> SimulateServerResponseWithHeader()
> 
> Then, the original function can just call the above with nullptr as param for
> the header.

Done.

https://codereview.chromium.org/1404583004/diff/20001/components/variations/s...
components/variations/service/variations_service_unittest.cc:494:
service.GetVariationsServerURL(&prefs, std::string());
On 2015/10/15 20:30:26, Alexei Svitkine wrote:
> On 2015/10/15 20:02:57, veranika wrote:
> > On 2015/10/15 15:50:44, Alexei Svitkine wrote:
> > > Could this line of code be moved into the TestVariationsService ctor, so
it
> > > isn't duplicated in all the tests?
> > 
> > VariationsService::variations_server_url_ is a private field and
> > TestVariationsService is in an anonymous namespace, so it's not possible to
> set
> > it as a friend of VariationsService. I could make variations_server_url_ a
> > protected field, but I'm not entirely sure it's the right thing to do.
> 
> I suggest providing a protected setter for it.

Good idea, thanks.

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
components/variations/service/variations_service.cc:245: if
(static_cast<size_t>(*is_delta_compressed + *is_gzip_compressed) !=
On 2015/10/15 20:30:26, Alexei Svitkine wrote:
> Not a fan of adding booleans and casting to size_t.
> 
> In fact, I'm not sure adding bools is guaranteed to give 2, if you don't cast
> them first.
> 
> How about:
> 
> int im_count = (*is_delta_compressed ? 1 : 0) + (*is_gzip_compressed ? 1 : 0);
> if (im_count != im.size()) {
> }

Done.

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
components/variations/service/variations_service.cc:543: std::string
supported_im = enable_deltas ? "x-bm,gzip" : "gzip";
On 2015/10/15 20:30:26, Alexei Svitkine wrote:
> Nit: I'd make this var a |const char*| and make it's value include the "A-IM:"
> prefix.
> 
> Then, just pass it in directly on the line below. This way, you avoid doing
> extra string conversions and concatenations.

Done.

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
components/variations/service/variations_service.cc:692: return;
On 2015/10/15 20:30:26, Alexei Svitkine wrote:
> Add a comment about why it's OK to return here.

Done.

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
File components/variations/service/variations_service_unittest.cc (right):

https://codereview.chromium.org/1404583004/diff/40001/components/variations/s...
components/variations/service/variations_service_unittest.cc:214: // if it is
not NULL.
On 2015/10/15 20:30:26, Alexei Svitkine wrote:
> Nit: For new code, in code use "nullptr", and in comments use "null".

Done.

[Alexei Svitkine (slow), 2015-10-16 15:16:44]

Thanks, almost there - probably the last round of comments. :)

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service.cc:221: const
net::HttpResponseHeaders* headers, const base::StringPiece& name) {
Nit: Prefer 1 param per line if the first one wraps.

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service.cc:544: const char *
supported_im = enable_deltas ? "A-IM:x-bm,gzip" : "A-IM:gzip";
Nit: No space between char and *.

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service.cc:687: const std::string
country_code = GetHeaderValue(headers, "X-Country");
Nit: Can you move these two lines to right above the StoreSeed() call that uses
them?

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service.cc:693: // Details of errors
are logged by GetInstanceManipulations, just return.
What I'd like in this comment is why it's okay to return - i.e. that the
instance manipulations specified in the response do not match any of our
supported formats.

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service.cc:694: return;
Also, it would be good to log this case in a histogram.

Instead of adding a new histogram, can you add an extra case for this in
Variations.SeedStoreResult histogram? Then, you can expose a method on the
SeedStore to log this specific enum. (i.e. the enum def can still stay in the
seed store .cc file, but you can have a method for logging this specific case)

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
File components/variations/service/variations_service_unittest.cc (right):

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service_unittest.cc:499:
service.set_intercepts_fetch(false);
Nit: If more places call this than not, suggest inverting the default and only
setting it to true for the ones that need it.

https://codereview.chromium.org/1404583004/diff/60001/components/variations/v...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/1404583004/diff/60001/components/variations/v...
components/variations/variations_seed_store.cc:205:
RecordSeedStoreHistogram(VARIATIONS_SEED_STORE_FAILED_EMPTY);
Nit: Please add a separate enum for this case, just so we can differentiate the
cases.

[veranika, 2015-10-16 21:01:00]

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
File components/variations/service/variations_service.cc (right):

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service.cc:221: const
net::HttpResponseHeaders* headers, const base::StringPiece& name) {
On 2015/10/16 15:16:44, Alexei Svitkine wrote:
> Nit: Prefer 1 param per line if the first one wraps.

Done.

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service.cc:544: const char *
supported_im = enable_deltas ? "A-IM:x-bm,gzip" : "A-IM:gzip";
On 2015/10/16 15:16:44, Alexei Svitkine wrote:
> Nit: No space between char and *.

Done.

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service.cc:687: const std::string
country_code = GetHeaderValue(headers, "X-Country");
On 2015/10/16 15:16:44, Alexei Svitkine wrote:
> Nit: Can you move these two lines to right above the StoreSeed() call that
uses
> them?

Done.

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service.cc:693: // Details of errors
are logged by GetInstanceManipulations, just return.
On 2015/10/16 15:16:44, Alexei Svitkine wrote:
> What I'd like in this comment is why it's okay to return - i.e. that the
> instance manipulations specified in the response do not match any of our
> supported formats.

Done.

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service.cc:694: return;
On 2015/10/16 15:16:44, Alexei Svitkine wrote:
> Also, it would be good to log this case in a histogram.
> 
> Instead of adding a new histogram, can you add an extra case for this in
> Variations.SeedStoreResult histogram? Then, you can expose a method on the
> SeedStore to log this specific enum. (i.e. the enum def can still stay in the
> seed store .cc file, but you can have a method for logging this specific case)

Done.

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
File components/variations/service/variations_service_unittest.cc (right):

https://codereview.chromium.org/1404583004/diff/60001/components/variations/s...
components/variations/service/variations_service_unittest.cc:499:
service.set_intercepts_fetch(false);
On 2015/10/16 15:16:44, Alexei Svitkine wrote:
> Nit: If more places call this than not, suggest inverting the default and only
> setting it to true for the ones that need it.

There are 11 uses of TestVariationsService and only 5 of them use
set_intercepts_fetch(false), so I don't change it.

https://codereview.chromium.org/1404583004/diff/60001/components/variations/v...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/1404583004/diff/60001/components/variations/v...
components/variations/variations_seed_store.cc:205:
RecordSeedStoreHistogram(VARIATIONS_SEED_STORE_FAILED_EMPTY);
On 2015/10/16 15:16:44, Alexei Svitkine wrote:
> Nit: Please add a separate enum for this case, just so we can differentiate
the
> cases.

Done.

[Alexei Svitkine (slow), 2015-10-16 21:10:36]

LGTM % remaining comments

https://codereview.chromium.org/1404583004/diff/60001/components/variations/v...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/1404583004/diff/60001/components/variations/v...
components/variations/variations_seed_store.cc:205:
RecordSeedStoreHistogram(VARIATIONS_SEED_STORE_FAILED_EMPTY);
On 2015/10/16 21:01:00, veranika wrote:
> On 2015/10/16 15:16:44, Alexei Svitkine wrote:
> > Nit: Please add a separate enum for this case, just so we can differentiate
> the
> > cases.
> 
> Done.

Looks like you forgot to update the enum param here.

https://codereview.chromium.org/1404583004/diff/80001/components/variations/v...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/1404583004/diff/80001/components/variations/v...
components/variations/variations_seed_store.cc:484: 
Nit: Remove extra blank line.

[veranika, 2015-10-16 21:42:59]

https://codereview.chromium.org/1404583004/diff/60001/components/variations/v...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/1404583004/diff/60001/components/variations/v...
components/variations/variations_seed_store.cc:205:
RecordSeedStoreHistogram(VARIATIONS_SEED_STORE_FAILED_EMPTY);
On 2015/10/16 21:10:36, Alexei Svitkine wrote:
> On 2015/10/16 21:01:00, veranika wrote:
> > On 2015/10/16 15:16:44, Alexei Svitkine wrote:
> > > Nit: Please add a separate enum for this case, just so we can
differentiate
> > the
> > > cases.
> > 
> > Done.
> 
> Looks like you forgot to update the enum param here.

Ouch.

https://codereview.chromium.org/1404583004/diff/80001/components/variations/v...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/1404583004/diff/80001/components/variations/v...
components/variations/variations_seed_store.cc:484: 
On 2015/10/16 21:10:36, Alexei Svitkine wrote:
> Nit: Remove extra blank line.

Done.

[Alexei Svitkine (slow), 2015-10-16 21:47:18]

(Also, please mention in the CL description about re-adding the
VariationsService tests to the BUILD file and also ping the other threads about
it for their FYI.)

[veranika, 2015-10-19 15:27:56]

Description was changed from

==========
Support gzip-compressed seed data from the variations server.

Firstly, the client asks for gzip-compressed data by adding "gzip" to
A-IM HTTP request header. Secondly, if "gzip" is present in IM HTTP
response header, the client uncompresses data. Gzip compression could be
used in combination with delta compression.

A-IM and IM headers are defined in RFC 3229:
http://tools.ietf.org/html/rfc3229#page-32

BUG=543271
==========

to

==========
Support gzip-compressed seed data from the variations server.

Firstly, the client asks for gzip-compressed data by adding "gzip" to
A-IM HTTP request header. Secondly, if "gzip" is present in IM HTTP
response header, the client uncompresses data. Gzip compression could be
used in combination with delta compression.

A-IM and IM headers are defined in RFC 3229:
http://tools.ietf.org/html/rfc3229#page-32

This CL re-adds components/variations/service tests to components_unittests.

BUG=543271
==========

[veranika, 2015-10-19 15:28:26]

The CQ bit was checked by veranika@chromium.org

[veranika, 2015-10-19 15:28:27]

The patchset sent to the CQ was uploaded after l-g-t-m from caitkp@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1404583004/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-10-19 15:28:57]

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

[commit-bot: I haz the power, 2015-10-19 16:38:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-19 16:38:59]

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