Record partial savings for incomplete responses through the DRP.

Record partial savings for incomplete responses through the DRP.

Before this CL, incomplete responses through the Data Reduction Proxy
would be recorded as having 0 savings, even though a non-zero amount of
data was downloaded. After this CL, if the partial response had both
content-length and x-original-content-length headers, then the savings
are calculated with an effective original content length of
(partial_length * total_original_length / total_content_length).

BUG=439606
Committed: https://crrev.com/0dd2db4a82a78f70ab36e6264f8d721e25f36d74
Cr-Commit-Position: refs/heads/master@{#412720}

[sclittle, 2016-08-16 03:00:50]

Patchset #1 (id:1) has been deleted

[sclittle, 2016-08-16 03:01:06]

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

[bengr, 2016-08-16 16:14:45]

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc
(right):

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:602:
"x-original-content-length: 3000\r\n\r\n",
Why do you use lower case for x-original-cl, but not for the other headers?

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:613:
300, 100},
Add a test case where xocl is 0 but received isn't, and another case where xocl
is 0, cl is > 0 and received is 0.

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:635:
100, 300},
Add a case with a corrupted xocl header and another where received is > max
int32.

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:638:
for (const auto& test : test_cases) {
Would you mind changing the previous test to also use auto when looping? Also,
either rename test_cases as tests or test as test_case, and be consistent in
naming with previous tests.

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:101:
int64_t ScaleContentLength(int64_t value,
This method has nothing to do with Content-Length, so consider renaming it to
something like: ScaleInt64Value() or ScaleValueByRatio()

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:146:
return ScaleContentLength(request.received_response_content_length(),
Does content type matter? I think not but wanted to check.

[sclittle, 2016-08-16 21:52:29]

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc
(right):

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:602:
"x-original-content-length: 3000\r\n\r\n",
On 2016/08/16 16:14:44, bengr wrote:
> Why do you use lower case for x-original-cl, but not for the other headers?

The case doesn't matter. I've now changed x-ocl to be capitalized for
consistency.

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:613:
300, 100},
On 2016/08/16 16:14:45, bengr wrote:
> Add a test case where xocl is 0 but received isn't, and another case where
xocl
> is 0, cl is > 0 and received is 0.

Done.

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:635:
100, 300},
On 2016/08/16 16:14:45, bengr wrote:
> Add a case with a corrupted xocl header and another where received is > max
> int32.

Added corrupted xocl header and corrupted cl header tests.

Making received > max int32 isn't really possible in a test, since received
represents an actual number of bytes in memory and these tests need to be
runnable on 32-bit systems and other systems with less memory than that, e.g. we
can't really allocate 2^31 bytes on a phone with 1GB (2^30 bytes) of RAM. Even
if we find a way around that, the test would take a really long time to run.

We already have a test where content length > max int32, which tests the same
codepath that received > max int32 would test.

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:638:
for (const auto& test : test_cases) {
On 2016/08/16 16:14:45, bengr wrote:
> Would you mind changing the previous test to also use auto when looping? Also,
> either rename test_cases as tests or test as test_case, and be consistent in
> naming with previous tests.

Done.

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:101:
int64_t ScaleContentLength(int64_t value,
On 2016/08/16 16:14:45, bengr wrote:
> This method has nothing to do with Content-Length, so consider renaming it to
> something like: ScaleInt64Value() or ScaleValueByRatio()

The reason it mentions Content-Length is because it assumes that all the numbers
are non-negative. We could do additional work to support negatives, but I don't
see any reason to add that support.

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:146:
return ScaleContentLength(request.received_response_content_length(),
On 2016/08/16 16:14:45, bengr wrote:
> Does content type matter? I think not but wanted to check.

I don't think content-type matters here; it's just a number of bytes.

We could perhaps only do this for video resources, but I don't see a reason not
to do this for all resources. Non-video resources would also tend to be so small
that this change shouldn't have any noticeable effect, so it's safe anyways.

[bengr, 2016-08-17 21:31:53]

lgtm with a naming suggestion.

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:101:
int64_t ScaleContentLength(int64_t value,
On 2016/08/16 21:52:29, sclittle wrote:
> On 2016/08/16 16:14:45, bengr wrote:
> > This method has nothing to do with Content-Length, so consider renaming it
to
> > something like: ScaleInt64Value() or ScaleValueByRatio()
> 
> The reason it mentions Content-Length is because it assumes that all the
numbers
> are non-negative. We could do additional work to support negatives, but I
don't
> see any reason to add that support.

I'd either rename the function to something closer to what it does, e.g.,
ScaleNonnegativeInt64Value() or change the parameter names so it's more obvious
how it is intended to be used with content length:
received_content_length, original_content_length_from_header,
content_length_from_header.

[sclittle, 2016-08-18 00:35:21]

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2242353003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:101:
int64_t ScaleContentLength(int64_t value,
On 2016/08/17 21:31:53, bengr wrote:
> On 2016/08/16 21:52:29, sclittle wrote:
> > On 2016/08/16 16:14:45, bengr wrote:
> > > This method has nothing to do with Content-Length, so consider renaming it
> to
> > > something like: ScaleInt64Value() or ScaleValueByRatio()
> > 
> > The reason it mentions Content-Length is because it assumes that all the
> numbers
> > are non-negative. We could do additional work to support negatives, but I
> don't
> > see any reason to add that support.
> 
> I'd either rename the function to something closer to what it does, e.g.,
> ScaleNonnegativeInt64Value() or change the parameter names so it's more
obvious
> how it is intended to be used with content length:
> received_content_length, original_content_length_from_header,
> content_length_from_header.

I've changed it to ScaleByteCountByRatio, and changed the first parameter to be
|byte_count|. This way, there's no connection to the Content-Length header or
other headers, and it's clear that the numbers should be non-negative, since
they're byte counts.

[sclittle, 2016-08-18 00:35:27]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2016-08-18 00:35:28]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2242353003/#ps60001
(title: "renamed ScaleContentLength to ScaleByteCountByRatio")

[commit-bot: I haz the power, 2016-08-18 00:36:11]

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-08-18 01:34:25]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-18 01:36:05]

Description was changed from

==========
Record partial savings for incomplete responses through the DRP.

Before this CL, incomplete responses through the Data Reduction Proxy
would be recorded as having 0 savings, even though a non-zero amount of
data was downloaded. After this CL, if the partial response had both
content-length and x-original-content-length headers, then the savings
are calculated with an effective original content length of
(partial_length * total_original_length / total_content_length).

BUG=439606
==========

to

==========
Record partial savings for incomplete responses through the DRP.

Before this CL, incomplete responses through the Data Reduction Proxy
would be recorded as having 0 savings, even though a non-zero amount of
data was downloaded. After this CL, if the partial response had both
content-length and x-original-content-length headers, then the savings
are calculated with an effective original content length of
(partial_length * total_original_length / total_content_length).

BUG=439606

Committed: https://crrev.com/0dd2db4a82a78f70ab36e6264f8d721e25f36d74
Cr-Commit-Position: refs/heads/master@{#412720}
==========

[commit-bot: I haz the power, 2016-08-18 01:36:06]

Patchset 3 (id:??) landed as
https://crrev.com/0dd2db4a82a78f70ab36e6264f8d721e25f36d74
Cr-Commit-Position: refs/heads/master@{#412720}