Adding q=low to the Chrome-Proxy request header

Adding q=low to the Chrome-Proxy request header
and renaming DataReductionProxyAuthRequestHandler to
DataReductionProxyRequestOptions

BUG=449665
Committed: https://crrev.com/8fbff4fad29b9e2408c634e178386d456acf6467
Cr-Commit-Position: refs/heads/master@{#315000}

[megjablon, 2015-01-16 23:01:49]

Patchset #1 (id:1) has been deleted

[megjablon, 2015-01-16 23:08:13]

Patchset #1 (id:20001) has been deleted

[megjablon, 2015-01-16 23:08:27]

Patchset #1 (id:40001) has been deleted

[megjablon, 2015-01-16 23:30:56]

megjablon@chromium.org changed reviewers:
+ bengr@chromium.org

[bengr, 2015-01-17 01:06:17]

https://codereview.chromium.org/791493015/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/791493015/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6384: +         Enable or disable using Data
Reduction Proxy lo-fi.
These should be "Enable Data Saver Lo-Fi mode" and "Put the Data Saver in Lo-Fi
mode when it is enabled."

https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_auth_request_handler.cc
(right):

https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_auth_request_handler.cc:173:
header_value += ", q=low";
This has even less to do with authorization than does client type and version.
You should write a new class that handles request options.

https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:101:
bool DataReductionProxyParams::IsLoFiCommandLineSwitchOn() {
I'd change this either to HasLowFiCommandLineSwitch() or IsLoFiEnabled(). I
prefer the latter because you will probably add other conditions that have
nothing to do with the switch.

https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:102:
if (base::CommandLine::ForCurrentProcess()->HasSwitch(
How about:

return base::CommandLine::ForCurrentProcess()->
    HasSwitch(data_reduction_proxy::switches::kEnableDataReductionProxyLoFi);

[megjablon, 2015-01-24 01:32:10]

Patchset #2 (id:80001) has been deleted

[megjablon, 2015-01-24 01:37:55]

Patchset #2 (id:100001) has been deleted

[megjablon, 2015-01-24 01:48:59]

https://codereview.chromium.org/791493015/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/791493015/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6384: +         Enable or disable using Data
Reduction Proxy lo-fi.
On 2015/01/17 01:06:16, bengr wrote:
> These should be "Enable Data Saver Lo-Fi mode" and "Put the Data Saver in
Lo-Fi
> mode when it is enabled."

Done.

https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_auth_request_handler.cc
(right):

https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_auth_request_handler.cc:173:
header_value += ", q=low";
On 2015/01/17 01:06:17, bengr wrote:
> This has even less to do with authorization than does client type and version.
> You should write a new class that handles request options.
> 

Done.

https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:101:
bool DataReductionProxyParams::IsLoFiCommandLineSwitchOn() {
On 2015/01/17 01:06:17, bengr wrote:
> I'd change this either to HasLowFiCommandLineSwitch() or IsLoFiEnabled(). I
> prefer the latter because you will probably add other conditions that have
> nothing to do with the switch.

Done.

https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:102:
if (base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/01/17 01:06:17, bengr wrote:
> How about:
> 
> return base::CommandLine::ForCurrentProcess()->
>     HasSwitch(data_reduction_proxy::switches::kEnableDataReductionProxyLoFi);

Done.

[megjablon, 2015-01-27 00:04:41]

Patchset #3 (id:140001) has been deleted

[megjablon, 2015-01-27 00:04:48]

Patchset #3 (id:160001) has been deleted

[megjablon, 2015-01-27 00:10:32]

On 2015/01/24 01:48:59, megjablon wrote:
>
https://codereview.chromium.org/791493015/diff/60001/chrome/app/generated_res...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/791493015/diff/60001/chrome/app/generated_res...
> chrome/app/generated_resources.grd:6384: +         Enable or disable using
Data
> Reduction Proxy lo-fi.
> On 2015/01/17 01:06:16, bengr wrote:
> > These should be "Enable Data Saver Lo-Fi mode" and "Put the Data Saver in
> Lo-Fi
> > mode when it is enabled."
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
> File
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_auth_request_handler.cc
> (right):
> 
>
https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_auth_request_handler.cc:173:
> header_value += ", q=low";
> On 2015/01/17 01:06:17, bengr wrote:
> > This has even less to do with authorization than does client type and
version.
> > You should write a new class that handles request options.
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
> File
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
> (right):
> 
>
https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
>
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:101:
> bool DataReductionProxyParams::IsLoFiCommandLineSwitchOn() {
> On 2015/01/17 01:06:17, bengr wrote:
> > I'd change this either to HasLowFiCommandLineSwitch() or IsLoFiEnabled(). I
> > prefer the latter because you will probably add other conditions that have
> > nothing to do with the switch.
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/60001/components/data_reductio...
>
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:102:
> if (base::CommandLine::ForCurrentProcess()->HasSwitch(
> On 2015/01/17 01:06:17, bengr wrote:
> > How about:
> > 
> > return base::CommandLine::ForCurrentProcess()->
> >    
HasSwitch(data_reduction_proxy::switches::kEnableDataReductionProxyLoFi);
> 
> Done.

Rebased and ready for review

[bengr, 2015-01-27 18:55:01]

https://codereview.chromium.org/791493015/diff/180001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
(right):

https://codereview.chromium.org/791493015/diff/180001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:88: Client
DataReductionProxyChromeSettings::GetClient() {
This requires the #include you removed, afaik.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:74:
std::map<std::string, std::string> header_options;
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.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:81:
if (DataReductionProxyParams::IsDataReductionProxyLoFiEnabled())
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.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:83:
SetRequestHeaderOptions(header_options);
If these are all being set internally, why bother with the function? Of course
you still need to call RegenerateRequestHeaderValue()

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:98:
header_options[kBuildNumber] = build_number_;
build_number_ and patch_number_ shouldn't be members.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:180:
void DataReductionProxyRequestOptions::AddAuthorizationHeader(
Maybe rename this as SetHeader()

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:183:
if (now - last_update_time_ > base::TimeDelta::FromHours(24)) {
Add a comment:

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

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:185:
ComputeCredentials(last_update_time_, &session_, &credentials_);
Call UpdateCredentials()

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:202:
void DataReductionProxyRequestOptions::ComputeCredentials(
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();
}

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:240:
ComputeCredentials(last_update_time_, &session_, &credentials_);
Call UpdateCredentials();

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:293:
for (std::map<std::string, std::string>::iterator
Another option here is to push the values into a vector and then call
JoinString(). That's arguably the tiniest bit cleaner.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:21:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h"
What requires this?

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:101:
// Returns true if this client has the command line switch to enable lo-fi
Lo-Fi?

[megjablon, 2015-01-27 23:44:11]

megjablon@chromium.org changed reviewers:
+ sgurun@chromium.org

[megjablon, 2015-01-27 23:44:13]

sgurun@chromium.org: android_webview/*

https://codereview.chromium.org/791493015/diff/180001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
(right):

https://codereview.chromium.org/791493015/diff/180001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:88: Client
DataReductionProxyChromeSettings::GetClient() {
On 2015/01/27 18:54:59, bengr wrote:
> This requires the #include you removed, afaik.

data_reduction_proxy_chrome_settings.h includes it.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:74:
std::map<std::string, std::string> header_options;
On 2015/01/27 18:54:59, bengr wrote:
> 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.

Done.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:81:
if (DataReductionProxyParams::IsDataReductionProxyLoFiEnabled())
On 2015/01/27 18:54:59, bengr wrote:
> 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.

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

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:83:
SetRequestHeaderOptions(header_options);
On 2015/01/27 18:55:00, bengr wrote:
> If these are all being set internally, why bother with the function? Of course
> you still need to call RegenerateRequestHeaderValue()

Done.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:98:
header_options[kBuildNumber] = build_number_;
On 2015/01/27 18:54:59, bengr wrote:
> build_number_ and patch_number_ shouldn't be members.

Done.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:180:
void DataReductionProxyRequestOptions::AddAuthorizationHeader(
On 2015/01/27 18:55:00, bengr wrote:
> Maybe rename this as SetHeader()

Done.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:183:
if (now - last_update_time_ > base::TimeDelta::FromHours(24)) {
On 2015/01/27 18:54:59, bengr wrote:
> Add a comment:
> 
> // Authorization credentials must be regenerated at least every 24 hours.

Done.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:185:
ComputeCredentials(last_update_time_, &session_, &credentials_);
On 2015/01/27 18:54:59, bengr wrote:
> Call UpdateCredentials()

Done.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:202:
void DataReductionProxyRequestOptions::ComputeCredentials(
On 2015/01/27 18:54:59, bengr wrote:
> 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();
> }

Done.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:240:
ComputeCredentials(last_update_time_, &session_, &credentials_);
On 2015/01/27 18:55:00, bengr wrote:
> Call UpdateCredentials();

Done.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:293:
for (std::map<std::string, std::string>::iterator
On 2015/01/27 18:55:00, bengr wrote:
> Another option here is to push the values into a vector and then call
> JoinString(). That's arguably the tiniest bit cleaner.

Done.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:21:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h"
On 2015/01/27 18:55:00, bengr wrote:
> What requires this?

Don't see anything that does. Removed.

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:101:
// Returns true if this client has the command line switch to enable lo-fi
On 2015/01/27 18:55:00, bengr wrote:
> Lo-Fi?

Done.

[sgurun-gerrit only, 2015-01-27 23:51:14]

On 2015/01/27 23:44:13, megjablon wrote:
> mailto:sgurun@chromium.org: android_webview/*
> 
>
https://codereview.chromium.org/791493015/diff/180001/chrome/browser/net/spdy...
> File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
> (right):
> 
>
https://codereview.chromium.org/791493015/diff/180001/chrome/browser/net/spdy...
> chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:88:
Client
> DataReductionProxyChromeSettings::GetClient() {
> On 2015/01/27 18:54:59, bengr wrote:
> > This requires the #include you removed, afaik.
> 
> data_reduction_proxy_chrome_settings.h includes it.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
> File
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
> (right):
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:74:
> std::map<std::string, std::string> header_options;
> On 2015/01/27 18:54:59, bengr wrote:
> > 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.
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:81:
> if (DataReductionProxyParams::IsDataReductionProxyLoFiEnabled())
> On 2015/01/27 18:54:59, bengr wrote:
> > 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.
> 
> I was planning on doing this when I start working on the menu, but I'll add it
> now.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:83:
> SetRequestHeaderOptions(header_options);
> On 2015/01/27 18:55:00, bengr wrote:
> > If these are all being set internally, why bother with the function? Of
course
> > you still need to call RegenerateRequestHeaderValue()
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:98:
> header_options[kBuildNumber] = build_number_;
> On 2015/01/27 18:54:59, bengr wrote:
> > build_number_ and patch_number_ shouldn't be members.
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:180:
> void DataReductionProxyRequestOptions::AddAuthorizationHeader(
> On 2015/01/27 18:55:00, bengr wrote:
> > Maybe rename this as SetHeader()
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:183:
> if (now - last_update_time_ > base::TimeDelta::FromHours(24)) {
> On 2015/01/27 18:54:59, bengr wrote:
> > Add a comment:
> > 
> > // Authorization credentials must be regenerated at least every 24 hours.
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:185:
> ComputeCredentials(last_update_time_, &session_, &credentials_);
> On 2015/01/27 18:54:59, bengr wrote:
> > Call UpdateCredentials()
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:202:
> void DataReductionProxyRequestOptions::ComputeCredentials(
> On 2015/01/27 18:54:59, bengr wrote:
> > 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();
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:240:
> ComputeCredentials(last_update_time_, &session_, &credentials_);
> On 2015/01/27 18:55:00, bengr wrote:
> > Call UpdateCredentials();
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:293:
> for (std::map<std::string, std::string>::iterator
> On 2015/01/27 18:55:00, bengr wrote:
> > Another option here is to push the values into a vector and then call
> > JoinString(). That's arguably the tiniest bit cleaner.
> 
> Done.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
> File
> components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
> (right):
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:21:
> #include
>
"components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h"
> On 2015/01/27 18:55:00, bengr wrote:
> > What requires this?
> 
> Don't see anything that does. Removed.
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
> File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
> (right):
> 
>
https://codereview.chromium.org/791493015/diff/180001/components/data_reducti...
> components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:101:
> // Returns true if this client has the command line switch to enable lo-fi
> On 2015/01/27 18:55:00, bengr wrote:
> > Lo-Fi?
> 
> Done.

I am not sure if this would work with webview apps or not. how will they turn it
off if it does not work?

[sgurun-gerrit only, 2015-01-27 23:51:51]

https://codereview.chromium.org/791493015/diff/200001/android_webview/native/...
File android_webview/native/aw_contents_statics.cc (right):

https://codereview.chromium.org/791493015/diff/200001/android_webview/native/...
android_webview/native/aw_contents_statics.cc:24: using
data_reduction_proxy::DataReductionProxyRequestOptions;
do we need this anymore?

[bengr, 2015-01-30 18:08:13]

lgtm

https://codereview.chromium.org/791493015/diff/200001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
(right):

https://codereview.chromium.org/791493015/diff/200001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:46:
const std::string kSession = "ps";
Ideally this wouldn't be duplicated here. Ok to postpone to another CL.

[megjablon, 2015-01-31 00:43:27]

Patchset #5 (id:220001) has been deleted

[megjablon, 2015-01-31 00:44:32]

https://codereview.chromium.org/791493015/diff/200001/android_webview/native/...
File android_webview/native/aw_contents_statics.cc (right):

https://codereview.chromium.org/791493015/diff/200001/android_webview/native/...
android_webview/native/aw_contents_statics.cc:24: using
data_reduction_proxy::DataReductionProxyRequestOptions;
On 2015/01/27 23:51:51, sgurun wrote:
> do we need this anymore?

Removed.

https://codereview.chromium.org/791493015/diff/200001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
(right):

https://codereview.chromium.org/791493015/diff/200001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:46:
const std::string kSession = "ps";
On 2015/01/30 18:08:13, bengr wrote:
> Ideally this wouldn't be duplicated here. Ok to postpone to another CL.

Fixed it in this one

[megjablon, 2015-01-31 00:52:15]

Patchset #5 (id:240001) has been deleted

[bengr, 2015-02-03 23:05:10]

On 2015/01/31 00:44:32, megjablon wrote:
>
https://codereview.chromium.org/791493015/diff/200001/android_webview/native/...
> File android_webview/native/aw_contents_statics.cc (right):
> 
>
https://codereview.chromium.org/791493015/diff/200001/android_webview/native/...
> android_webview/native/aw_contents_statics.cc:24: using
> data_reduction_proxy::DataReductionProxyRequestOptions;
> On 2015/01/27 23:51:51, sgurun wrote:
> > do we need this anymore?
> 
> Removed.
> 
>
https://codereview.chromium.org/791493015/diff/200001/components/data_reducti...
> File
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/791493015/diff/200001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:46:
> const std::string kSession = "ps";
> On 2015/01/30 18:08:13, bengr wrote:
> > Ideally this wouldn't be duplicated here. Ok to postpone to another CL.
> 
> Fixed it in this one

still lgtm

[sgurun-gerrit only, 2015-02-03 23:08:18]

megjablon: I don't think you responded to my comment above, maybe I missed it,
so repeating again.
do we think this is safe for webview using apps? what impact do we expect?

[megjablon, 2015-02-04 00:09:13]

On 2015/02/03 23:08:18, sgurun wrote:
> megjablon: I don't think you responded to my comment above, maybe I missed it,
> so repeating again.
> do we think this is safe for webview using apps? what impact do we expect?

Sorry I missed it when responding to the comments. We have no plans to launch
this for webview. This is Chrome Android only and currently an experiment, but
we can discuss offline.

[sgurun-gerrit only, 2015-02-04 01:00:22]

On 2015/02/04 00:09:13, megjablon wrote:
> On 2015/02/03 23:08:18, sgurun wrote:
> > megjablon: I don't think you responded to my comment above, maybe I missed
it,
> > so repeating again.
> > do we think this is safe for webview using apps? what impact do we expect?
> 
> Sorry I missed it when responding to the comments. We have no plans to launch
> this for webview. This is Chrome Android only and currently an experiment, but
> we can discuss offline.

how do we differentiate between webview and chrome?

[megjablon, 2015-02-04 02:13:56]

On 2015/02/04 01:00:22, sgurun wrote:
> On 2015/02/04 00:09:13, megjablon wrote:
> > On 2015/02/03 23:08:18, sgurun wrote:
> > > megjablon: I don't think you responded to my comment above, maybe I missed
> it,
> > > so repeating again.
> > > do we think this is safe for webview using apps? what impact do we expect?
> > 
> > Sorry I missed it when responding to the comments. We have no plans to
launch
> > this for webview. This is Chrome Android only and currently an experiment,
but
> > we can discuss offline.
> 
> how do we differentiate between webview and chrome?

The Lo-Fi mechanism is available on all platforms, but is currently only turned
on by a flag in about_flags. We will have Chrome ui to turn it on in the future,
but are only targeting Chrome at this time. This could be in webview in the
future.

[sgurun-gerrit only, 2015-02-04 02:16:44]

On 2015/02/04 02:13:56, megjablon wrote:
> On 2015/02/04 01:00:22, sgurun wrote:
> > On 2015/02/04 00:09:13, megjablon wrote:
> > > On 2015/02/03 23:08:18, sgurun wrote:
> > > > megjablon: I don't think you responded to my comment above, maybe I
missed
> > it,
> > > > so repeating again.
> > > > do we think this is safe for webview using apps? what impact do we
expect?
> > > 
> > > Sorry I missed it when responding to the comments. We have no plans to
> launch
> > > this for webview. This is Chrome Android only and currently an experiment,
> but
> > > we can discuss offline.
> > 
> > how do we differentiate between webview and chrome?
> 
> The Lo-Fi mechanism is available on all platforms, but is currently only
turned
> on by a flag in about_flags. We will have Chrome ui to turn it on in the
future,
> but are only targeting Chrome at this time. This could be in webview in the
> future.

lgtm

[megjablon, 2015-02-04 23:00:49]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2015-02-04 23:00:52]

The CQ bit was unchecked by megjablon@chromium.org

[megjablon, 2015-02-04 23:48:48]

New patchsets have been uploaded after l-g-t-m from
bengr@chromium.org,sgurun@chromium.org

[megjablon, 2015-02-05 05:27:46]

megjablon@chromium.org changed reviewers:
+ asvitkine@chromium.org

[megjablon, 2015-02-05 05:27:47]

asvitkine: histograms.xml

[Alexei Svitkine (slow), 2015-02-05 17:12:43]

lgtm

[megjablon, 2015-02-05 18:40:25]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2015-02-05 18:41:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/791493015/300001

[commit-bot: I haz the power, 2015-02-05 18:48:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-05 18:48:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[megjablon, 2015-02-05 18:59:36]

New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org

[megjablon, 2015-02-05 19:38:25]

Patchset #8 (id:320001) has been deleted

[megjablon, 2015-02-06 03:02:30]

Patchset #9 (id:360001) has been deleted

[megjablon, 2015-02-06 03:04:56]

Patchset #9 (id:380001) has been deleted

[megjablon, 2015-02-06 07:02:04]

Patchset #10 (id:420001) has been deleted

[megjablon, 2015-02-06 07:55:06]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2015-02-06 07:56:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/791493015/440001

[commit-bot: I haz the power, 2015-02-06 08:06:15]

Committed patchset #10 (id:440001)

[commit-bot: I haz the power, 2015-02-06 08:07:48]

Patchset 10 (id:??) landed as
https://crrev.com/8fbff4fad29b9e2408c634e178386d456acf6467
Cr-Commit-Position: refs/heads/master@{#315000}

[apavlov, 2015-02-06 14:40:10]

apavlov@chromium.org changed reviewers:
+ apavlov@chromium.org

[apavlov, 2015-02-06 14:40:12]

Speculatively reverted as
https://crrev.com/adb1d7096d2c2da9a5843edc718378663587ef91
(https://codereview.chromium.org/903073003/) due to multiple browser_tests
failures.

[megjablon, 2015-02-06 20:02:29]

Patchset #6 (id:280001) has been deleted

[megjablon, 2015-02-06 21:54:19]

Patchset #10 (id:460001) has been deleted