Record Data Savings for Client-Side LoFi

Record Data Savings for Client-Side LoFi

This CL causes Chrome to record data savings for Client LoFi when it is
used, according to the Content-Range header if it's present (e.g.
"Content-Range: bytes 0-2047/10000" implies that the original resource
was 10000 bytes). This CL also adds a new PreviewsState bit field,
CLIENT_LOFI_AUTO_RELOAD, that is set when a Client LoFi image is auto
reloaded because of a decoding error, so that the browser process can
count the data used for the reload against the savings.

See design doc for more info:
https://docs.google.com/document/d/1CoZfTswPq67VaHKsr0OrkpPbWT87dhhvPq9hylOxrlU/edit

BUG=605347
Review-Url: https://codereview.chromium.org/2873793002
Cr-Commit-Position: refs/heads/master@{#470850}
Committed: https://chromium.googlesource.com/chromium/src/+/afaa2a65a0d50a88e08fd595507668538cb6689e

[sclittle, 2017-05-09 22:05:48]

sclittle@chromium.org changed reviewers:
+ japhet@chromium.org, kinuko@chromium.org, megjablon@chromium.org

[sclittle, 2017-05-09 22:05:49]

megjablon: all
japhet: third_part/WebKit/*
kinuko: content/public/common/previews_state.h

Thanks in advance!

[kinuko, 2017-05-10 06:31:10]

Description was changed from

==========
Record Data Savings for Client-Side LoFi

This CL causes Chrome to record data savings for Client LoFi when it is
used, according to the Content-Range header if it's present (e.g.
"Content-Range: bytes 0-2047/10000" implies that the original resource
was 10000 bytes). This CL also adds a new PreviewsState bit field,
CLIENT_LOFI_AUTO_RELOAD, that is set when a Client LoFi image is auto
reloaded because of a decoding error, so that the browser process can
count the data used for the reload against the savings.

See design doc for more info:
https://docs.google.com/document/d/1CoZfTswPq67VaHKsr0OrkpPbWT87dhhvPq9hylOxr...

BUG=605347
==========

to

==========
Record Data Savings for Client-Side LoFi

This CL causes Chrome to record data savings for Client LoFi when it is
used, according to the Content-Range header if it's present (e.g.
"Content-Range: bytes 0-2047/10000" implies that the original resource
was 10000 bytes). This CL also adds a new PreviewsState bit field,
CLIENT_LOFI_AUTO_RELOAD, that is set when a Client LoFi image is auto
reloaded because of a decoding error, so that the browser process can
count the data used for the reload against the savings.

See design doc for more info:
https://docs.google.com/document/d/1CoZfTswPq67VaHKsr0OrkpPbWT87dhhvPq9hylOxr...

BUG=605347
==========

[kinuko, 2017-05-10 07:00:46]

lgtm

[Nate Chapin, 2017-05-10 17:18:41]

blink lgtm

[RyanSturm, 2017-05-10 17:56:24]

ryansturm@chromium.org changed reviewers:
+ ryansturm@chromium.org
- megjablon@chromium.org

[RyanSturm, 2017-05-10 18:14:09]

lgtm % a couple minor nits. I'd be fine with it as is though.

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

https://codereview.chromium.org/2873793002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:133:
return std::max<int64_t>(0, request.GetTotalReceivedBytes() -
As a note, I don't believe GetTotalReceivedBytes counts previous jobs (cache
revalidation, redirects) can you double check and add that information to
https://crbug.com/535701?

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

https://codereview.chromium.org/2873793002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1744:
{kSimple200ResponseHeaders, 140, false, false, 0},
nit: Can you comment on some of these test cases (like saying the content is
under 2048 for the first set and maybe some detail on the rest?

https://codereview.chromium.org/2873793002/diff/20001/content/public/common/p...
File content/public/common/previews_state.h (right):

https://codereview.chromium.org/2873793002/diff/20001/content/public/common/p...
content/public/common/previews_state.h:36: CLIENT_LOFI_AUTO_RELOAD = 1 << 2,  //
Request the original version of the
tiny nit: these comments don't line up well. Does git cl format leave it this
way? Feel free to leave it.

[sclittle, 2017-05-10 19:56:37]

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

https://codereview.chromium.org/2873793002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:133:
return std::max<int64_t>(0, request.GetTotalReceivedBytes() -
On 2017/05/10 18:14:08, RyanSturm wrote:
> As a note, I don't believe GetTotalReceivedBytes counts previous jobs (cache
> revalidation, redirects) can you double check and add that information to
> https://crbug.com/535701

From looking at URLRequest::GetTotalReceivedBytes(), it looks like you're right
that it doesn't include redirects, but it does seem to include revalidations
(running totals are saved from previous network transactions:
https://cs.chromium.org/chromium/src/net/http/http_cache_transaction.cc?l=2778).

Even if GetTotalReceivedBytes() is changed in the future to include the
redirect, it'll be fine since that way both the received size and the original
size used for data savings calculations will include the size of the redirect.
So that should just work.

Also, IIRC successful revalidations (i.e. 304 response) are marked as
was_cached, so they'll be handled by the if(request.was_cached()) check in
EstimateOriginalReceivedBytes below, so they don't get to this function.

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

https://codereview.chromium.org/2873793002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1744:
{kSimple200ResponseHeaders, 140, false, false, 0},
On 2017/05/10 18:14:09, RyanSturm wrote:
> nit: Can you comment on some of these test cases (like saying the content is
> under 2048 for the first set and maybe some detail on the rest?

Done.

https://codereview.chromium.org/2873793002/diff/20001/content/public/common/p...
File content/public/common/previews_state.h (right):

https://codereview.chromium.org/2873793002/diff/20001/content/public/common/p...
content/public/common/previews_state.h:36: CLIENT_LOFI_AUTO_RELOAD = 1 << 2,  //
Request the original version of the
On 2017/05/10 18:14:09, RyanSturm wrote:
> tiny nit: these comments don't line up well. Does git cl format leave it this
> way? Feel free to leave it.

git cl format left it this way; I agree it looks kinda bad, but hopefully it's
still readable enough.

[sclittle, 2017-05-10 19:56:46]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2017-05-10 19:56:46]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
ryansturm@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2873793002/#ps40001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-05-10 19:58:45]

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, 2017-05-10 20:59:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 20:59:26]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sclittle, 2017-05-10 21:10:37]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:12:01]

CQ is trying da patch.

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

[sclittle, 2017-05-10 21:26:45]

The CQ bit was unchecked by sclittle@chromium.org

[sclittle, 2017-05-10 21:34:55]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2017-05-10 21:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
ryansturm@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2873793002/#ps60001
(title: "fix crash in DRPNetworkDelegate when lofi_ui_service is null.")

[commit-bot: I haz the power, 2017-05-10 21:36:18]

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, 2017-05-11 00:00:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 00:00:59]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sclittle, 2017-05-11 00:16:41]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2017-05-11 00:17:57]

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, 2017-05-11 03:21:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 03:21:12]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sclittle, 2017-05-11 03:23:44]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2017-05-11 03:25:12]

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, 2017-05-11 06:42:00]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494473024417090,
"parent_rev": "d3fb9e2435e2c3015314496c2afa5bf64c4ace56", "commit_rev":
"afaa2a65a0d50a88e08fd595507668538cb6689e"}

[commit-bot: I haz the power, 2017-05-11 06:42:10]

Description was changed from

==========
Record Data Savings for Client-Side LoFi

This CL causes Chrome to record data savings for Client LoFi when it is
used, according to the Content-Range header if it's present (e.g.
"Content-Range: bytes 0-2047/10000" implies that the original resource
was 10000 bytes). This CL also adds a new PreviewsState bit field,
CLIENT_LOFI_AUTO_RELOAD, that is set when a Client LoFi image is auto
reloaded because of a decoding error, so that the browser process can
count the data used for the reload against the savings.

See design doc for more info:
https://docs.google.com/document/d/1CoZfTswPq67VaHKsr0OrkpPbWT87dhhvPq9hylOxr...

BUG=605347
==========

to

==========
Record Data Savings for Client-Side LoFi

This CL causes Chrome to record data savings for Client LoFi when it is
used, according to the Content-Range header if it's present (e.g.
"Content-Range: bytes 0-2047/10000" implies that the original resource
was 10000 bytes). This CL also adds a new PreviewsState bit field,
CLIENT_LOFI_AUTO_RELOAD, that is set when a Client LoFi image is auto
reloaded because of a decoding error, so that the browser process can
count the data used for the reload against the savings.

See design doc for more info:
https://docs.google.com/document/d/1CoZfTswPq67VaHKsr0OrkpPbWT87dhhvPq9hylOxr...

BUG=605347

Review-Url: https://codereview.chromium.org/2873793002
Cr-Commit-Position: refs/heads/master@{#470850}
Committed:
https://chromium.googlesource.com/chromium/src/+/afaa2a65a0d50a88e08fd5955076...
==========

[commit-bot: I haz the power, 2017-05-11 06:42:15]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/afaa2a65a0d50a88e08fd5955076...