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

Issue 2910783002: Adds Lo-Fi fallback support for new Data Reduction Proxy protocol. (Closed)

Created:
3 years, 6 months ago by dougarnett
Modified:
3 years, 6 months ago
Reviewers:
megjablon, nasko, sclittle
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, mlamouri+watch-content_chromium.org, RyanSturm
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Adds Lo-Fi fallback support for new Data Reduction Proxy protocol. Background: the original client-based preview fallback support that existed in RenderFrameImpl::SendDidCommitProvisionalLoad for main frame responses was removed in CL 2856223008. This CL now adds fallback support back in at the same call site but introduces a helper method to better isolate the (now more complicated) logic with unittests. This CL also adds integration tests that cover ECT header test coverage requested in bug 691135. BUG=701802, 691135 Review-Url: https://codereview.chromium.org/2910783002 Cr-Commit-Position: refs/heads/master@{#478812} Committed: https://chromium.googlesource.com/chromium/src/+/55ef5301fc7f49a2eda5b84b12d816ff051dbfb5

Patch Set 1 #

Patch Set 2 : Added support for legacy Lo-Fi path (without Lite-Page enabled). #

Total comments: 21

Patch Set 3 : Improvements from sclittle feedback #

Patch Set 4 : Removed LITE_PAGE_ON check from RenderFrameImpl::ShouldUseClientLoFiForRequest #

Patch Set 5 : Added integration test the excercises page-polices fallback #

Total comments: 35

Patch Set 6 : Updates for Megan's feedback #

Patch Set 7 : Added Lite Page integration tests for slow connection and fast connection #

Total comments: 2

Patch Set 8 : Updated with sclittle's suggestion #

Patch Set 9 : Makes SlowConnection integration test more permissive in which type of server optimization response… #

Total comments: 6

Patch Set 10 : Added unittest for server providing lite page and also directing page-policy #

Total comments: 5

Patch Set 11 : Renamed helper method from Update* to Get* #

Unified diffs Side-by-side diffs Delta from patch set Stats (+394 lines, -20 lines) Patch
M content/public/common/previews_state.h View 1 chunk +0 lines, -6 lines 0 comments Download
M content/renderer/BUILD.gn View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
A content/renderer/previews_state_helper.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +21 lines, -0 lines 0 comments Download
A content/renderer/previews_state_helper.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +100 lines, -0 lines 0 comments Download
A content/renderer/previews_state_helper_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +146 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +5 lines, -5 lines 0 comments Download
M content/renderer/render_frame_impl_browsertest.cc View 1 2 3 1 chunk +6 lines, -6 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M tools/chrome_proxy/webdriver/lite_page.py View 1 2 3 4 5 6 7 8 2 chunks +113 lines, -3 lines 0 comments Download

Messages

