New CPAT support in ContentLoFiDecider guarded by feature flag.

components/data_reduction_proxy/content/browser/content_lofi_decider.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/data_reduction_proxy/content/browser/content_lofi_decider.h"
 
 #include <string>
 
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_features.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/common/previews_state.h"
 #include "content/public/common/resource_type.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_request_headers.h"
 #include "net/url_request/url_request.h"
 
 namespace data_reduction_proxy {
@@ skipping 38 matching lines @@
     return;
 
   content::ResourceType resource_type = request_info->GetResourceType();
 
   if (resource_type == content::RESOURCE_TYPE_MEDIA) {
     headers->SetHeader(chrome_proxy_accept_transform_header(),
                        compressed_video_directive());
     return;
   }
 
-  // The Lo-Fi and Lite Page directives should not be added for users in the
-  // Lo-Fi field trial "Control" group.
-  bool lofi_enabled_via_flags_or_field_trial =
-      params::IsLoFiOnViaFlags() || params::IsIncludedInLoFiEnabledFieldTrial();
-
-  bool lite_page_enabled_via_flags_or_field_trial =
-      (params::IsLoFiOnViaFlags() && params::AreLitePagesEnabledViaFlags()) ||
-      params::IsIncludedInLitePageFieldTrial();
-
-  // User does not have previews enabled.
-  if (!lofi_enabled_via_flags_or_field_trial &&
-      !lite_page_enabled_via_flags_or_field_trial) {
+  // Do not add the Chrome-Proxy-Accept-Transform header when the page load
+  // explicitly forbids previews transformations.
+  content::PreviewsState previews_state = request_info->GetPreviewsState();

[megjablon, 2017/05/30 18:38:53]

I'd move this line above the comment.

[dougarnett, 2017/05/30 22:15:48]

Done.

+  if (previews_state & content::PREVIEWS_NO_TRANSFORM) {

[megjablon, 2017/05/30 18:38:53]

If you want, you can remove the brackets here.

[dougarnett, 2017/05/30 22:15:48]

Acknowledged.

     return;
   }
 
-  // Previews has been disabled.
-  if (are_previews_disabled)
-    return;
+  // For the proxy-decides-transform feature, we determine whether to set

[megjablon, 2017/05/30 18:38:53]

Don't use we. "the header is set based only on the request's PreviewsState"

[dougarnett, 2017/05/30 22:15:48]

Done.

+  // header by the request's PreviewsState (no further flag checks here).
+  // This is for a newer version of the proxy server protocol where the
+  // server makes the transform decision and client simply advertises
+  // if is accepts transforms for the resource type (determined here from
+  // previously set previews state bits). The previous logic in this method
+  // that checks various flags will be deprecated.
+  bool decide_by_previews_state = base::FeatureList::IsEnabled(

[megjablon, 2017/05/30 18:38:53]

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

[dougarnett, 2017/05/30 22:15:48]

Done.

+      features::kDataReductionProxyDecidesTransform);
 
-  // Do not add the Chrome-Proxy-Accept-Transform header when the page load
-  // explicitly forbids previews transformations.
-  if (request_info->GetPreviewsState() & content::PREVIEWS_NO_TRANSFORM)
-    return;
+  bool lofi_enabled_via_flags_or_field_trial = false;
+  bool lite_page_enabled_via_flags_or_field_trial = false;
+  if (!decide_by_previews_state) {
+    // The Lo-Fi and Lite Page directives should not be added for users in the
+    // Lo-Fi field trial "Control" group.
+    lofi_enabled_via_flags_or_field_trial =
+        params::IsLoFiOnViaFlags() ||
+        params::IsIncludedInLoFiEnabledFieldTrial();
+
+    lite_page_enabled_via_flags_or_field_trial =
+        (params::IsLoFiOnViaFlags() && params::AreLitePagesEnabledViaFlags()) ||
+        params::IsIncludedInLitePageFieldTrial();
+
+    // Previews has been disabled.
+    if (are_previews_disabled)
+      return;
+
+    // User does not have previews enabled.
+    if (!lofi_enabled_via_flags_or_field_trial &&
+        !lite_page_enabled_via_flags_or_field_trial) {
+      return;
+    }
+  }
 
   // Lo-Fi is not allowed on the main frame, stylesheet, script, font resource,
   // media, service worker, or CSP report.
   bool resource_type_supports_empty_image =
       !(resource_type == content::RESOURCE_TYPE_MAIN_FRAME ||
         resource_type == content::RESOURCE_TYPE_STYLESHEET ||
         resource_type == content::RESOURCE_TYPE_SCRIPT ||
         resource_type == content::RESOURCE_TYPE_FONT_RESOURCE ||
         resource_type == content::RESOURCE_TYPE_MEDIA ||
         resource_type == content::RESOURCE_TYPE_CSP_REPORT);
 
-  // If the Lite Page field trial or flag is enabled, only add the "lite-page"
-  // directive on main frame requests. Only add "empty-image" directives to
-  // other requests when Lite Page previews are not enabled after the main
-  // frame. Add the "if-heavy" qualifier to allow the server to provide a
-  // preview when the page is data heavy on if a preview was not otherwise
-  // triggered.
   std::string accept_transform_value;
-  if (lite_page_enabled_via_flags_or_field_trial &&
-      resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
+  if (decide_by_previews_state) {
+    // For the proxy-decides-transform feature, check PreviewsState bits and
+    // type of resource to determine which, if any, transformation to accept.
+    if ((previews_state & content::SERVER_LITE_PAGE_ON) &&
+        resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {

[megjablon, 2017/05/30 18:38:53]

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. 

[dougarnett, 2017/05/30 22:15:48]

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).






 

+      accept_transform_value = lite_page_directive();
+    } else if ((previews_state & content::SERVER_LOFI_ON) &&
+               resource_type_supports_empty_image) {
+      accept_transform_value = empty_image_directive();
+    }
+  } else if (lite_page_enabled_via_flags_or_field_trial &&
+             resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
+    // If the Lite Page field trial or flag is enabled, only add the "lite-page"
+    // directive on main frame requests. Only add "empty-image" directives to
+    // other requests when Lite Page previews are not enabled after the main
+    // frame. Add the "if-heavy" qualifier to allow the server to provide a
+    // preview when the page is data heavy on if a preview was not otherwise
+    // triggered.
     accept_transform_value = lite_page_directive();
 
     // Since a Lite Page was not triggered client side, ask the server to
     // provide a Lite Page only if the page is otherwise data-heavy.
     if (!(request_info->GetPreviewsState() & content::SERVER_LITE_PAGE_ON))
       accept_transform_value += base::StringPrintf(";%s", if_heavy_qualifier());
   } else if (lofi_enabled_via_flags_or_field_trial &&
              // Only use Lo-Fi if Lite Pages aren't enabled or fallback from
              // Lite Pages to Lo-Fi is enabled.
              (!lite_page_enabled_via_flags_or_field_trial ||
@@ skipping 83 matching lines @@
 }
 
 bool ContentLoFiDecider::IsClientLoFiAutoReloadRequest(
     const net::URLRequest& request) const {
   const content::ResourceRequestInfo* request_info =
       content::ResourceRequestInfo::ForRequest(&request);
   return request_info &&
          (request_info->GetPreviewsState() & content::CLIENT_LOFI_AUTO_RELOAD);
 }
 }  // namespace data_reduction_proxy