Adding q=low to the Chrome-Proxy request header

components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc

Index: components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_auth_request_handler.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
similarity index 66%
rename from components/data_reduction_proxy/core/browser/data_reduction_proxy_auth_request_handler.cc
rename to components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
index b4bad4fcee32e16f121f3c2a25ef6b9062c5ef41..dbd5a386a590e3ed1c88d3806d0fcaec2353068e 100644
--- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_auth_request_handler.cc
+++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
@@ -2,7 +2,7 @@
 // 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/core/browser/data_reduction_proxy_auth_request_handler.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -28,6 +28,15 @@
 
 namespace data_reduction_proxy {
 
+namespace {
+const std::string kSession = "ps";
+const std::string kCredentials = "sid";
+const std::string kBuildNumber = "b";
+const std::string kPatchNumber = "p";
+const std::string kClient = "c";
+const std::string kLoFi = "q";
+}
+
 // The empty version for the authentication protocol. Currently used by
 // Android webview.
 #if defined(OS_ANDROID)
@@ -46,14 +55,14 @@ const char* GetString(Client client) {
 #undef CLIENT_ENUM
 
 // static
-bool DataReductionProxyAuthRequestHandler::IsKeySetOnCommandLine() {
+bool DataReductionProxyRequestOptions::IsKeySetOnCommandLine() {
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
   return command_line.HasSwitch(
       data_reduction_proxy::switches::kDataReductionProxyKey);
 }
 
-DataReductionProxyAuthRequestHandler::DataReductionProxyAuthRequestHandler(
+DataReductionProxyRequestOptions::DataReductionProxyRequestOptions(
     Client client,
     DataReductionProxyParams* params,
     scoped_refptr<base::SingleThreadTaskRunner> network_task_runner)
@@ -62,9 +71,19 @@ DataReductionProxyAuthRequestHandler::DataReductionProxyAuthRequestHandler(
       network_task_runner_(network_task_runner) {
   GetChromiumBuildAndPatch(ChromiumVersion(), &build_number_, &patch_number_);
   Init();
+  std::map<std::string, std::string> header_options;

[bengr, 2015/01/27 18:54:59]

Add a method called:

void UpdateVersion(const Client& client) {
  std::string build_number;
  std::string patch_number;
  GetChromiumBuildAndPatch(ChromiumVersion(), &build_number, &patch_number)
  if (!build_number_.empty() && !patch_number_.empty()) {
    header_options_[kBuildNumber] = build_number;
    header_options_[kPatchNumber] = patch_number;
  }
  std::string client_string = GetString(client);
  if (!client_string.empty())
    header_options_[kClient] = client_string;
  RegenerateRequestHeaderValue();
}

Note, I added RegenerateRequestHeaderValue() to the update methods. This will
result in double calls sometimes, but prevents us from forgetting to call it at
all. Feel free to do something different.

[megjablon, 2015/01/27 23:44:12]

Done.

+  if (!build_number_.empty() && !patch_number_.empty()) {
+    header_options[kBuildNumber] = build_number_;
+    header_options[kPatchNumber] = patch_number_;
+  }
+  if (!client_.empty())
+    header_options[kClient] = client_;
+  if (DataReductionProxyParams::IsDataReductionProxyLoFiEnabled())

[bengr, 2015/01/27 18:54:59]

I know this can't happen because you only enable via a flag, but what happens if
this returns false after return true? I think you should not depend on the
implementation of IsDataReductionProxyLoFiEnabled() to always return the same
value for the session.

So you'll need to add a condition to AddAuthorizationHeader (renamed as
SetHeader) to check if the value changed and regenerate the header string if so.

[megjablon, 2015/01/27 23:44:12]

I was planning on doing this when I start working on the menu, but I'll add it
now.

+    header_options[kLoFi] = "low";
+  SetRequestHeaderOptions(header_options);

[bengr, 2015/01/27 18:55:00]

If these are all being set internally, why bother with the function? Of course
you still need to call RegenerateRequestHeaderValue()

[megjablon, 2015/01/27 23:44:12]

Done.

 }
 
-DataReductionProxyAuthRequestHandler::DataReductionProxyAuthRequestHandler(
+DataReductionProxyRequestOptions::DataReductionProxyRequestOptions(
     Client client,
     const std::string& version,
     DataReductionProxyParams* params,
@@ -74,9 +93,19 @@ DataReductionProxyAuthRequestHandler::DataReductionProxyAuthRequestHandler(
       network_task_runner_(network_task_runner) {
   GetChromiumBuildAndPatch(version, &build_number_, &patch_number_);
   Init();
+  std::map<std::string, std::string> header_options;
+  if (!build_number_.empty() && !patch_number_.empty()) {
+    header_options[kBuildNumber] = build_number_;

[bengr, 2015/01/27 18:54:59]

build_number_ and patch_number_ shouldn't be members.

[megjablon, 2015/01/27 23:44:12]

Done.

+    header_options[kPatchNumber] = patch_number_;
+  }
+  if (!client_.empty())
+    header_options[kClient] = client_;
+  if (DataReductionProxyParams::IsDataReductionProxyLoFiEnabled())
+    header_options[kLoFi] = "low";
+  SetRequestHeaderOptions(header_options);
 }
 
-std::string DataReductionProxyAuthRequestHandler::ChromiumVersion() const {
+std::string DataReductionProxyRequestOptions::ChromiumVersion() const {
 #if defined(PRODUCT_VERSION)
   return PRODUCT_VERSION;
 #else
@@ -85,7 +114,7 @@ std::string DataReductionProxyAuthRequestHandler::ChromiumVersion() const {
 }
 
 
-void DataReductionProxyAuthRequestHandler::GetChromiumBuildAndPatch(
+void DataReductionProxyRequestOptions::GetChromiumBuildAndPatch(
     const std::string& version,
     std::string* build,
     std::string* patch) const {
@@ -97,16 +126,16 @@ void DataReductionProxyAuthRequestHandler::GetChromiumBuildAndPatch(
   *patch = version_parts[3];
 }
 
-void DataReductionProxyAuthRequestHandler::Init() {
+void DataReductionProxyRequestOptions::Init() {
   InitAuthentication(GetDefaultKey());
 }
 
 
-DataReductionProxyAuthRequestHandler::~DataReductionProxyAuthRequestHandler() {
+DataReductionProxyRequestOptions::~DataReductionProxyRequestOptions() {
 }
 
 // static
-base::string16 DataReductionProxyAuthRequestHandler::AuthHashForSalt(
+base::string16 DataReductionProxyRequestOptions::AuthHashForSalt(
     int64 salt,
     const std::string& key) {
   std::string salted_key =
@@ -119,16 +148,15 @@ base::string16 DataReductionProxyAuthRequestHandler::AuthHashForSalt(
 
 
 
-base::Time DataReductionProxyAuthRequestHandler::Now() const {
+base::Time DataReductionProxyRequestOptions::Now() const {
   return base::Time::Now();
 }
 
-void DataReductionProxyAuthRequestHandler::RandBytes(
-    void* output, size_t length) {
+void DataReductionProxyRequestOptions::RandBytes(void* output, size_t length) {
   crypto::RandBytes(output, length);
 }
 
-void DataReductionProxyAuthRequestHandler::MaybeAddRequestHeader(
+void DataReductionProxyRequestOptions::MaybeAddRequestHeader(
     net::URLRequest* request,
     const net::ProxyServer& proxy_server,
     net::HttpRequestHeaders* request_headers) {
@@ -142,19 +170,23 @@ void DataReductionProxyAuthRequestHandler::MaybeAddRequestHeader(
                             request_headers);
 }
 
-void DataReductionProxyAuthRequestHandler::MaybeAddProxyTunnelRequestHandler(
+void DataReductionProxyRequestOptions::MaybeAddProxyTunnelRequestHandler(
     const net::HostPortPair& proxy_server,
     net::HttpRequestHeaders* request_headers) {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
   MaybeAddRequestHeaderImpl(proxy_server, true, request_headers);
 }
 
-void DataReductionProxyAuthRequestHandler::AddAuthorizationHeader(
+void DataReductionProxyRequestOptions::AddAuthorizationHeader(

[bengr, 2015/01/27 18:55:00]

Maybe rename this as SetHeader()

[megjablon, 2015/01/27 23:44:12]

Done.

     net::HttpRequestHeaders* headers) {
   base::Time now = Now();
   if (now - last_update_time_ > base::TimeDelta::FromHours(24)) {

[bengr, 2015/01/27 18:54:59]

Add a comment:

// Authorization credentials must be regenerated at least every 24 hours.

[megjablon, 2015/01/27 23:44:12]

Done.

     last_update_time_ = now;
     ComputeCredentials(last_update_time_, &session_, &credentials_);

[bengr, 2015/01/27 18:54:59]

Call UpdateCredentials()

[megjablon, 2015/01/27 23:44:12]

Done.

+    std::map<std::string, std::string> header_options;
+    header_options[kSession] = session_;
+    header_options[kCredentials] = credentials_;
+    SetRequestHeaderOptions(header_options);
   }
   const char kChromeProxyHeader[] = "Chrome-Proxy";
   std::string header_value;
@@ -163,16 +195,11 @@ void DataReductionProxyAuthRequestHandler::AddAuthorizationHeader(
     headers->RemoveHeader(kChromeProxyHeader);
     header_value += ", ";
   }
-  header_value +=
-      "ps=" + session_ + ", sid=" + credentials_;
-  if (!build_number_.empty() && !patch_number_.empty())
-    header_value += ", b=" + build_number_ + ", p=" + patch_number_;
-  if (!client_.empty())
-    header_value += ", c=" + client_;
+  header_value += header_value_;
   headers->SetHeader(kChromeProxyHeader, header_value);
 }
 
-void DataReductionProxyAuthRequestHandler::ComputeCredentials(
+void DataReductionProxyRequestOptions::ComputeCredentials(

[bengr, 2015/01/27 18:54:59]

I'd add a method called void UpdateCredentials() that calls this.

void UpdateCredentials() {
  std::string session;
  std::string credentials;
  ComputeCredentials(last_update_time_; &session, &credentials);
  header_options_[kSession] = session;
  header_options_[kCredentials] = credentials;
  RegenerateRequestHeaderValue();
}

[megjablon, 2015/01/27 23:44:12]

Done.

     const base::Time& now,
     std::string* session,
     std::string* credentials) {
@@ -194,12 +221,12 @@ void DataReductionProxyAuthRequestHandler::ComputeCredentials(
            << "password: [" << *credentials  << "]";
 }
 
-void DataReductionProxyAuthRequestHandler::InitAuthentication(
+void DataReductionProxyRequestOptions::InitAuthentication(
     const std::string& key) {
   if (!network_task_runner_->BelongsToCurrentThread()) {
     network_task_runner_->PostTask(
         FROM_HERE,
-        base::Bind(&DataReductionProxyAuthRequestHandler::InitAuthentication,
+        base::Bind(&DataReductionProxyRequestOptions::InitAuthentication,
                    base::Unretained(this),
                    key));
     return;
@@ -211,9 +238,13 @@ void DataReductionProxyAuthRequestHandler::InitAuthentication(
   key_ = key;
   last_update_time_ = Now();
   ComputeCredentials(last_update_time_, &session_, &credentials_);

[bengr, 2015/01/27 18:55:00]

Call UpdateCredentials();

[megjablon, 2015/01/27 23:44:12]

Done.

+  std::map<std::string, std::string> header_options;
+  header_options[kSession] = session_;
+  header_options[kCredentials] = credentials_;
+  SetRequestHeaderOptions(header_options);
 }
 
-std::string DataReductionProxyAuthRequestHandler::GetDefaultKey() const {
+std::string DataReductionProxyRequestOptions::GetDefaultKey() const {
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
   std::string key =
@@ -233,7 +264,7 @@ std::string DataReductionProxyAuthRequestHandler::GetDefaultKey() const {
   return key;
 }
 
-void DataReductionProxyAuthRequestHandler::MaybeAddRequestHeaderImpl(
+void DataReductionProxyRequestOptions::MaybeAddRequestHeaderImpl(
     const net::HostPortPair& proxy_server,
     bool expect_ssl,
     net::HttpRequestHeaders* request_headers) {
@@ -248,4 +279,23 @@ void DataReductionProxyAuthRequestHandler::MaybeAddRequestHeaderImpl(
   }
 }
 
+void DataReductionProxyRequestOptions::SetRequestHeaderOptions(
+    std::map<std::string, std::string> header_options) {
+  for (std::map<std::string, std::string>::iterator it = header_options.begin();
+       it != header_options.end(); ++it) {
+    header_options_[it->first] = it->second;
+  }
+  RegenerateRequestHeaderValue();
+}
+
+void DataReductionProxyRequestOptions::RegenerateRequestHeaderValue() {
+  header_value_ = "";
+  for (std::map<std::string, std::string>::iterator

[bengr, 2015/01/27 18:55:00]

Another option here is to push the values into a vector and then call
JoinString(). That's arguably the tiniest bit cleaner.

[megjablon, 2015/01/27 23:44:12]

Done.

+       it = header_options_.begin(); it != header_options_.end(); ++it) {
+    header_value_ += it->first + "=" + it->second;
+    if (it != --header_options_.end())
+      header_value_ += ", ";
+  }
+}
+
 }  // namespace data_reduction_proxy