New CPAT support in ContentLoFiDecider guarded by feature flag.

components/data_reduction_proxy/content/browser/content_lofi_decider.cc

Index: components/data_reduction_proxy/content/browser/content_lofi_decider.cc
diff --git a/components/data_reduction_proxy/content/browser/content_lofi_decider.cc b/components/data_reduction_proxy/content/browser/content_lofi_decider.cc
index ac9452408650d0d10faad044a1f504a280c32253..2d2b7048358bda6f70a442a70a3eeb3368640926 100644
--- a/components/data_reduction_proxy/content/browser/content_lofi_decider.cc
+++ b/components/data_reduction_proxy/content/browser/content_lofi_decider.cc
@@ -9,6 +9,7 @@
 #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"
@@ -67,29 +68,46 @@ void ContentLoFiDecider::MaybeSetAcceptTransformHeader(
     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) {
-    return;
-  }
-
-  // Previews has been disabled.
-  if (are_previews_disabled)
-    return;
-
   // 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)
+  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;
+  }
+
+  // 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);
+
+  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.
@@ -101,15 +119,25 @@ void ContentLoFiDecider::MaybeSetAcceptTransformHeader(
         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