|
|
Created:
3 years, 6 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. |
DescriptionAdd force_empty_page proxy experiment flag and simplify ShouldAccept's
Client will now send Chrome-Proxy exp=force_empty_page if LoFi is
enabled via flags but Lite Pages are not enabled flags. This will
allow flag-savvy users to exercise LoFi instead of Lite Page previews
for testing the LoFi behavior. This experiment will be supported with
the new CPAT protocol changes. In conjunction, the logic for whether
to inform the proxy server that previews are accepted (via CPAT
header) is substantially simplified for the new protocol path since
the server will be making the decision (including whether copyright
restriction applies or not).
BUG=701802
Review-Url: https://codereview.chromium.org/2921503002
Cr-Commit-Position: refs/heads/master@{#479034}
Committed: https://chromium.googlesource.com/chromium/src/+/4ddaa6f836d00180fc6302b25863d04347d8f274
Patch Set 1 #Patch Set 2 : Added testLoFiForcedExperiment integration test #
Total comments: 14
Patch Set 3 : Some update per comments. #Patch Set 4 : Collapsed ShouldAccept* methods into single ShouldAcceptServerPreview method #
Total comments: 21
Patch Set 5 : Some updated naming per bengr feedback #
Total comments: 8
Patch Set 6 : Reverted the flag accesor names (with TODO to revisit with cleanup work) #Messages
Total messages: 43 (30 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 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...
dougarnett@chromium.org changed reviewers: + megjablon@chromium.org
Here is my take on simplifying the ShouldAccept* methods plus the force_empty_image directive addition.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (left): https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1092: if (params::IsLoFiSlowConnectionsOnlyViaFlags() || Making a note: as discussed offline the slow connections flag will do nothing in the new CPAT world. We need to make sure we're ok with not having that. https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1102: // Lite Pages depends on at least Lo-Fi being enabled as well. If we end up having separate blacklists do we want this dependency? The flag checks we definitely need, but I'm not sure I like the double blacklist check. Separately, would "Lite Pages depends on at least Lo-Fi being accepted as well" make more sense? https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:121: !params::AreLitePagesEnabledViaFlags()) { !params::AreLitePagesEnabledViaFlags() is unnecessary since it's checked in the else if above https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:122: experiments_.push_back(chrome_proxy_force_empty_image_experiment()); Can we test this? Also, we need to verify that on the server this doesn't check ECT, but does check that CPAT: previews is there. I believe we could end up setting chrome_proxy_force_empty_image_experiment (or force lite page) but not CPAT: previews right? In which case we wouldn't want a preview or lo-fi. https://codereview.chromium.org/2921503002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lofi.py (right): https://codereview.chromium.org/2921503002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:189: self.skipTest('This test cannot be run with other experiments.') Are we going to fix this so it can be run on staging by having a different staging directive? https://codereview.chromium.org/2921503002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:204: if (self.checkLoFiResponse(response, True)): Should we also check that we get a "page-policies=empty-image" response too?
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/2921503002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1102: // Lite Pages depends on at least Lo-Fi being enabled as well. On 2017/06/01 21:57:14, megjablon wrote: > If we end up having separate blacklists do we want this dependency? The flag > checks we definitely need, but I'm not sure I like the double blacklist check. > > Separately, would "Lite Pages depends on at least Lo-Fi being accepted as well" > make more sense? Did the wording change. Good question on the blacklist checks. If they could return different values, then I think we have to check for both types (could look at api change to take multiple?). Or else we might not honor the local blacklist - ie, if one type is blacklisted and the other is not, then we could return a blacklisted preview since we have no control over which one the server gives us. https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:121: !params::AreLitePagesEnabledViaFlags()) { On 2017/06/01 21:57:14, megjablon wrote: > !params::AreLitePagesEnabledViaFlags() is unnecessary since it's checked in the > else if above Done - yeah, knew this was coming - one of those more explicit self-documenting code things vs. streamlined source code. https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:122: experiments_.push_back(chrome_proxy_force_empty_image_experiment()); On 2017/06/01 21:57:14, megjablon wrote: > Can we test this? > > Also, we need to verify that on the server this doesn't check ECT, but does > check that CPAT: previews is there. I believe we could end up setting > chrome_proxy_force_empty_image_experiment (or force lite page) but not CPAT: > previews right? In which case we wouldn't want a preview or lo-fi. Yes, the included integration test does verify this force directive is indeed in the request header. We can write much more complete integration tests once we have server support landed and get the other new path client CLs landed. For example, we can force ECT to 4G to verify server does not check it. Plus we can check page-polices on main response and more. https://codereview.chromium.org/2921503002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lofi.py (right): https://codereview.chromium.org/2921503002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:189: self.skipTest('This test cannot be run with other experiments.') On 2017/06/01 21:57:14, megjablon wrote: > Are we going to fix this so it can be run on staging by having a different > staging directive? Needs further negotiation (not sure if this CL should block on it). https://codereview.chromium.org/2921503002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:204: if (self.checkLoFiResponse(response, True)): On 2017/06/01 21:57:14, megjablon wrote: > Should we also check that we get a "page-policies=empty-image" response too? Will add TODOs for this and forcing 4G ECT
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...
https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1102: // Lite Pages depends on at least Lo-Fi being enabled as well. On 2017/06/01 23:15:13, dougarnett wrote: > On 2017/06/01 21:57:14, megjablon wrote: > > If we end up having separate blacklists do we want this dependency? The flag > > checks we definitely need, but I'm not sure I like the double blacklist check. > > > > Separately, would "Lite Pages depends on at least Lo-Fi being accepted as > well" > > make more sense? > > Did the wording change. > > Good question on the blacklist checks. If they could return different values, > then I think we have to check for both types (could look at api change to take > multiple?). Or else we might not honor the local blacklist - ie, if one type is > blacklisted and the other is not, then we could return a blacklisted preview > since we have no control over which one the server gives us. After pondering further, I update to just have a single ShouldAcceptPreview() method (since really that is what the new protocol model is anyway (lite-page => server previews on)). https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:122: experiments_.push_back(chrome_proxy_force_empty_image_experiment()); On 2017/06/01 23:15:13, dougarnett wrote: > On 2017/06/01 21:57:14, megjablon wrote: > > Can we test this? > > > > Also, we need to verify that on the server this doesn't check ECT, but does > > check that CPAT: previews is there. I believe we could end up setting > > chrome_proxy_force_empty_image_experiment (or force lite page) but not CPAT: > > previews right? In which case we wouldn't want a preview or lo-fi. > > Yes, the included integration test does verify this force directive is indeed in > the request header. > > We can write much more complete integration tests once we have server support > landed and get the other new path client CLs landed. For example, we can force > ECT to 4G to verify server does not check it. Plus we can check page-polices on > main response and more. Note - now have landed separate change that forces ECT 4G for the force_lite_page test to ensure server does not check it.
dougarnett@chromium.org changed reviewers: + bengr@chromium.org
Ben, please take a look at this simplification of consulting very few flags not wrt accepting previews for new CPAT protocol (and no longer looking at field trials). Megan has a comment she wants to check with you about the SlowConnectionFlag now becoming equivalent to AlwaysOn.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1073: const net::URLRequest& request, Do we need the whole request? Can we just pass in the url? https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1079: if (params::IsLoFiDisabledViaFlags()) { Should this be called IsServerPreviewsDisabled()? https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1083: if (IsBlackListedOrDisabled(request, previews_decider, Can we narrow this interface to url or host? https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1088: if (params::IsLoFiCellularOnlyViaFlags()) { Should this be IsServerPreviewCellularOnlyViaFlags()? https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (left): https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1654: TEST_F(DataReductionProxyConfigTest, ShouldAcceptLitePages) { Why is it safe to delete this test? https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:120: } else if (params::IsLoFiOnViaFlags()) { So there's a lofi on and a lofi disabled via flags? Should this be IsServerPreviewsOnViaFlags? https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:106: // preview for requests that enable proxy provided previews. proxy provider -> server-provided https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:107: const char* chrome_proxy_force_empty_image_experiment(); nit: We should probably rename all of these as chrome_proxy_experiment... https://codereview.chromium.org/2921503002/diff/60001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lofi.py (right): https://codereview.chromium.org/2921503002/diff/60001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:183: # Checks that LoFi images are served and the force_empty_image experiment We use the term "empty image" sometimes and LoFi other times. Personally, I like "image placeholders" Would you file a bug to clean up our naming?
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/2921503002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (left): https://codereview.chromium.org/2921503002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1092: if (params::IsLoFiSlowConnectionsOnlyViaFlags() || On 2017/06/01 21:57:14, megjablon wrote: > Making a note: as discussed offline the slow connections flag will do nothing in > the new CPAT world. We need to make sure we're ok with not having that. Fyi, I raised with Ben today and he gets it. https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1073: const net::URLRequest& request, On 2017/06/08 18:53:00, bengr wrote: > Do we need the whole request? Can we just pass in the url? This ends up calling PreviewsIOData::ShouldAllowPreviewAtECT() which uses url(), context(), and load_flags() on the request. https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1079: if (params::IsLoFiDisabledViaFlags()) { On 2017/06/08 18:53:00, bengr wrote: > Should this be called IsServerPreviewsDisabled()? Done. https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1083: if (IsBlackListedOrDisabled(request, previews_decider, On 2017/06/08 18:53:00, bengr wrote: > Can we narrow this interface to url or host? noted above https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1088: if (params::IsLoFiCellularOnlyViaFlags()) { On 2017/06/08 18:52:59, bengr wrote: > Should this be IsServerPreviewCellularOnlyViaFlags()? Done. But not changing about flags yet. https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (left): https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1654: TEST_F(DataReductionProxyConfigTest, ShouldAcceptLitePages) { On 2017/06/08 18:53:00, bengr wrote: > Why is it safe to delete this test? The method it was testing has gone away in this CL https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:120: } else if (params::IsLoFiOnViaFlags()) { On 2017/06/08 18:53:00, bengr wrote: > So there's a lofi on and a lofi disabled via flags? Should this be > IsServerPreviewsOnViaFlags? This one is used in many more places and I think it would be better to rename in an M-62 clean-up (where many uses go away). I could add a new method that calls this one, however, in this call site it seems to fit pretty well. https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:106: // preview for requests that enable proxy provided previews. On 2017/06/08 18:53:00, bengr wrote: > proxy provider -> server-provided Done. https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:107: const char* chrome_proxy_force_empty_image_experiment(); On 2017/06/08 18:53:00, bengr wrote: > nit: We should probably rename all of these as chrome_proxy_experiment... Done. https://codereview.chromium.org/2921503002/diff/60001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lofi.py (right): https://codereview.chromium.org/2921503002/diff/60001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:183: # Checks that LoFi images are served and the force_empty_image experiment On 2017/06/08 18:53:00, bengr wrote: > We use the term "empty image" sometimes and LoFi other times. Personally, I like > "image placeholders" Would you file a bug to clean up our naming? Not sure we want to add a 3rd name. LoFi seems pretty stuck at the feature name and empty-image is stuck in the protocol definition.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1073: const net::URLRequest& request, On 2017/06/08 22:18:12, dougarnett wrote: > On 2017/06/08 18:53:00, bengr wrote: > > Do we need the whole request? Can we just pass in the url? > > This ends up calling PreviewsIOData::ShouldAllowPreviewAtECT() which uses > url(), context(), and load_flags() on the request. Acknowledged. https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/2921503002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:120: } else if (params::IsLoFiOnViaFlags()) { On 2017/06/08 22:18:12, dougarnett wrote: > On 2017/06/08 18:53:00, bengr wrote: > > So there's a lofi on and a lofi disabled via flags? Should this be > > IsServerPreviewsOnViaFlags? > > This one is used in many more places and I think it would be better to rename in > an M-62 clean-up (where many uses go away). I could add a new method that calls > this one, however, in this call site it seems to fit pretty well. Acknowledged. https://codereview.chromium.org/2921503002/diff/60001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lofi.py (right): https://codereview.chromium.org/2921503002/diff/60001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:183: # Checks that LoFi images are served and the force_empty_image experiment On 2017/06/08 22:18:13, dougarnett wrote: > On 2017/06/08 18:53:00, bengr wrote: > > We use the term "empty image" sometimes and LoFi other times. Personally, I > like > > "image placeholders" Would you file a bug to clean up our naming? > > Not sure we want to add a 3rd name. LoFi seems pretty stuck at the feature name > and empty-image is stuck in the protocol definition. Acknowledged.
https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:119: experiments_.push_back(chrome_proxy_experiment_force_lite_page()); The server still checks that the cpat:lite-page header is there for these right? We could end up setting the experimental header for cellular-only, but not the CPAT if we're on wifi. https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:83: bool IsLoFiOnViaFlags(); Should we update all of these to AreServerPreviews*? I feel like all the data-reduction-proxy-lo-fi flag related methods should use a consistent naming scheme. https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:91: bool IsServerPreviewCellularOnlyViaFlags(); s/IsServerPreview/AreServerPreviews I'm not a huge fan of this rename without updating the flags, but I'm ok with keeping it since they will all be aligned when we update the flags. Maybe we should add a comment for now that server previews are cellular only or disabled if Lo-Fi flags are set to disabled. I'd prefer we choose to rename all of them or none of them. https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:98: bool IsServerPreviewsDisabled(); s/IsServerPreviews/AreServerPreviews?
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/2921503002/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc (right): https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:119: experiments_.push_back(chrome_proxy_experiment_force_lite_page()); On 2017/06/12 23:02:28, megjablon wrote: > The server still checks that the cpat:lite-page header is there for these right? > We could end up setting the experimental header for cellular-only, but not the > CPAT if we're on wifi. Yes, that is correct, the server must check CPAT for the page as these exp settings are now session-oriented. https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:83: bool IsLoFiOnViaFlags(); On 2017/06/12 23:02:28, megjablon wrote: > Should we update all of these to AreServerPreviews*? I feel like all the > data-reduction-proxy-lo-fi flag related methods should use a consistent naming > scheme. This is more broadly used. I think we should keep current names for now. https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:91: bool IsServerPreviewCellularOnlyViaFlags(); On 2017/06/12 23:02:28, megjablon wrote: > s/IsServerPreview/AreServerPreviews > > I'm not a huge fan of this rename without updating the flags, but I'm ok with > keeping it since they will all be aligned when we update the flags. Maybe we > should add a comment for now that server previews are cellular only or disabled > if Lo-Fi flags are set to disabled. I'd prefer we choose to rename all of them > or none of them. Ok, none of them at this point. https://codereview.chromium.org/2921503002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:98: bool IsServerPreviewsDisabled(); On 2017/06/12 23:02:28, megjablon wrote: > s/IsServerPreviews/AreServerPreviews? Acknowledged.
lgtm
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
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2921503002/#ps100001 (title: "Reverted the flag accesor names (with TODO to revisit with cleanup work)")
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": 100001, "attempt_start_ts": 1497369314368400, "parent_rev": "2ab9c3e0419fd5eed46456cf2b6f772525ba45bd", "commit_rev": "4ddaa6f836d00180fc6302b25863d04347d8f274"}
Message was sent while issue was closed.
Description was changed from ========== Add force_empty_page proxy experiment flag and simplify ShouldAccept's Client will now send Chrome-Proxy exp=force_empty_page if LoFi is enabled via flags but Lite Pages are not enabled flags. This will allow flag-savvy users to exercise LoFi instead of Lite Page previews for testing the LoFi behavior. This experiment will be supported with the new CPAT protocol changes. In conjunction, the logic for whether to inform the proxy server that previews are accepted (via CPAT header) is substantially simplified for the new protocol path since the server will be making the decision (including whether copyright restriction applies or not). BUG=701802 ========== to ========== Add force_empty_page proxy experiment flag and simplify ShouldAccept's Client will now send Chrome-Proxy exp=force_empty_page if LoFi is enabled via flags but Lite Pages are not enabled flags. This will allow flag-savvy users to exercise LoFi instead of Lite Page previews for testing the LoFi behavior. This experiment will be supported with the new CPAT protocol changes. In conjunction, the logic for whether to inform the proxy server that previews are accepted (via CPAT header) is substantially simplified for the new protocol path since the server will be making the decision (including whether copyright restriction applies or not). BUG=701802 Review-Url: https://codereview.chromium.org/2921503002 Cr-Commit-Position: refs/heads/master@{#479034} Committed: https://chromium.googlesource.com/chromium/src/+/4ddaa6f836d00180fc6302b25863... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4ddaa6f836d00180fc6302b25863... |