Move adding Lo-Fi directives from DRPRequestOptions to ContentLoFiDecider

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 0132adde720726bed468cc670b0865b482cb5d44..20aaf5d97067c80e06ec87b7a700e3320a462f24 100644
--- a/components/data_reduction_proxy/content/browser/content_lofi_decider.cc
+++ b/components/data_reduction_proxy/content/browser/content_lofi_decider.cc
@@ -4,10 +4,17 @@
 
 #include "components/data_reduction_proxy/content/browser/content_lofi_decider.h"
 
+#include <string>
+
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "content/public/browser/resource_request_info.h"
+#include "net/http/http_request_headers.h"
 
 namespace data_reduction_proxy {
 
+const char kLoFiHeader[] = "q=low";

[tbansal1, 2015/11/20 21:56:58]

Is it possible to move kLoFiHeader from data_reduction_proxy_headers.cc to
params, then you can simply use params::kLoFiHeader here, instead of redefining
it?

[megjablon, 2015/11/20 23:14:50]

Just using the value from data_reduction_proxy_headers.cc instead.

+const char kLoFiExperimentHeader[] = "exp=lofi_active_control";
+
 ContentLoFiDecider::ContentLoFiDecider() {}
 
 ContentLoFiDecider::~ContentLoFiDecider() {}
@@ -15,8 +22,58 @@ ContentLoFiDecider::~ContentLoFiDecider() {}
 bool ContentLoFiDecider::IsUsingLoFiMode(const net::URLRequest& request) const {
   const content::ResourceRequestInfo* request_info =
       content::ResourceRequestInfo::ForRequest(&request);
+  // The Lo-Fi directive should not be added for users in the Lo-Fi field
+  // trial "Control" group. Check that the user is in a group that should
+  // get "q=low".
+  bool lofi_enabled_via_flag_or_field_trial =
+      params::IsLoFiOnViaFlags() || params::IsIncludedInLoFiEnabledFieldTrial();
   if (request_info)
-    return request_info->IsUsingLoFi();
+    return request_info->IsUsingLoFi() && lofi_enabled_via_flag_or_field_trial;
+  return false;
+}
+
+bool ContentLoFiDecider::MaybeEnableLoFiMode(
+    const net::URLRequest& request,
+    net::HttpRequestHeaders* headers) const {
+  const content::ResourceRequestInfo* request_info =
+      content::ResourceRequestInfo::ForRequest(&request);
+
+  if (request_info) {

[sclittle, 2015/11/20 22:59:11]

So, Lo-Fi mode is not enabled for any requests that don't have
ResourceRequestInfo, like background requests or other requests that aren't tied
to a visible frame?

This is probably OK, but could you add a comment somewhere (maybe in the .h file
on the class or the method) stating this?

[megjablon, 2015/11/20 23:14:50]

Yep this was a side effect of the implementation tying Lo-Fi to a page that, and
we are ok with it. Added a comment.

+    // The Lo-Fi directive should not be added for users in the Lo-Fi field
+    // trial "Control" group. Check that the user is in a group that should
+    // get "q=low".
+    bool lofi_enabled_via_flag_or_field_trial =
+        params::IsLoFiOnViaFlags() ||
+        params::IsIncludedInLoFiEnabledFieldTrial();

[tbansal1, 2015/11/20 21:56:58]

Is it possible to move this common code to anonymous namespace?

[megjablon, 2015/11/20 23:14:50]

I don't see the need to move this. We don't use this logic anywhere else.

[tbansal1, 2015/11/23 20:39:52]

Line 26 up.

+
+    const char kChromeProxyHeader[] = "Chrome-Proxy";

[tbansal1, 2015/11/20 21:56:58]

Same here, is it possible to move this to params?

[megjablon, 2015/11/20 23:14:50]

Just using the value from data_reduction_proxy_headers.cc instead.

+    std::string header_value;
+
+    // User is using Lo-Fi and not part of the "Control" group.
+    if (request_info->IsUsingLoFi() && lofi_enabled_via_flag_or_field_trial) {
+      if (headers->HasHeader(kChromeProxyHeader)) {
+        headers->GetHeader(kChromeProxyHeader, &header_value);
+        headers->RemoveHeader(kChromeProxyHeader);
+        header_value += ", ";
+      }
+      header_value += kLoFiHeader;
+      headers->SetHeader(kChromeProxyHeader, header_value);
+      return true;
+    }
+
+    // User is part of Lo-Fi active control experiment.
+    if (request_info->IsUsingLoFi() &&
+        params::IsIncludedInLoFiControlFieldTrial()) {

[tbansal1, 2015/11/20 21:56:58]

Should this also check that Lo-Fi is not enabled from flags?
 && !params::IsLoFiOnViaFlags()

[megjablon, 2015/11/20 23:14:50]

That's redundant. If it's enabled from flags then
lofi_enabled_via_flag_or_field_trial will be true and it will enter the if
statement above.

[tbansal1, 2015/11/23 20:39:52]

Aah, did not see the return statement.

+      if (headers->HasHeader(kChromeProxyHeader)) {
+        headers->GetHeader(kChromeProxyHeader, &header_value);
+        headers->RemoveHeader(kChromeProxyHeader);
+        header_value += ", ";
+      }
+      header_value += kLoFiExperimentHeader;

[tbansal1, 2015/11/20 21:56:58]

Can the user be in more than two experiments?
In that case, does the header look like:
exp=a,exp=b
OR
exp=a,b?

[megjablon, 2015/11/20 23:14:50]

Yes the user can. It looks like exp=a,exp=b

https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re...

+      headers->SetHeader(kChromeProxyHeader, header_value);
+    }
+  }
+
   return false;
 }