Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(470)

Issue 1363673004: [DRP] Consistently use LoFi for an entire page (Closed)

Created:
5 years, 3 months ago by megjablon
Modified:
5 years, 2 months ago
Reviewers:
Lei Zhang, bengr, tbansal1
CC:
chromium-reviews, darin-cc_chromium.org, cbentzel+watch_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[DRP] Consistently use LoFi for an entire page Data Reduction Proxy changes for the cl that stores the LoFi state in the renderer and uses that state to add LoFi request headers when appropriate. Creates the method ShouldEnableLoFiMode which the renderer will use to get the Lo-Fi state for a navigation. The renderer will then add lofi_on to the request via its ResourceRequestInfo. DRPNetworkDelegate will add the "q=low" header when lofi_on is true. The snackbar triggering does not use this state yet, but still looks at the last mainframe LoFi value. This cl turns off Lo-Fi until the renderer follow up cl is landed. BUG=524652 Committed: https://crrev.com/61d87091c812e803e060854d24877c513814661b Cr-Commit-Position: refs/heads/master@{#355674}

Patch Set 1 #

Total comments: 18

Patch Set 2 : addressing tbansal comments #

Total comments: 10

Patch Set 3 : check config #

Total comments: 5

Patch Set 4 : comments and adding IsLoFiOnViaFlags #

Total comments: 14

Patch Set 5 : nits and test fixes #

Total comments: 18

Patch Set 6 : addressing bengr comments #

Patch Set 7 : rebase and fix experiment control header #

Total comments: 49

Patch Set 8 : bengr comments #

Total comments: 14

Patch Set 9 : thestig comment #

Total comments: 6

Patch Set 10 : bengr comments #

Total comments: 8

