|
|
Created:
3 years, 7 months ago by dougarnett Modified:
3 years, 6 months ago CC:
chromium-reviews, tbansal+watch-data-reduction-proxy_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew CPAT support in DataReductionProxyConfig guarded by feature flag.
Defines new feature "DataReductionProxyDecidesTransform" and uses it
to select new behavior for ShouldEnableLoFi() and ShouldEnableLitePages
that does not check the effective connection type.
This is step 1 of 4 in adding new CPAT protocol support where the
proxy server is meant to drive optimizations. These are the expected
steps:
1. Add support in DataReductionProxyConfig (behind flag).
2. Add support in ContentLoFiDecider (behind same flag).
3. Add force-lite-page experiment support.
4. Add support for transforms directive from the server. [Also,
may add about-flags support for this new feature once the feature
is code complete.]
Later release (once new CPAT stable):
5. Clean up old path code.
Note that this CL does not check for client driven fallback
nor deal with Control group setting and later clearing for the
new feature flag path.
BUG=701802
Review-Url: https://codereview.chromium.org/2889993004
Cr-Commit-Position: refs/heads/master@{#474808}
Committed: https://chromium.googlesource.com/chromium/src/+/cbda5f6b2b2d76236388e663c3439ef91c59ec0a
Patch Set 1 #
Total comments: 20
Patch Set 2 : Updated per comment feedback #Patch Set 3 : Factored blacklist and lofi_off_ check (and comment verbage) into helper #
Total comments: 2
Patch Set 4 : Improved comment wording for previews_decider arg #Patch Set 5 : Added const-ness to PreviewDecider arg #
Total comments: 20
Patch Set 6 : Updates per Megan's comments #
Total comments: 24
Patch Set 7 : Updates per Ben's comments #Patch Set 8 : Update legacy ShouldEnableLoFiInternal to return false in LitePages enabled #Patch Set 9 : Updates new code path to accept LoFi when LitePages enabled (to allow fallback) #Messages
Total messages: 54 (34 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: + bengr@chromium.org, megjablon@chromium.org
New approach on supporting the new CPAT protocol to land in a few increments with new code gated by feature flag (per request from Ben). One key point with my plan here is to not support fallback until we can do it as directed from the server (that simplifies things).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping
On 2017/05/19 15:44:18, dougarnett wrote: > Ping The steps seem reasonable. How much of the whole feature do you already have implemented? I want to be highly confident that it will work before signing of on the its pieces.
https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1018: // Make sure request is not locally blacklisted. These functions have a lot of duplicate code, which should be refactored into a helper method. Ideally, though these two methods would have 100% overlap. It seems flags asymmetry is the reason they don't. Ryan is refactoring flags. Can you sync with him? If you build on top of his work, we can probably change these to 'bool ShouldAcceptServerPreviews(previews::PreviewsType type, ...)' https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:307: // it is will to accept the Server Lo-Fi optimization for |request|. will -> willing https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:308: // |previews_decider| is a non-null object that determines eligibility of the I can't parse this sentence. https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:310: // Should only be used if kDataReductionProxyDecidesTransform feature enabled. feature -> feature is https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:315: // it is will to accept a LitePage optimization for |request|. will -> willing https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:316: // |previews_decider| is a non-null object that determines eligibility of the I can't parse. https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:318: // Should only be used if kDataReductionProxyDecidesTransform feature enabled. feature -> feature is https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:327: // Should NOT be used if kDataReductionProxyDecidesTransform feature enabled. if -> if the feature -> feature is https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:337: // Should NOT be used if kDataReductionProxyDecidesTransform feature enabled. Add 'the' and 'is'
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/2889993004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1018: // Make sure request is not locally blacklisted. On 2017/05/19 20:20:47, bengr wrote: > These functions have a lot of duplicate code, which should be refactored into a > helper method. Ideally, though these two methods would have 100% overlap. It > seems flags asymmetry is the reason they don't. Ryan is refactoring flags. Can > you sync with him? If you build on top of his work, we can probably change these > to 'bool ShouldAcceptServerPreviews(previews::PreviewsType type, ...)' I agree on merit of refactoring along with cleaning up flags. Doesn't seem to be in scope of this change though - these are actually simpler that the corresponding *Internal() methods and keeping them similar initially helps compare them. I can clean up after Ryan consolidates flags if he does not do such clean-up here as part of the consolidation. https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:307: // it is will to accept the Server Lo-Fi optimization for |request|. On 2017/05/19 20:20:47, bengr wrote: > will -> willing Done. https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:308: // |previews_decider| is a non-null object that determines eligibility of the On 2017/05/19 20:20:47, bengr wrote: > I can't parse this sentence. Yeah, not my wording, I lifted this exact wording from ShouldEnableLoFiInternal() to keep consistent (since outside scope of what I am meaning to change here). https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:310: // Should only be used if kDataReductionProxyDecidesTransform feature enabled. On 2017/05/19 20:20:47, bengr wrote: > feature -> feature is Done. https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:315: // it is will to accept a LitePage optimization for |request|. On 2017/05/19 20:20:47, bengr wrote: > will -> willing Done. https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:316: // |previews_decider| is a non-null object that determines eligibility of the On 2017/05/19 20:20:47, bengr wrote: > I can't parse. ditto https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:318: // Should only be used if kDataReductionProxyDecidesTransform feature enabled. On 2017/05/19 20:20:47, bengr wrote: > feature -> feature is Done. https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:327: // Should NOT be used if kDataReductionProxyDecidesTransform feature enabled. On 2017/05/19 20:20:47, bengr wrote: > if -> if the > feature -> feature is Done. https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:337: // Should NOT be used if kDataReductionProxyDecidesTransform feature enabled. On 2017/05/19 20:20:47, bengr wrote: > Add 'the' and 'is' 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...
https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1018: // Make sure request is not locally blacklisted. On 2017/05/19 21:43:58, dougarnett wrote: > On 2017/05/19 20:20:47, bengr wrote: > > These functions have a lot of duplicate code, which should be refactored into > a > > helper method. Ideally, though these two methods would have 100% overlap. It > > seems flags asymmetry is the reason they don't. Ryan is refactoring flags. Can > > you sync with him? If you build on top of his work, we can probably change > these > > to 'bool ShouldAcceptServerPreviews(previews::PreviewsType type, ...)' > > I agree on merit of refactoring along with cleaning up flags. Doesn't seem to be > in scope of this change though - these are actually simpler that the > corresponding *Internal() methods and keeping them similar initially helps > compare them. I can clean up after Ryan consolidates flags if he does not do > such clean-up here as part of the consolidation. Ok, have factored just this blacklist and loff_off_ check (with verbage) into a helper method. Also made entry on Ryan's TODO bug wrt not wanting ECT after all actually.
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/2889993004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2889993004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:308: // |previews_decider| is a non-null object that determines eligibility of the On 2017/05/19 21:43:58, dougarnett wrote: > On 2017/05/19 20:20:47, bengr wrote: > > I can't parse this sentence. > > Yeah, not my wording, I lifted this exact wording from > ShouldEnableLoFiInternal() to keep consistent (since outside scope of what I am > meaning to change here). Tried to clarify this wording now.
https://codereview.chromium.org/2889993004/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1051: previews::PreviewsDecider* previews_decider, Why is this not a const previews::PreviewsDecider& and why isn't the method const? Same question for all methods in this file that take a previews_decider?
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/2889993004/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1051: previews::PreviewsDecider* previews_decider, On 2017/05/19 22:44:53, bengr wrote: > Why is this not a const previews::PreviewsDecider& and why isn't the method > const? Same question for all methods in this file that take a previews_decider? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1013: features::kDataReductionProxyDecidesTransform)) Can this check just be moved inside the Internal methods around the network delegate code so we don't end up with so much duplicate code? https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1036: features::kDataReductionProxyDecidesTransform)) Same here https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1142: previews::PreviewsType::LOFI)) Use braces https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1174: previews::PreviewsType::LITE_PAGE)) Use braces https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1452: net::TestURLRequestContext context_; s/context_/context/ https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1453: net::TestDelegate delegate_; s/delegate_/delegate https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1571: // Verify for Cellular Only flag. Verify false for Cellular Only flag and WIFI connection. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1579: *previews_decider.get())); Verify true for Cellular Only flag and 3G connection. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1665: // Verify for Cellular Only flag. Verify true for Cellular Only flag and 3G connection. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1677: config()->ShouldAcceptLitePages(*request.get(), *previews_decider.get())); Verify false for Cellular Only flag and WIFI connection.
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/2889993004/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1013: features::kDataReductionProxyDecidesTransform)) On 2017/05/22 23:58:57, megjablon wrote: > Can this check just be moved inside the Internal methods around the network > delegate code so we don't end up with so much duplicate code? My intention is to delete the *Internal methods in cleanup pass after stable (would then be easy to review change). Also was thinking of dropping this method as well and just going with the new ShouldAccept* naming to be more in line with it just driving populating CPAT header without so much extra logic. This would include removing this feature definition and so the feature check as well. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1036: features::kDataReductionProxyDecidesTransform)) On 2017/05/22 23:58:57, megjablon wrote: > Same here Acknowledged. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1142: previews::PreviewsType::LOFI)) On 2017/05/22 23:58:57, megjablon wrote: > Use braces Done. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1174: previews::PreviewsType::LITE_PAGE)) On 2017/05/22 23:58:57, megjablon wrote: > Use braces Done. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1452: net::TestURLRequestContext context_; On 2017/05/22 23:58:57, megjablon wrote: > s/context_/context/ Done. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1453: net::TestDelegate delegate_; On 2017/05/22 23:58:57, megjablon wrote: > s/delegate_/delegate Done. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1571: // Verify for Cellular Only flag. On 2017/05/22 23:58:57, megjablon wrote: > Verify false for Cellular Only flag and WIFI connection. Done. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1579: *previews_decider.get())); On 2017/05/22 23:58:57, megjablon wrote: > Verify true for Cellular Only flag and 3G connection. Done. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1665: // Verify for Cellular Only flag. On 2017/05/22 23:58:57, megjablon wrote: > Verify true for Cellular Only flag and 3G connection. Done. https://codereview.chromium.org/2889993004/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1677: config()->ShouldAcceptLitePages(*request.get(), *previews_decider.get())); On 2017/05/22 23:58:57, megjablon wrote: > Verify false for Cellular Only flag and WIFI connection. Done.
https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1014: return ShouldAcceptServerLoFi(request, previews_decider); Add curly braces. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1037: return ShouldAcceptLitePages(request, previews_decider); Add curly braces. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1081: previews::PreviewsType::LOFI)) Add curly braces. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1109: previews::PreviewsType::LITE_PAGE)) Add curly braces. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:203: bool ShouldEnableLoFi(const net::URLRequest& request, Can this method be const? https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:209: bool ShouldEnableLitePages(const net::URLRequest& request, Can this method be const? https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:217: bool IsBlackListedOrDisabled( Does this need to be public? Does it even need to be part of the class? I.e., can it be a helper method in the .cc? https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:317: bool ShouldAcceptServerLoFi( Can this method be const? https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:326: bool ShouldAcceptLitePages(const net::URLRequest& request, Const method? https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:336: bool ShouldEnableLoFiInternal( Const method? https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:348: bool ShouldEnableLitePagesInternal( Const method? https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc (right): https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc:18: // Enables letting proxy server decide on transformations vs. client Please rewrite to make this clearer: // Enables a new version of the data reduction proxy protocol where the server // decides if a server-generated preview should be served. The previous // version required the client to make this decision. The new protocol relies // on updates primarily to the Chrome-Proxy-Accept-Transform header.
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/2889993004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1014: return ShouldAcceptServerLoFi(request, previews_decider); On 2017/05/23 17:51:24, bengr wrote: > Add curly braces. Done. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1037: return ShouldAcceptLitePages(request, previews_decider); On 2017/05/23 17:51:24, bengr wrote: > Add curly braces. Done. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1081: previews::PreviewsType::LOFI)) On 2017/05/23 17:51:24, bengr wrote: > Add curly braces. Done. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1109: previews::PreviewsType::LITE_PAGE)) On 2017/05/23 17:51:24, bengr wrote: > Add curly braces. Done. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:203: bool ShouldEnableLoFi(const net::URLRequest& request, On 2017/05/23 17:51:24, bengr wrote: > Can this method be const? No, *Internal() sets some members. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:209: bool ShouldEnableLitePages(const net::URLRequest& request, On 2017/05/23 17:51:24, bengr wrote: > Can this method be const? No, *Internal() sets some members. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:217: bool IsBlackListedOrDisabled( On 2017/05/23 17:51:25, bengr wrote: > Does this need to be public? Does it even need to be part of the class? I.e., > can it be a helper method in the .cc? private now. Currently accesses a member. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:317: bool ShouldAcceptServerLoFi( On 2017/05/23 17:51:24, bengr wrote: > Can this method be const? Done. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:326: bool ShouldAcceptLitePages(const net::URLRequest& request, On 2017/05/23 17:51:24, bengr wrote: > Const method? Done. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:336: bool ShouldEnableLoFiInternal( On 2017/05/23 17:51:24, bengr wrote: > Const method? No, sets some members. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:348: bool ShouldEnableLitePagesInternal( On 2017/05/23 17:51:24, bengr wrote: > Const method? No, sets some members. https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc (right): https://codereview.chromium.org/2889993004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc:18: // Enables letting proxy server decide on transformations vs. client On 2017/05/23 17:51:25, bengr wrote: > Please rewrite to make this clearer: > > // Enables a new version of the data reduction proxy protocol where the server > // decides if a server-generated preview should be served. The previous > // version required the client to make this decision. The new protocol relies > // on updates primarily to the Chrome-Proxy-Accept-Transform header. 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...
Megan, per our discussion I made ShouldEnableLoFiInternal return false now if LitePages are enabled (so that the SERVER_LOFI_ON bit will NOT be set if SERVER_LITE_PAGES_ON bit is set). [This is meant to allow us to distinguish between new code path and old path in RenderFrameImpl based on this bit wrt determining to do preview fallback.]
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping - any more comments on this one?
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...
The CQ bit was unchecked by dougarnett@chromium.org
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...
lgtm
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495741437854000, "parent_rev": "3a5be87dc36a4267a126c809880ed2621568cb98", "commit_rev": "cbda5f6b2b2d76236388e663c3439ef91c59ec0a"}
Message was sent while issue was closed.
Description was changed from ========== New CPAT support in DataReductionProxyConfig guarded by feature flag. Defines new feature "DataReductionProxyDecidesTransform" and uses it to select new behavior for ShouldEnableLoFi() and ShouldEnableLitePages that does not check the effective connection type. This is step 1 of 4 in adding new CPAT protocol support where the proxy server is meant to drive optimizations. These are the expected steps: 1. Add support in DataReductionProxyConfig (behind flag). 2. Add support in ContentLoFiDecider (behind same flag). 3. Add force-lite-page experiment support. 4. Add support for transforms directive from the server. [Also, may add about-flags support for this new feature once the feature is code complete.] Later release (once new CPAT stable): 5. Clean up old path code. Note that this CL does not check for client driven fallback nor deal with Control group setting and later clearing for the new feature flag path. BUG=701802 ========== to ========== New CPAT support in DataReductionProxyConfig guarded by feature flag. Defines new feature "DataReductionProxyDecidesTransform" and uses it to select new behavior for ShouldEnableLoFi() and ShouldEnableLitePages that does not check the effective connection type. This is step 1 of 4 in adding new CPAT protocol support where the proxy server is meant to drive optimizations. These are the expected steps: 1. Add support in DataReductionProxyConfig (behind flag). 2. Add support in ContentLoFiDecider (behind same flag). 3. Add force-lite-page experiment support. 4. Add support for transforms directive from the server. [Also, may add about-flags support for this new feature once the feature is code complete.] Later release (once new CPAT stable): 5. Clean up old path code. Note that this CL does not check for client driven fallback nor deal with Control group setting and later clearing for the new feature flag path. BUG=701802 Review-Url: https://codereview.chromium.org/2889993004 Cr-Commit-Position: refs/heads/master@{#474808} Committed: https://chromium.googlesource.com/chromium/src/+/cbda5f6b2b2d76236388e663c343... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/cbda5f6b2b2d76236388e663c343... |