Chromium Code Reviews
|
|
Chromium Code Reviews
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 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. |
DescriptionAdds 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* #
Messages
Total messages: 73 (48 generated)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dougarnett@chromium.org changed reviewers: + megjablon@chromium.org, sclittle@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping: - megjablon: want l-g-t-m - sclittle: want logic review of UpdatePreviewsStateFromMainFrameResponse() wrt CLIENT_LOFI_ON bit
Left some preliminary comments. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:28: (original_state & SERVER_LOFI_ON)) { I don't think this is quite right. If Server LoFi is disabled (e.g. if the user is in the Server LoFi field trial's control group), but Client LoFi is enabled, then Client LoFi should be used. Maybe change to: if (!(original_state & SERVER_LITE_PAGE_ON) && (original_state & (SERVER_LOFI_ON | CLIENT_LOFI_ON))) { ... } https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:34: // Clear the Lite Page bit if Lite Page transformation did not occur. Looking at the code here, it looks like if the DRP returns a light page AND an "empty-image" page policy, then Server LoFi is allowed to be used if its bit is set. In this situation, Client LoFi should also be allowed to be used if its bit is set. However, currently there's an explicit check in RenderFrameImpl::ShouldUseClientLoFiForRequest() that doesn't allow Client LoFi and Server Lite Pages to be used on the same page. Could you remove that check from ShouldUseClientLoFiForRequest()? We'll also need tests around this. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:46: // Server Lo-Fi not enabled so ensure Client Lo-Fi off for this request. Client LoFi shouldn't be dependent on Server LoFi - if only Server Lite Pages and Client LoFi are enabled, then Server Lite Pages should be able to fall back to Client LoFi. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:55: size_t page_policies_pos = Could you separate this string searching into a helper function? https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:56: chrome_proxy_value.find(kChromeProxyPagePoliciesDirective); Could you just use base::SplitStringPiece here to go through the directives? That way, it'll work case-insensitively and it'll be resilient to whitespace around the '=' sign and stuff. e.g.: for (const auto& directive : base::SplitStringPiece( header, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) { if (!base::StartsWith(directive, kChromeProxyPagePoliciesDirective, base::CompareCase::INSENSITIVE_ASCII)) { continue; } // ... Search for the policy in |directive| ... } https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:60: std::string page_policies_value = chrome_proxy_value.substr( nit: Don't make an unnecessary copy of the std::string here; you can just use base::StringPiece(chrome_proxy_value).substr(...) https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:61: page_policies_pos + strlen(kChromeProxyPagePoliciesDirective) + 1, nit: you can use arraysize or sizeof instead of strlen since it's a char array, plus arraysize can be executed at compile time. Note that arraysize includes the trailing null character, so (strlen() == arraysize() - 1). https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:62: end_pos); string::substr takes in a total number of characters to extract, not an end index. Please fix. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:64: base::SplitString(page_policies_value, "|", base::TRIM_WHITESPACE, nit: use base::SplitStringPiece, not base::SplitString, since there's no need to allocate and copy all the strings here. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:66: if (policy == kChromeProxyEmptyImageDirective) { Use case-insensitive comparison, e.g. base::LowerCaseEqualsASCII.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the good comments Scott. There are a couple points we should discuss. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:28: (original_state & SERVER_LOFI_ON)) { On 2017/06/01 00:59:29, sclittle wrote: > I don't think this is quite right. If Server LoFi is disabled (e.g. if the user > is in the Server LoFi field trial's control group), but Client LoFi is enabled, > then Client LoFi should be used. > > Maybe change to: > if (!(original_state & SERVER_LITE_PAGE_ON) && (original_state & (SERVER_LOFI_ON > | CLIENT_LOFI_ON))) { ... } Adding an early return above if neither server preview is specified which will shortcut this case. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:34: // Clear the Lite Page bit if Lite Page transformation did not occur. On 2017/06/01 00:59:29, sclittle wrote: > Looking at the code here, it looks like if the DRP returns a light page AND an > "empty-image" page policy, then Server LoFi is allowed to be used if its bit is > set. > > In this situation, Client LoFi should also be allowed to be used if its bit is > set. > > However, currently there's an explicit check in > RenderFrameImpl::ShouldUseClientLoFiForRequest() that doesn't allow Client LoFi > and Server Lite Pages to be used on the same page. > > Could you remove that check from ShouldUseClientLoFiForRequest()? We'll also > need tests around this. Let's discuss. It depends on when this method is called. If called for subresource, then I think this check is good (we will clear the LITE_PAGE_ON bit for main frame in order to proceed with Lo-Fi) https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:46: // Server Lo-Fi not enabled so ensure Client Lo-Fi off for this request. On 2017/06/01 00:59:29, sclittle wrote: > Client LoFi shouldn't be dependent on Server LoFi - if only Server Lite Pages > and Client LoFi are enabled, then Server Lite Pages should be able to fall back > to Client LoFi. No, that was not my understanding. I thought that we do not want https-only fallback for proxied pages. We should discuss further if this doesn't sound right to you. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:55: size_t page_policies_pos = On 2017/06/01 00:59:29, sclittle wrote: > Could you separate this string searching into a helper function? Done. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:56: chrome_proxy_value.find(kChromeProxyPagePoliciesDirective); On 2017/06/01 00:59:29, sclittle wrote: > Could you just use base::SplitStringPiece here to go through the directives? > That way, it'll work case-insensitively and it'll be resilient to whitespace > around the '=' sign and stuff. > > e.g.: > > for (const auto& directive : base::SplitStringPiece( > header, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) { > if (!base::StartsWith(directive, kChromeProxyPagePoliciesDirective, > base::CompareCase::INSENSITIVE_ASCII)) { > continue; > } > > // ... Search for the policy in |directive| ... > } Done. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:60: std::string page_policies_value = chrome_proxy_value.substr( On 2017/06/01 00:59:29, sclittle wrote: > nit: Don't make an unnecessary copy of the std::string here; you can just use > base::StringPiece(chrome_proxy_value).substr(...) Done. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:61: page_policies_pos + strlen(kChromeProxyPagePoliciesDirective) + 1, On 2017/06/01 00:59:29, sclittle wrote: > nit: you can use arraysize or sizeof instead of strlen since it's a char array, > plus arraysize can be executed at compile time. > > Note that arraysize includes the trailing null character, so (strlen() == > arraysize() - 1). Done. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:62: end_pos); On 2017/06/01 00:59:29, sclittle wrote: > string::substr takes in a total number of characters to extract, not an end > index. Please fix. Acknowledged. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:64: base::SplitString(page_policies_value, "|", base::TRIM_WHITESPACE, On 2017/06/01 00:59:29, sclittle wrote: > nit: use base::SplitStringPiece, not base::SplitString, since there's no need to > allocate and copy all the strings here. Done. https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:66: if (policy == kChromeProxyEmptyImageDirective) { On 2017/06/01 00:59:29, sclittle wrote: > Use case-insensitive comparison, e.g. base::LowerCaseEqualsASCII. Done.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated ShouldUseClientLoFiForRequest() https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/20001/content/renderer/previe... content/renderer/previews_state_helper.cc:34: // Clear the Lite Page bit if Lite Page transformation did not occur. On 2017/06/01 00:59:29, sclittle wrote: > Looking at the code here, it looks like if the DRP returns a light page AND an > "empty-image" page policy, then Server LoFi is allowed to be used if its bit is > set. > > In this situation, Client LoFi should also be allowed to be used if its bit is > set. > > However, currently there's an explicit check in > RenderFrameImpl::ShouldUseClientLoFiForRequest() that doesn't allow Client LoFi > and Server Lite Pages to be used on the same page. > > Could you remove that check from ShouldUseClientLoFiForRequest()? We'll also > need tests around this. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Now that the ContentLoFiDecider change has landed, I was able to add a Fallback integration test to this CL to verify overall that page-polices getting handled properly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping Megan and Scott. I would like to be sure this looks good on our side and then get a content owner added to the review.
https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:9: namespace { move the anonymous namespace inside content namespace https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:19: bool HasEmptyPageDirective(const blink::WebURLResponse& web_url_response) { s/HasEmptyPageDirective/HasEmptyImageDirective/? https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:20: std::string chrome_proxy_value = #include <string> https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:22: .HttpHeaderField(blink::WebString::FromUTF8(kChromeProxyHeader)) #include "third_party/WebKit/public/platform/WebString.h" https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:27: if (!base::StartsWith(directive, kChromeProxyPagePoliciesDirective, #include "base/strings/string_util.cc" https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:33: base::StringPiece page_policies_value = base::StringPiece(directive).substr( #include "base/strings/string_piece.h" https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:34: arraysize(kChromeProxyPagePoliciesDirective)); #include "src/base/macros.h" https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:56: return original_state; if statement is more than two lines, add braces. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:60: (original_state & SERVER_LOFI_ON)) { Just want to make sure, in the updated protocol can we ever end up in a state where Lo-Fi is enabled, but not Lite Page, for the main frame? https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:64: // At this point, we have a proxy main frame response for which we want Don't use we. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:81: if (!(updated_state & SERVER_LOFI_ON)) { Even though it's the same, I'd use original_state since we want to check that the original had server lofi on. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:87: Can we check here if we cleared all the bits and if so return PREVIEWS_OFF instead of PREVIEWS_UNSPECIFIED? Also, maybe we should do that at the top too. We can immediately check if PREVIEWS_UNSPECIFIED and return PREVIEWS_OFF. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... File content/renderer/previews_state_helper.h (right): https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.h:15: CONTENT_EXPORT PreviewsState UpdatePreviewsStateFromMainFrameResponse( Where are we calling this that it needs a CONTENT_EXPORT? https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... File content/renderer/previews_state_helper_unittest.cc (right): https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper_unittest.cc:10: namespace {} // namespaces Remove https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper_unittest.cc:13: blink::WebURLResponse response_no_headers; update your #includes https://codereview.chromium.org/2910783002/diff/80001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lite_page.py (right): https://codereview.chromium.org/2910783002/diff/80001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lite_page.py:100: # Lo-Fi fallback is not currently supported via the client. Check that Update this to "Lo-Fi fallback is not supported without the DataReductionProxyDecidesTransform feature. Check that no Lo-Fi response is received if a Lite Page is not served." https://codereview.chromium.org/2910783002/diff/80001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lite_page.py:139: def testLitePageFallbackViaPagePolicies(self): Please also add tests with DataReductionProxyDecidesTransform feature on for forcing a lite page via the flag and for forcing Lo-Fi enabled with the flag
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:9: namespace { On 2017/06/05 23:47:58, megjablon wrote: > move the anonymous namespace inside content namespace Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:19: bool HasEmptyPageDirective(const blink::WebURLResponse& web_url_response) { On 2017/06/05 23:47:59, megjablon wrote: > s/HasEmptyPageDirective/HasEmptyImageDirective/? Yuck! Thanks! https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:20: std::string chrome_proxy_value = On 2017/06/05 23:47:59, megjablon wrote: > #include <string> Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:22: .HttpHeaderField(blink::WebString::FromUTF8(kChromeProxyHeader)) On 2017/06/05 23:47:58, megjablon wrote: > #include "third_party/WebKit/public/platform/WebString.h" Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:27: if (!base::StartsWith(directive, kChromeProxyPagePoliciesDirective, On 2017/06/05 23:47:58, megjablon wrote: > #include "base/strings/string_util.cc" Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:33: base::StringPiece page_policies_value = base::StringPiece(directive).substr( On 2017/06/05 23:47:59, megjablon wrote: > #include "base/strings/string_piece.h" Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:34: arraysize(kChromeProxyPagePoliciesDirective)); On 2017/06/05 23:47:58, megjablon wrote: > #include "src/base/macros.h" Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:56: return original_state; On 2017/06/05 23:47:58, megjablon wrote: > if statement is more than two lines, add braces. Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:60: (original_state & SERVER_LOFI_ON)) { On 2017/06/05 23:47:58, megjablon wrote: > Just want to make sure, in the updated protocol can we ever end up in a state > where Lo-Fi is enabled, but not Lite Page, for the main frame? We don't want to since SERVER_LITE_PAGE_ON => accept lite-page => server previews enabled for page. The server takes responsibility for the country restrictions and will handle the fallback to Lo-Fi. This is not true of the current ShouldAcceptLitePages though. The other open CL will address this. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:64: // At this point, we have a proxy main frame response for which we want On 2017/06/05 23:47:58, megjablon wrote: > Don't use we. Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:81: if (!(updated_state & SERVER_LOFI_ON)) { On 2017/06/05 23:47:59, megjablon wrote: > Even though it's the same, I'd use original_state since we want to check that > the original had server lofi on. Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.cc:87: On 2017/06/05 23:47:58, megjablon wrote: > Can we check here if we cleared all the bits and if so return PREVIEWS_OFF > instead of PREVIEWS_UNSPECIFIED? > > Also, maybe we should do that at the top too. We can immediately check if > PREVIEWS_UNSPECIFIED and return PREVIEWS_OFF. Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... File content/renderer/previews_state_helper.h (right): https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper.h:15: CONTENT_EXPORT PreviewsState UpdatePreviewsStateFromMainFrameResponse( On 2017/06/05 23:47:59, megjablon wrote: > Where are we calling this that it needs a CONTENT_EXPORT? It was not exposed for the unittests. Is there better/reduced way to get it exposed for them? https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... File content/renderer/previews_state_helper_unittest.cc (right): https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper_unittest.cc:10: namespace {} // namespaces On 2017/06/05 23:47:59, megjablon wrote: > Remove Done. https://codereview.chromium.org/2910783002/diff/80001/content/renderer/previe... content/renderer/previews_state_helper_unittest.cc:13: blink::WebURLResponse response_no_headers; On 2017/06/05 23:47:59, megjablon wrote: > update your #includes Done. https://codereview.chromium.org/2910783002/diff/80001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lite_page.py (right): https://codereview.chromium.org/2910783002/diff/80001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lite_page.py:100: # Lo-Fi fallback is not currently supported via the client. Check that On 2017/06/05 23:47:59, megjablon wrote: > Update this to "Lo-Fi fallback is not supported without the > DataReductionProxyDecidesTransform feature. Check that no Lo-Fi response is > received if a Lite Page is not served." Done. https://codereview.chromium.org/2910783002/diff/80001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lite_page.py:139: def testLitePageFallbackViaPagePolicies(self): On 2017/06/05 23:47:59, megjablon wrote: > Please also add tests with DataReductionProxyDecidesTransform feature on for > forcing a lite page via the flag and for forcing Lo-Fi enabled with the flag In different CL? I don't see how these tests would apply to this change. I have several more integration tests that I want to add (some already coded) - pending server support pushing out to prod (also force_empty_image is not yet implemented by the server but initial test for it verifying client sends force exp is in another CL).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dougarnett@chromium.org changed reviewers: + nasko@chromium.org
Nasko, are you able to review this RenderFrameImpl change? This adds back the same type of hook that megjablon added in https://codereview.chromium.org/2642793005/ (but temporarily pulled back out in https://codereview.chromium.org/2856223008/). The logic though now is a bit more complicated, so this creates a helper method to contain it.
https://codereview.chromium.org/2910783002/diff/80001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lite_page.py (right): https://codereview.chromium.org/2910783002/diff/80001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lite_page.py:139: def testLitePageFallbackViaPagePolicies(self): On 2017/06/06 16:49:46, dougarnett wrote: > On 2017/06/05 23:47:59, megjablon wrote: > > Please also add tests with DataReductionProxyDecidesTransform feature on for > > forcing a lite page via the flag and for forcing Lo-Fi enabled with the flag > > In different CL? I don't see how these tests would apply to this change. I have > several more integration tests that I want to add (some already coded) - pending > server support pushing out to prod (also force_empty_image is not yet > implemented by the server but initial test for it verifying client sends force > exp is in another CL). I went ahead and added my integration tests here for getting lite page for slow connection but not for fast connection with new feature on. These work now against staging but not yet for prod server (as ECT checking change has not yet pushed to prod - should by end of week).
LGTM https://codereview.chromium.org/2910783002/diff/120001/content/renderer/previ... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/120001/content/renderer/previ... content/renderer/previews_state_helper.cc:71: (original_state & SERVER_LOFI_ON)) { nit: You could combine the previous if statement and this one here into: if (!(original_state & SERVER_LITE_PAGE_ON)) { return original_state; } You're welcome to keep them separate though if you think it significantly improves readability.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
https://codereview.chromium.org/2910783002/diff/120001/content/renderer/previ... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/120001/content/renderer/previ... content/renderer/previews_state_helper.cc:71: (original_state & SERVER_LOFI_ON)) { On 2017/06/07 20:31:57, sclittle wrote: > nit: You could combine the previous if statement and this one here into: > > if (!(original_state & SERVER_LITE_PAGE_ON)) { > return original_state; > } > > You're welcome to keep them separate though if you think it significantly > improves readability. Good point. I had this separate so that it will be clear to see this case and then remove this check in M-62. But your suggestion seems better now.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ cannot get SignCLA result. Please try later.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ryansturm@chromium.org changed reviewers: + ryansturm@chromium.org
Just a question so I can understand a little better https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... content/renderer/previews_state_helper.cc:85: if (!(original_state & SERVER_LOFI_ON)) { What about HTTPS pages? Would client lofi be allowed for HTTPS pages (would server lofi be allowed for HTTPS pages)? Asking because I don't know :)
https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... content/renderer/previews_state_helper.cc:85: if (!(original_state & SERVER_LOFI_ON)) { On 2017/06/09 17:59:03, RyanSturm wrote: > What about HTTPS pages? Would client lofi be allowed for HTTPS pages (would > server lofi be allowed for HTTPS pages)? > > Asking because I don't know :) HTTPS pages should not reach here (return at line 65 because SERVER_LITE_PAGE_ON bit not set) => so client LoFi not affected here for HTTPS (so if enabled, will happen). We don't have any support (nor definition as far as I know) for server LoFi on HTTPS pages but we should raise with Scott. It may be good topic to visit later if we see meaningful opportunity.
https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... content/renderer/previews_state_helper.cc:85: if (!(original_state & SERVER_LOFI_ON)) { On 2017/06/09 18:39:18, dougarnett wrote: > On 2017/06/09 17:59:03, RyanSturm wrote: > > What about HTTPS pages? Would client lofi be allowed for HTTPS pages (would > > server lofi be allowed for HTTPS pages)? > > > > Asking because I don't know :) > > HTTPS pages should not reach here (return at line 65 because SERVER_LITE_PAGE_ON > bit not set) => so client LoFi not affected here for HTTPS (so if enabled, will > happen). > > We don't have any support (nor definition as far as I know) for server LoFi on > HTTPS pages but we should raise with Scott. It may be good topic to visit later > if we see meaningful opportunity. I see, thanks!
https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... content/renderer/previews_state_helper.cc:85: if (!(original_state & SERVER_LOFI_ON)) { On 2017/06/09 18:41:33, RyanSturm wrote: > On 2017/06/09 18:39:18, dougarnett wrote: > > On 2017/06/09 17:59:03, RyanSturm wrote: > > > What about HTTPS pages? Would client lofi be allowed for HTTPS pages (would > > > server lofi be allowed for HTTPS pages)? > > > > > > Asking because I don't know :) > > > > HTTPS pages should not reach here (return at line 65 because > SERVER_LITE_PAGE_ON > > bit not set) => so client LoFi not affected here for HTTPS (so if enabled, > will > > happen). > > > > We don't have any support (nor definition as far as I know) for server LoFi on > > HTTPS pages but we should raise with Scott. It may be good topic to visit > later > > if we see meaningful opportunity. > > I see, thanks! Client LoFi will automatically handle all images (regardless of http:// or https://) on https:// pages if Server LoFi isn't enabled, plus http:// images on https:// pages should be extremely rare, so this should just be fine.
lgtm % comment https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... File content/renderer/previews_state_helper_unittest.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... content/renderer/previews_state_helper_unittest.cc:48: TEST(PreviewsStateHelperTest, UpdatePreviewsStateLitePageTransform) { Add a test for a lite page transform that has lo-fi page policy since we support that now if the server tells us to.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dougarnett@chromium.org changed reviewers: - ryansturm@chromium.org
https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... File content/renderer/previews_state_helper_unittest.cc (right): https://codereview.chromium.org/2910783002/diff/160001/content/renderer/previ... content/renderer/previews_state_helper_unittest.cc:48: TEST(PreviewsStateHelperTest, UpdatePreviewsStateLitePageTransform) { On 2017/06/09 19:42:19, megjablon wrote: > Add a test for a lite page transform that has lo-fi page policy since we support > that now if the server tells us to. Done.
Description was changed from ========== 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. BUG=701802 ========== to ========== 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 ==========
Hi Nasko, This CL is now just waiting on content owner review. Looks like you may have busy travel schedule. Any chance you could give it a look on Monday or so? Or can you suggest an alternate content reviewer?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
https://codereview.chromium.org/2910783002/diff/180001/content/renderer/previ... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/180001/content/renderer/previ... content/renderer/previews_state_helper.cc:56: PreviewsState UpdatePreviewsStateFromMainFrameResponse( Since this method doesn't actually update any of the parameters, isn't it more correct to call it GetPreviewState...? It is also more consistent with a function that returns just a simple value based on input parameters. https://codereview.chromium.org/2910783002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2910783002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:3614: extra_data->previews_state(), web_url_response); Why not pass in only web_url_response? The extra_data comes from there as well and can easily be made unconditional, where null extra_data returns PREVIEWS_OFF.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2910783002/diff/180001/content/renderer/previ... File content/renderer/previews_state_helper.cc (right): https://codereview.chromium.org/2910783002/diff/180001/content/renderer/previ... content/renderer/previews_state_helper.cc:56: PreviewsState UpdatePreviewsStateFromMainFrameResponse( On 2017/06/09 22:51:34, nasko (slow) wrote: > Since this method doesn't actually update any of the parameters, isn't it more > correct to call it GetPreviewState...? It is also more consistent with a > function that returns just a simple value based on input parameters. Done. https://codereview.chromium.org/2910783002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2910783002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:3614: extra_data->previews_state(), web_url_response); On 2017/06/09 22:51:34, nasko (slow) wrote: > Why not pass in only web_url_response? The extra_data comes from there as well > and can easily be made unconditional, where null extra_data returns > PREVIEWS_OFF. I explored this a bit. I still need to determine extra_data here for the effective_connection_type determination so it ends up looking like: if (is_main_frame_ && !navigation_state->WasWithinSameDocument()) { previews_state_ = GetPreviewsStateFromMainFrameResponse(web_url_response); WebURLResponseExtraDataImpl* extra_data = GetExtraDataFromResponse(web_url_response); if (extra_data) { effective_connection_type_ = EffectiveConnectionTypeToWebEffectiveConnectionType( extra_data->effective_connection_type()); } } Also, the GetExtraDataFromResponse() helper is local to this file so I am not sure I want to extract extra_data in two places (and have both places know WebURLResponseExtraDataImpl). WDYT? Should I proceed with extracting extra_data in both places? [An opposite way to go, would be to pull the header map out of the response (not exposed currently though) and then pass the previews state and header map into the helper.]
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2910783002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2910783002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:3614: extra_data->previews_state(), web_url_response); On 2017/06/10 00:07:27, dougarnett wrote: > On 2017/06/09 22:51:34, nasko (slow) wrote: > > Why not pass in only web_url_response? The extra_data comes from there as well > > and can easily be made unconditional, where null extra_data returns > > PREVIEWS_OFF. > > I explored this a bit. I still need to determine extra_data here for the > effective_connection_type determination so it ends up looking like: > > if (is_main_frame_ && !navigation_state->WasWithinSameDocument()) { > previews_state_ = GetPreviewsStateFromMainFrameResponse(web_url_response); > WebURLResponseExtraDataImpl* extra_data = > GetExtraDataFromResponse(web_url_response); > if (extra_data) { > effective_connection_type_ = > EffectiveConnectionTypeToWebEffectiveConnectionType( > extra_data->effective_connection_type()); > } > } > > Also, the GetExtraDataFromResponse() helper is local to this file so I am not > sure I want to extract extra_data in two places (and have both places know > WebURLResponseExtraDataImpl). > > WDYT? Should I proceed with extracting extra_data in both places? > > > > [An opposite way to go, would be to pull the header map out of the response (not > exposed currently though) and then pass the previews state and header map into > the helper.] Thanks for looking into this. It is fine to stay as it is.
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sclittle@chromium.org, megjablon@chromium.org Link to the patchset: https://codereview.chromium.org/2910783002/#ps200001 (title: "Renamed helper method from Update* to Get*")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1497302991187580,
"parent_rev": "837b32b0bc91016102f09448259fb6048516f0a1", "commit_rev":
"e662928d521c748ad5a76aeaf5ce4d6037740013"}
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1497302991187580,
"parent_rev": "b815cd6600b8b299285369ae798439c02cfaced9", "commit_rev":
"55ef5301fc7f49a2eda5b84b12d816ff051dbfb5"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/55ef5301fc7f49a2eda5b84b12d8... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/55ef5301fc7f49a2eda5b84b12d8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
