|
|
Created:
3 years, 7 months ago by dougarnett Modified:
3 years, 6 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew CPAT support in ContentLoFiDecider guarded by feature flag.
Updates MaybeSetAcceptTransformHeader to set accept header based
only on PreviewsState (if new feature enabled) vs. checking various
other flags. Also new path will not add "ifheavy" qualifier.
This is step 2 of 4 in adding new CPAT protocol support.
Step 1 is in CL 2889993004.
Step 3 is in CL 2903453003.
BUG=701802
Review-Url: https://codereview.chromium.org/2911673002
Cr-Commit-Position: refs/heads/master@{#476678}
Committed: https://chromium.googlesource.com/chromium/src/+/f134cffafa9fca0640bacbb56d9de9af0a8f25aa
Patch Set 1 #Patch Set 2 : FIxed a comment #
Total comments: 14
Patch Set 3 : Address some of megjablon's comments #Patch Set 4 : Added comment to help clarify the new empty-image accept path #Patch Set 5 : Improved unittests per feedback #
Total comments: 6
Patch Set 6 : Further improved unittests #Patch Set 7 : Backed out some debug code #
Messages
Total messages: 39 (29 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...
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: + bengr@chromium.org, megjablon@chromium.org
Description was changed from ========== New CPAT support in ContentLoFiDecider guarded by feature flag. Updates MaybeSetAcceptTransformHeader to set accept header based only on PreviewsState (if new feature enabled) vs. checking various other flags. Also new path will not add "ifheavy" qualifier. This is step 2 of 4 in adding new CPAT protocol support. Step 1 is in CL 2889993004. BUG=701802 ========== to ========== New CPAT support in ContentLoFiDecider guarded by feature flag. Updates MaybeSetAcceptTransformHeader to set accept header based only on PreviewsState (if new feature enabled) vs. checking various other flags. Also new path will not add "ifheavy" qualifier. This is step 2 of 4 in adding new CPAT protocol support. Step 1 is in CL 2889993004. Step 3 is in CL 2903453003. BUG=701802 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:73: content::PreviewsState previews_state = request_info->GetPreviewsState(); I'd move this line above the comment. https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:74: if (previews_state & content::PREVIEWS_NO_TRANSFORM) { If you want, you can remove the brackets here. https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:78: // For the proxy-decides-transform feature, we determine whether to set Don't use we. "the header is set based only on the request's PreviewsState" https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:85: bool decide_by_previews_state = base::FeatureList::IsEnabled( Maybe call this check_previews_flags and use the inverse since we still use the previews state in the old world and this implies that we don't. bool check_previews_flags = !base::FeatureList::IsEnabled( Also import src/base/feature_list.h https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:127: resource_type == content::RESOURCE_TYPE_MAIN_FRAME) { Do we want: if (previews_state & content::SERVER_LITE_PAGE_ON) { if (resource_type == content::RESOURCE_TYPE_MAIN_FRAME) accept_transform_value = lite_page_directive(); } } With what you have here we have to guarantee the Lo-Fi bit is never on if we get a preview back. With the above option we have to guarantee that the lite page bit stays on if we get a preview back. I think I'm in favor of the later, but could be convinced otherwise and either way we should guarantee both in the content code. If we keep what you have here then we could do Lo-Fi images on a lite page by only changing the bits, which may be the policy we want at some point. https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc (right): https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:311: TEST_F(ContentLoFiDeciderTest, MaybeSetAcceptTransformHeaderProxyDecides) { It looks like there's a lot of repeated code in this test. Is there any way we can use a struct for it?
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 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/2911673002/diff/20001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:73: content::PreviewsState previews_state = request_info->GetPreviewsState(); On 2017/05/30 18:38:53, megjablon wrote: > I'd move this line above the comment. Done. https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:74: if (previews_state & content::PREVIEWS_NO_TRANSFORM) { On 2017/05/30 18:38:53, megjablon wrote: > If you want, you can remove the brackets here. Acknowledged. https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:78: // For the proxy-decides-transform feature, we determine whether to set On 2017/05/30 18:38:53, megjablon wrote: > Don't use we. "the header is set based only on the request's PreviewsState" Done. https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:85: bool decide_by_previews_state = base::FeatureList::IsEnabled( On 2017/05/30 18:38:53, megjablon wrote: > Maybe call this check_previews_flags and use the inverse since we still use the > previews state in the old world and this implies that we don't. > > bool check_previews_flags = !base::FeatureList::IsEnabled( > > Also import src/base/feature_list.h Done. https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:127: resource_type == content::RESOURCE_TYPE_MAIN_FRAME) { On 2017/05/30 18:38:53, megjablon wrote: > Do we want: > > if (previews_state & content::SERVER_LITE_PAGE_ON) { > if (resource_type == content::RESOURCE_TYPE_MAIN_FRAME) > accept_transform_value = lite_page_directive(); > } > } > > With what you have here we have to guarantee the Lo-Fi bit is never on if we get > a preview back. With the above option we have to guarantee that the lite page > bit stays on if we get a preview back. I think I'm in favor of the later, but > could be convinced otherwise and either way we should guarantee both in the > content code. If we keep what you have here then we could do Lo-Fi images on a > lite page by only changing the bits, which may be the policy we want at some > point. Discussed in person. I favor the client not wiring together a constraint that Lo-Fi can't happen if we got a lite page. Instead if we can separate and simplify the logic to keep/clear Lo-Fi bit based on Server's page-policies, then it would be possible for the server to do both without the client blocking that combination (new HD preview + Lo-Fi maybe?). For reference, the logic for updating the Previews bits from what is enabled by the client to what is decided by the server is in CL 2910783002 (RenderFrameImpl calling PreviewsStateHelper method). It is a bit different from the emailed logic (to separately decide whether to clear/keep LO_FI bits (based on page-policies) from clearing/keeping the LITE_PAGE bit). https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc (right): https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:311: TEST_F(ContentLoFiDeciderTest, MaybeSetAcceptTransformHeaderProxyDecides) { On 2017/05/30 18:38:53, megjablon wrote: > It looks like there's a lot of repeated code in this test. Is there any way we > can use a struct for it? In my previous mondo CL, I had used the struct approach and Ben commented to not take that approach (which I agree with). Perhaps this would be an interesting example to discuss trade-offs with the struct approach.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc (right): https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:311: TEST_F(ContentLoFiDeciderTest, MaybeSetAcceptTransformHeaderProxyDecides) { On 2017/05/30 22:15:48, dougarnett wrote: > On 2017/05/30 18:38:53, megjablon wrote: > > It looks like there's a lot of repeated code in this test. Is there any way we > > can use a struct for it? > > In my previous mondo CL, I had used the struct approach and Ben commented to not > take that approach (which I agree with). > Perhaps this would be an interesting example to discuss trade-offs > with the struct approach. I don't like the pattern of using an array of structs to handle various test cases. Imo, each logical struct instance should be its own test. However I do agree with Megan that there is a lot of code repetition. Can each block call the same helper method? Something like VerifyStuff(...)? Also, unless these blocks need to be in series, I'd break them into separate tests.
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/2911673002/diff/20001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc (right): https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:311: TEST_F(ContentLoFiDeciderTest, MaybeSetAcceptTransformHeaderProxyDecides) { On 2017/05/31 18:48:28, bengr wrote: > On 2017/05/30 22:15:48, dougarnett wrote: > > On 2017/05/30 18:38:53, megjablon wrote: > > > It looks like there's a lot of repeated code in this test. Is there any way > we > > > can use a struct for it? > > > > In my previous mondo CL, I had used the struct approach and Ben commented to > not > > take that approach (which I agree with). > > Perhaps this would be an interesting example to discuss trade-offs > > with the struct approach. > > I don't like the pattern of using an array of structs to handle various test > cases. Imo, each logical struct instance should be its own test. However I do > agree with Megan that there is a lot of code repetition. Can each block call the > same helper method? Something like VerifyStuff(...)? Also, unless these blocks > need to be in series, I'd break them into separate tests. Ok, broke up into 4 difference tests with simplified verify method.
https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:72: content::PreviewsState previews_state = request_info->GetPreviewsState(); Replace request_info->GetPreviewsState() calls below in existing code with previews_state. https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:103: // Previews has been disabled. nit: Previews have? https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc (right): https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:196: Can we move even more of the repetitive code: static void VerifyAcceptTransformHeader( content::ResourceType resource_type, bool is_https, content::PreviewsState previews_state, bool expected_accept_lite_page, bool expected_accept_empty_image) { std::unique_ptr<net::URLRequest> request = CreateRequestByType(resource_type, is_https, previews_state); net::HttpRequestHeaders headers; lofi_decider->MaybeSetAcceptTransformHeader(*request.get(), false, &headers); std::string header_value; EXPECT_EQ(expected_accept_lite_page || expected_accept_empty_image, headers.GetHeader(chrome_proxy_accept_transform_header(), &header_value)); if (expected_accept_lite_page) { EXPECT_TRUE(header_value == lite_page_directive()); } else if (expected_accept_empty_image) { EXPECT_TRUE(header_value == empty_image_directive()); } else { EXPECT_FALSE(headers.HasHeader(chrome_proxy_accept_transform_header())); } }
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...
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 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/2911673002/diff/80001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:72: content::PreviewsState previews_state = request_info->GetPreviewsState(); On 2017/06/01 00:06:21, megjablon wrote: > Replace request_info->GetPreviewsState() calls below in existing code with > previews_state. Done. https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:103: // Previews has been disabled. On 2017/06/01 00:06:21, megjablon wrote: > nit: Previews have? Done. https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc (right): https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:196: On 2017/06/01 00:06:21, megjablon wrote: > Can we move even more of the repetitive code: > > static void VerifyAcceptTransformHeader( > content::ResourceType resource_type, > bool is_https, > content::PreviewsState previews_state, > bool expected_accept_lite_page, > bool expected_accept_empty_image) { > std::unique_ptr<net::URLRequest> request = > CreateRequestByType(resource_type, is_https, previews_state); > > net::HttpRequestHeaders headers; > lofi_decider->MaybeSetAcceptTransformHeader(*request.get(), false, &headers); > > std::string header_value; > EXPECT_EQ(expected_accept_lite_page || expected_accept_empty_image, > headers.GetHeader(chrome_proxy_accept_transform_header(), > &header_value)); > > if (expected_accept_lite_page) { > EXPECT_TRUE(header_value == lite_page_directive()); > } else if (expected_accept_empty_image) { > EXPECT_TRUE(header_value == empty_image_directive()); > } else { > EXPECT_FALSE(headers.HasHeader(chrome_proxy_accept_transform_header())); > } > } Pulling in the target api call helps quite a bit. I went with passing in the request rather than all the parms for the request so that the tests themselves are more readable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by dougarnett@chromium.org
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": 120001, "attempt_start_ts": 1496418878445090, "parent_rev": "9bdbcce6c4e83bbbc7c52ef37f939b6c76602b20", "commit_rev": "f134cffafa9fca0640bacbb56d9de9af0a8f25aa"}
Message was sent while issue was closed.
Description was changed from ========== New CPAT support in ContentLoFiDecider guarded by feature flag. Updates MaybeSetAcceptTransformHeader to set accept header based only on PreviewsState (if new feature enabled) vs. checking various other flags. Also new path will not add "ifheavy" qualifier. This is step 2 of 4 in adding new CPAT protocol support. Step 1 is in CL 2889993004. Step 3 is in CL 2903453003. BUG=701802 ========== to ========== New CPAT support in ContentLoFiDecider guarded by feature flag. Updates MaybeSetAcceptTransformHeader to set accept header based only on PreviewsState (if new feature enabled) vs. checking various other flags. Also new path will not add "ifheavy" qualifier. This is step 2 of 4 in adding new CPAT protocol support. Step 1 is in CL 2889993004. Step 3 is in CL 2903453003. BUG=701802 Review-Url: https://codereview.chromium.org/2911673002 Cr-Commit-Position: refs/heads/master@{#476678} Committed: https://chromium.googlesource.com/chromium/src/+/f134cffafa9fca0640bacbb56d9d... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f134cffafa9fca0640bacbb56d9d... |