Total messages: 73 (48 generated)
dougarnett
3 years, 6 months ago (2017-05-26 22:34:48 UTC) #4
dougarnett
Ping: - megjablon: want l-g-t-m - sclittle: want logic review of UpdatePreviewsStateFromMainFrameResponse() wrt CLIENT_LOFI_ON bit
3 years, 6 months ago (2017-05-31 23:44:13 UTC) #11
sclittle
Left some preliminary comments. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previews_state_helper.cc#newcode28 content/renderer/previews_state_helper.cc:28: (original_state & SERVER_LOFI_ON)) { I ...
3 years, 6 months ago (2017-06-01 00:59:29 UTC) #12
dougarnett
Thanks for the good comments Scott. There are a couple points we should discuss. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previews_state_helper.cc ...
3 years, 6 months ago (2017-06-01 20:53:12 UTC) #15
dougarnett
Updated ShouldUseClientLoFiForRequest() https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previews_state_helper.cc#newcode34 content/renderer/previews_state_helper.cc:34: // Clear the Lite Page bit if ...
3 years, 6 months ago (2017-06-01 22:05:10 UTC) #18
dougarnett
Now that the ContentLoFiDecider change has landed, I was able to add a Fallback integration ...
3 years, 6 months ago (2017-06-02 22:28:31 UTC) #23
dougarnett
Ping Megan and Scott. I would like to be sure this looks good on our ...
3 years, 6 months ago (2017-06-05 16:08:05 UTC) #26
megjablon
https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previews_state_helper.cc#newcode9 content/renderer/previews_state_helper.cc:9: namespace { move the anonymous namespace inside content namespace ...
3 years, 6 months ago (2017-06-05 23:47:59 UTC) #27
dougarnett
https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previews_state_helper.cc#newcode9 content/renderer/previews_state_helper.cc:9: namespace { On 2017/06/05 23:47:58, megjablon wrote: > move ...
3 years, 6 months ago (2017-06-06 16:49:47 UTC) #30
dougarnett
Nasko, are you able to review this RenderFrameImpl change? This adds back the same type ...
3 years, 6 months ago (2017-06-06 18:49:05 UTC) #34
dougarnett
https://codereview.chromium.org/2910783002/diff/80001/tools/chrome_proxy/webdriver/lite_page.py File tools/chrome_proxy/webdriver/lite_page.py (right): https://codereview.chromium.org/2910783002/diff/80001/tools/chrome_proxy/webdriver/lite_page.py#newcode139 tools/chrome_proxy/webdriver/lite_page.py:139: def testLitePageFallbackViaPagePolicies(self): On 2017/06/06 16:49:46, dougarnett wrote: > On ...
3 years, 6 months ago (2017-06-07 16:57:49 UTC) #35
sclittle
LGTM https://codereview.chromium.org/2910783002/diff/120001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/120001/content/renderer/previews_state_helper.cc#newcode71 content/renderer/previews_state_helper.cc:71: (original_state & SERVER_LOFI_ON)) { nit: You could combine ...
3 years, 6 months ago (2017-06-07 20:31:57 UTC) #36
dougarnett
https://codereview.chromium.org/2910783002/diff/120001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/120001/content/renderer/previews_state_helper.cc#newcode71 content/renderer/previews_state_helper.cc:71: (original_state & SERVER_LOFI_ON)) { On 2017/06/07 20:31:57, sclittle wrote: ...
3 years, 6 months ago (2017-06-07 20:56:11 UTC) #38
RyanSturm
Just a question so I can understand a little better https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper.cc#newcode85 ...
3 years, 6 months ago (2017-06-09 17:59:03 UTC) #47
dougarnett
https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper.cc#newcode85 content/renderer/previews_state_helper.cc:85: if (!(original_state & SERVER_LOFI_ON)) { On 2017/06/09 17:59:03, RyanSturm ...
3 years, 6 months ago (2017-06-09 18:39:18 UTC) #48
RyanSturm
https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper.cc#newcode85 content/renderer/previews_state_helper.cc:85: if (!(original_state & SERVER_LOFI_ON)) { On 2017/06/09 18:39:18, dougarnett ...
3 years, 6 months ago (2017-06-09 18:41:34 UTC) #49
sclittle
https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper.cc#newcode85 content/renderer/previews_state_helper.cc:85: if (!(original_state & SERVER_LOFI_ON)) { On 2017/06/09 18:41:33, RyanSturm ...
3 years, 6 months ago (2017-06-09 19:33:11 UTC) #50
megjablon
lgtm % comment https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper_unittest.cc File content/renderer/previews_state_helper_unittest.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper_unittest.cc#newcode48 content/renderer/previews_state_helper_unittest.cc:48: TEST(PreviewsStateHelperTest, UpdatePreviewsStateLitePageTransform) { Add a test ...
3 years, 6 months ago (2017-06-09 19:42:19 UTC) #51
dougarnett
https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper_unittest.cc File content/renderer/previews_state_helper_unittest.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previews_state_helper_unittest.cc#newcode48 content/renderer/previews_state_helper_unittest.cc:48: TEST(PreviewsStateHelperTest, UpdatePreviewsStateLitePageTransform) { On 2017/06/09 19:42:19, megjablon wrote: > ...
3 years, 6 months ago (2017-06-09 20:09:18 UTC) #55
dougarnett
Hi Nasko, This CL is now just waiting on content owner review. Looks like you ...
3 years, 6 months ago (2017-06-09 21:34:17 UTC) #57
nasko
https://codereview.chromium.org/2910783002/diff/180001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/180001/content/renderer/previews_state_helper.cc#newcode56 content/renderer/previews_state_helper.cc:56: PreviewsState UpdatePreviewsStateFromMainFrameResponse( Since this method doesn't actually update any ...
3 years, 6 months ago (2017-06-09 22:51:35 UTC) #60
dougarnett
https://codereview.chromium.org/2910783002/diff/180001/content/renderer/previews_state_helper.cc File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/180001/content/renderer/previews_state_helper.cc#newcode56 content/renderer/previews_state_helper.cc:56: PreviewsState UpdatePreviewsStateFromMainFrameResponse( On 2017/06/09 22:51:34, nasko (slow) wrote: > ...
3 years, 6 months ago (2017-06-10 00:07:27 UTC) #63
nasko
LGTM https://codereview.chromium.org/2910783002/diff/180001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2910783002/diff/180001/content/renderer/render_frame_impl.cc#newcode3614 content/renderer/render_frame_impl.cc:3614: extra_data->previews_state(), web_url_response); On 2017/06/10 00:07:27, dougarnett wrote: > ...
3 years, 6 months ago (2017-06-12 17:38:50 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2910783002/200001
3 years, 6 months ago (2017-06-12 21:30:27 UTC) #69
commit-bot: I haz the power
3 years, 6 months ago (2017-06-12 23:10:23 UTC) #73
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/55ef5301fc7f49a2eda5b84b12d8...

Powered by Google App Engine
This is Rietveld 408576698