Patch Set 11 : final comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+554 lines, -700 lines) Patch
M chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/common/chrome_content_client.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M chrome/renderer/page_load_histograms.cc View 1 2 3 4 5 6 7 8 9 10 11 chunks +28 lines, -22 lines 0 comments Download
M components/data_reduction_proxy.gypi View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M components/data_reduction_proxy/content/browser/BUILD.gn View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
A components/data_reduction_proxy/content/browser/content_lofi_decider.h View 1 2 3 4 5 6 7 8 9 1 chunk +35 lines, -0 lines 0 comments Download
A components/data_reduction_proxy/content/browser/content_lofi_decider.cc View 1 2 3 4 5 6 7 1 chunk +18 lines, -0 lines 0 comments Download
M components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h View 1 2 3 4 5 6 7 8 9 3 chunks +4 lines, -6 lines 0 comments Download
M components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -11 lines 0 comments Download
M components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter_unittest.cc View 1 chunk +15 lines, -106 lines 0 comments Download
M components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h View 1 chunk +3 lines, -8 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h View 1 2 3 4 5 6 7 8 9 5 chunks +41 lines, -86 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc View 1 2 3 4 5 6 7 13 chunks +71 lines, -169 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h View 1 2 3 4 5 6 3 chunks +0 lines, -18 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc View 1 2 3 4 5 6 3 chunks +2 lines, -22 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc View 1 2 3 4 5 6 7 6 chunks +52 lines, -86 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h View 1 2 3 4 5 6 7 4 chunks +15 lines, -0 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -0 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +15 lines, -18 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc View 1 2 3 4 5 6 17 chunks +73 lines, -60 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h View 1 2 3 4 5 6 4 chunks +13 lines, -8 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc View 1 2 3 4 5 6 5 chunks +23 lines, -18 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc View 1 2 3 4 5 6 14 chunks +50 lines, -32 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc View 1 2 3 4 5 6 7 8 9 10 9 chunks +10 lines, -20 lines 0 comments Download
M components/data_reduction_proxy/core/common/data_reduction_proxy_params.h View 1 2 3 4 5 6 7 8 9 2 chunks +20 lines, -7 lines 0 comments Download
M components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc View 1 2 3 4 5 6 2 chunks +14 lines, -0 lines 0 comments Download
A components/data_reduction_proxy/core/common/lofi_decider.h View 1 2 3 4 5 6 7 1 chunk +27 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 62 (27 generated)
megjablon
5 years, 3 months ago (2015-09-24 22:32:01 UTC) #2
tbansal1
drive-by review. https://codereview.chromium.org/1363673004/diff/1/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/1/chrome/renderer/page_load_histograms.cc#newcode70 chrome/renderer/page_load_histograms.cc:70: bool IsInLoFiEnabledGroup() { It would be preferable ...
5 years, 2 months ago (2015-09-28 17:49:02 UTC) #4
megjablon
https://codereview.chromium.org/1363673004/diff/1/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/1/chrome/renderer/page_load_histograms.cc#newcode70 chrome/renderer/page_load_histograms.cc:70: bool IsInLoFiEnabledGroup() { On 2015/09/28 17:49:02, tbansal1 wrote: > ...
5 years, 2 months ago (2015-09-28 22:43:38 UTC) #7
bengr
Let me know when the other CL stabilizes and I'll take a look. Thanks.
5 years, 2 months ago (2015-09-28 23:50:48 UTC) #8
tbansal1
https://codereview.chromium.org/1363673004/diff/60001/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/60001/chrome/renderer/page_load_histograms.cc#newcode225 chrome/renderer/page_load_histograms.cc:225: bool lofi_on /* Whether the frame used the Lo-Fi ...
5 years, 2 months ago (2015-09-29 00:47:48 UTC) #9
megjablon
https://codereview.chromium.org/1363673004/diff/60001/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/60001/chrome/renderer/page_load_histograms.cc#newcode225 chrome/renderer/page_load_histograms.cc:225: bool lofi_on /* Whether the frame used the Lo-Fi ...
5 years, 2 months ago (2015-09-29 02:38:38 UTC) #11
tbansal1
https://codereview.chromium.org/1363673004/diff/170001/components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1363673004/diff/170001/components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc#newcode20 components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:20: !params::IsIncludedInLoFiControlFieldTrial(); nit: this is outside commented section. https://codereview.chromium.org/1363673004/diff/170001/components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc#newcode20 components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:20: ...
5 years, 2 months ago (2015-09-30 15:52:40 UTC) #13
megjablon
https://codereview.chromium.org/1363673004/diff/170001/components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1363673004/diff/170001/components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc#newcode20 components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:20: !params::IsIncludedInLoFiControlFieldTrial(); On 2015/09/30 15:52:39, tbansal1 wrote: > This may ...
5 years, 2 months ago (2015-09-30 18:26:03 UTC) #14
tbansal1
nits... https://codereview.chromium.org/1363673004/diff/190001/components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://codereview.chromium.org/1363673004/diff/190001/components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc#newcode20 components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:20: // (params::IsLoFiOnViaFlags() || Please add DCHECKs here. e.g., ...
5 years, 2 months ago (2015-09-30 19:55:25 UTC) #15
megjablon
https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/190001/components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc#newcode20 components/data_reduction_proxy/content/browser/content_data_reduction_proxy_lofi_helper.cc:20: // (params::IsLoFiOnViaFlags() || On 2015/09/30 19:55:25, tbansal1 wrote: > ...
5 years, 2 months ago (2015-09-30 21:43:27 UTC) #16
bengr
Here's a start. In general I think we need to be clearer in our naming ...
5 years, 2 months ago (2015-10-03 00:01:30 UTC) #19
megjablon
https://chromiumcodereview.appspot.com/1363673004/diff/250001/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/250001/chrome/renderer/page_load_histograms.cc#newcode225 chrome/renderer/page_load_histograms.cc:225: bool lofi_on /* The current conditions may use the ...
5 years, 2 months ago (2015-10-05 20:22:59 UTC) #21
megjablon
On 2015/10/05 20:22:59, megjablon wrote: > https://chromiumcodereview.appspot.com/1363673004/diff/250001/chrome/renderer/page_load_histograms.cc > File chrome/renderer/page_load_histograms.cc (right): > > https://chromiumcodereview.appspot.com/1363673004/diff/250001/chrome/renderer/page_load_histograms.cc#newcode225 > ...
5 years, 2 months ago (2015-10-09 20:06:19 UTC) #22
bengr
https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_load_histograms.cc#newcode226 chrome/renderer/page_load_histograms.cc:226: bool is_using_lofi, How about lofi_active_for_page? Then the comment could ...
5 years, 2 months ago (2015-10-12 21:06:41 UTC) #23
megjablon
thestig: chrome/* https://chromiumcodereview.appspot.com/1363673004/diff/310001/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/310001/chrome/renderer/page_load_histograms.cc#newcode226 chrome/renderer/page_load_histograms.cc:226: bool is_using_lofi, On 2015/10/12 21:06:40, bengr wrote: ...
5 years, 2 months ago (2015-10-12 23:08:05 UTC) #26
Lei Zhang
I defer to the other reviewers. Happy to stamp when you have their approvals. https://chromiumcodereview.appspot.com/1363673004/diff/350001/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc ...
5 years, 2 months ago (2015-10-12 23:14:26 UTC) #27
megjablon
https://chromiumcodereview.appspot.com/1363673004/diff/350001/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/350001/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc#newcode70 chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:70: scoped_ptr<data_reduction_proxy::LoFiDecider> lofi_decider( On 2015/10/12 23:14:26, Lei Zhang wrote: > ...
5 years, 2 months ago (2015-10-12 23:21:59 UTC) #28
tbansal1
https://codereview.chromium.org/1363673004/diff/310001/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1363673004/diff/310001/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc#newcode28 components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:28: #include "net/base/load_flags.h" Is this include needed? https://codereview.chromium.org/1363673004/diff/310001/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc ...
5 years, 2 months ago (2015-10-13 17:12:02 UTC) #29
megjablon
https://chromiumcodereview.appspot.com/1363673004/diff/370001/components/data_reduction_proxy/content/browser/content_lofi_decider.h File components/data_reduction_proxy/content/browser/content_lofi_decider.h (right): https://chromiumcodereview.appspot.com/1363673004/diff/370001/components/data_reduction_proxy/content/browser/content_lofi_decider.h#newcode17 components/data_reduction_proxy/content/browser/content_lofi_decider.h:17: // Class responsible for deciding whether a request should ...
5 years, 2 months ago (2015-10-14 17:50:18 UTC) #30
bengr
https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_load_histograms.cc#newcode487 chrome/renderer/page_load_histograms.cc:487: PLT_HISTOGRAM( On 2015/10/12 23:08:04, megjablon wrote: > On 2015/10/12 ...
5 years, 2 months ago (2015-10-14 22:34:10 UTC) #31
megjablon
https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_load_histograms.cc#newcode487 chrome/renderer/page_load_histograms.cc:487: PLT_HISTOGRAM( On 2015/10/14 22:34:10, bengr wrote: > On 2015/10/12 ...
5 years, 2 months ago (2015-10-15 03:44:06 UTC) #33
megjablon
On 2015/10/15 03:44:06, megjablon wrote: > https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_load_histograms.cc > File chrome/renderer/page_load_histograms.cc (right): > > https://codereview.chromium.org/1363673004/diff/310001/chrome/renderer/page_load_histograms.cc#newcode487 > ...
5 years, 2 months ago (2015-10-16 20:39:44 UTC) #34
bengr
https://codereview.chromium.org/1363673004/diff/430001/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1363673004/diff/430001/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc#newcode181 components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:181: is_using_lofi_mode = lofi_decider->IsUsingLoFiMode(*request); I don't understand how this is ...
5 years, 2 months ago (2015-10-16 21:10:07 UTC) #35
megjablon
https://codereview.chromium.org/1363673004/diff/430001/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1363673004/diff/430001/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc#newcode181 components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:181: is_using_lofi_mode = lofi_decider->IsUsingLoFiMode(*request); On 2015/10/16 21:10:07, bengr wrote: > ...
5 years, 2 months ago (2015-10-16 22:06:41 UTC) #36
bengr
lgtm https://codereview.chromium.org/1363673004/diff/430001/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/1363673004/diff/430001/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc#newcode181 components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:181: is_using_lofi_mode = lofi_decider->IsUsingLoFiMode(*request); On 2015/10/16 22:06:40, megjablon wrote: ...
5 years, 2 months ago (2015-10-17 00:06:34 UTC) #37
Lei Zhang
rs lgtm https://codereview.chromium.org/1363673004/diff/430001/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://codereview.chromium.org/1363673004/diff/430001/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc#newcode71 chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:71: scoped_ptr<data_reduction_proxy::LoFiDecider>( you can probably use make_scoped_ptr here. ...
5 years, 2 months ago (2015-10-17 00:40:38 UTC) #38
megjablon
https://chromiumcodereview.appspot.com/1363673004/diff/430001/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://chromiumcodereview.appspot.com/1363673004/diff/430001/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc#newcode71 chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:71: scoped_ptr<data_reduction_proxy::LoFiDecider>( On 2015/10/17 00:40:38, Lei Zhang wrote: > you ...
5 years, 2 months ago (2015-10-19 19:44:21 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363673004/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363673004/470001
5 years, 2 months ago (2015-10-22 02:42:45 UTC) #42
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/85423)
5 years, 2 months ago (2015-10-22 06:44:48 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363673004/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363673004/470001
5 years, 2 months ago (2015-10-22 06:59:16 UTC) #46
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/85461)
5 years, 2 months ago (2015-10-22 10:25:19 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363673004/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363673004/470001
5 years, 2 months ago (2015-10-22 17:44:35 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363673004/510001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363673004/510001
5 years, 2 months ago (2015-10-22 22:47:26 UTC) #54
commit-bot: I haz the power
Committed patchset #17 (id:510001)
5 years, 2 months ago (2015-10-22 23:45:43 UTC) #55
commit-bot: I haz the power
5 years, 2 months ago (2015-10-22 23:46:32 UTC) #56
Message was sent while issue was closed.
Patchset 17 (id:??) landed as
https://crrev.com/61d87091c812e803e060854d24877c513814661b
Cr-Commit-Position: refs/heads/master@{#355674}

Powered by Google App Engine
This is Rietveld 408576698