Move adding Lo-Fi directives from DRPRequestOptions to ContentLoFiDecider

Move adding Lo-Fi directives from DRPRequestOptions to ContentLoFiDecider

All Lo-Fi directives for the "Chrome-Proxy" header should be added by the
ContentLoFiDecider. This includes "q=low" and "exp=lofi_active_control".

BUG=539934
Committed: https://crrev.com/563061cf9adac13722ec27c88ed6806c99462678
Cr-Commit-Position: refs/heads/master@{#361386}

[megjablon, 2015-11-19 21:58:30]

Patchset #1 (id:1) has been deleted

[megjablon, 2015-11-20 00:37:02]

Patchset #1 (id:20001) has been deleted

[megjablon, 2015-11-20 00:37:12]

Patchset #1 (id:40001) has been deleted

[megjablon, 2015-11-20 18:10:21]

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

[megjablon, 2015-11-20 20:59:04]

Description was changed from

==========
Move adding Lo-Fi directives from DRPRequestOptions to ContentLoFiDecider

All Lo-Fi directives for the "Chrome-Proxy" header should be added by the
ContentLoFiDecider. This includes "q=low" and "exp=lofi_active_control".

BUG=539934
==========

to

==========
Move adding Lo-Fi directives from DRPRequestOptions to ContentLoFiDecider

All Lo-Fi directives for the "Chrome-Proxy" header should be added by the
ContentLoFiDecider. This includes "q=low" and "exp=lofi_active_control".

BUG=539934
==========

[megjablon, 2015-11-20 20:59:04]

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

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

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:15:
const char kLoFiHeader[] = "q=low";
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?

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:47:
params::IsIncludedInLoFiEnabledFieldTrial();
Is it possible to move this common code to anonymous namespace?

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:49:
const char kChromeProxyHeader[] = "Chrome-Proxy";
Same here, is it possible to move this to params?

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:66:
params::IsIncludedInLoFiControlFieldTrial()) {
Should this also check that Lo-Fi is not enabled from flags?
 && !params::IsLoFiOnViaFlags()

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:72:
header_value += kLoFiExperimentHeader;
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?

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
File components/data_reduction_proxy/content/browser/content_lofi_decider.h
(right):

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.h:13: class
HttpRequestHeaders;
Alphabetize.

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.h:33: bool
MaybeEnableLoFiMode(const net::URLRequest& request,
// LoFiDecider implementation:


Also, this is not really enabling a mode..right? I would suggest
MaybeAddLoFiDirectiveToHeaders but it might be too long.

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
File components/data_reduction_proxy/core/common/lofi_decider.h (right):

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/core/common/lofi_decider.h:28: // |request| is
using Lo-Fi mode, adds the "q=low" directive to the |headers|.
May be also say something about lofi_active_control?

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

I'll defer this review to tbansal, just added a quick comment.

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:41: if
(request_info) {
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]

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:15:
const char kLoFiHeader[] = "q=low";
On 2015/11/20 21:56:58, tbansal1 wrote:
> 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?

Just using the value from data_reduction_proxy_headers.cc instead.

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:41: if
(request_info) {
On 2015/11/20 22:59:11, sclittle wrote:
> 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?

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.

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:47:
params::IsIncludedInLoFiEnabledFieldTrial();
On 2015/11/20 21:56:58, tbansal1 wrote:
> Is it possible to move this common code to anonymous namespace?

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

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:49:
const char kChromeProxyHeader[] = "Chrome-Proxy";
On 2015/11/20 21:56:58, tbansal1 wrote:
> Same here, is it possible to move this to params?

Just using the value from data_reduction_proxy_headers.cc instead.

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:66:
params::IsIncludedInLoFiControlFieldTrial()) {
On 2015/11/20 21:56:58, tbansal1 wrote:
> Should this also check that Lo-Fi is not enabled from flags?
>  && !params::IsLoFiOnViaFlags()

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.

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:72:
header_value += kLoFiExperimentHeader;
On 2015/11/20 21:56:58, tbansal1 wrote:
> 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?

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

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

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
File components/data_reduction_proxy/content/browser/content_lofi_decider.h
(right):

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.h:13: class
HttpRequestHeaders;
On 2015/11/20 21:56:58, tbansal1 wrote:
> Alphabetize.

Done.

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.h:33: bool
MaybeEnableLoFiMode(const net::URLRequest& request,
On 2015/11/20 21:56:58, tbansal1 wrote:
> // LoFiDecider implementation:
> 
> 
> Also, this is not really enabling a mode..right? I would suggest
> MaybeAddLoFiDirectiveToHeaders but it might be too long.

SGTM. It's only called in one place so I don't think that's too long.

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
File components/data_reduction_proxy/core/common/lofi_decider.h (right):

https://chromiumcodereview.appspot.com/1463583003/diff/60001/components/data_...
components/data_reduction_proxy/core/common/lofi_decider.h:28: // |request| is
using Lo-Fi mode, adds the "q=low" directive to the |headers|.
On 2015/11/20 21:56:58, tbansal1 wrote:
> May be also say something about lofi_active_control?

Done.

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

https://codereview.chromium.org/1463583003/diff/60001/components/data_reducti...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/1463583003/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:47:
params::IsIncludedInLoFiEnabledFieldTrial();
On 2015/11/20 23:14:50, megjablon wrote:
> On 2015/11/20 21:56:58, tbansal1 wrote:
> > Is it possible to move this common code to anonymous namespace?
> 
> I don't see the need to move this. We don't use this logic anywhere else.

Line 26 up.

https://codereview.chromium.org/1463583003/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:66:
params::IsIncludedInLoFiControlFieldTrial()) {
On 2015/11/20 23:14:50, megjablon wrote:
> On 2015/11/20 21:56:58, tbansal1 wrote:
> > Should this also check that Lo-Fi is not enabled from flags?
> >  && !params::IsLoFiOnViaFlags()
> 
> 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.

Aah, did not see the return statement.

https://codereview.chromium.org/1463583003/diff/80001/components/data_reducti...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/1463583003/diff/80001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:24: //
trial "Control" group. Check that the user is in a group that should
s/should/can?

https://codereview.chromium.org/1463583003/diff/80001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:29:
return request_info->IsUsingLoFi() && lofi_enabled_via_flag_or_field_trial;
Is it possible that "request_info->IsUsingLoFi()" is true even though
|lofi_enabled_via_flag_or_field_trial| is false?
Please add a comment when this may happen.

https://codereview.chromium.org/1463583003/diff/80001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:39: if
(request_info) {
nit to reduce indentation:
if(!request_info)
  return false;

https://codereview.chromium.org/1463583003/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(right):

https://codereview.chromium.org/1463583003/diff/80001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:294:
false, false, false,
Can you please make these compact (like Lines 242-250). Thanks.

https://codereview.chromium.org/1463583003/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/1463583003/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:180:
bool is_using_lofi_mode =
Seems like this temp variable is not needed or at least it could be put inside
the if conditional below.

[megjablon, 2015-11-23 20:53:55]

https://chromiumcodereview.appspot.com/1463583003/diff/80001/components/data_...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://chromiumcodereview.appspot.com/1463583003/diff/80001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:24: //
trial "Control" group. Check that the user is in a group that should
On 2015/11/23 20:39:52, tbansal1 wrote:
> s/should/can?

Done.

https://chromiumcodereview.appspot.com/1463583003/diff/80001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:29:
return request_info->IsUsingLoFi() && lofi_enabled_via_flag_or_field_trial;
On 2015/11/23 20:39:52, tbansal1 wrote:
> Is it possible that "request_info->IsUsingLoFi()" is true even though
> |lofi_enabled_via_flag_or_field_trial| is false?
> Please add a comment when this may happen.

Done.

https://chromiumcodereview.appspot.com/1463583003/diff/80001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:39: if
(request_info) {
On 2015/11/23 20:39:52, tbansal1 wrote:
> nit to reduce indentation:
> if(!request_info)
>   return false;

Done.

https://chromiumcodereview.appspot.com/1463583003/diff/80001/components/data_...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(right):

https://chromiumcodereview.appspot.com/1463583003/diff/80001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:294:
false, false, false,
On 2015/11/23 20:39:52, tbansal1 wrote:
> Can you please make these compact (like Lines 242-250). Thanks.

Done.

https://chromiumcodereview.appspot.com/1463583003/diff/80001/components/data_...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://chromiumcodereview.appspot.com/1463583003/diff/80001/components/data_...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:180:
bool is_using_lofi_mode =
On 2015/11/23 20:39:52, tbansal1 wrote:
> Seems like this temp variable is not needed or at least it could be put inside
> the if conditional below.

The conditional below only runs on mainframe requests. We need to add the
headers on all requests and then let the UI thread know if we added the headers
for only mainframes.

https://chromiumcodereview.appspot.com/1463583003/diff/100001/components/data...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(right):

https://chromiumcodereview.appspot.com/1463583003/diff/100001/components/data...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:295:
{true, false, true},   {true, true, true},
git cl format did this

[tbansal1, 2015-11-23 20:55:06]

lgtm

[megjablon, 2015-11-24 17:56:59]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2015-11-24 17:57:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463583003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463583003/100001

[commit-bot: I haz the power, 2015-11-24 18:22:20]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-11-24 18:23:22]

Patchset 3 (id:??) landed as
https://crrev.com/563061cf9adac13722ec27c88ed6806c99462678
Cr-Commit-Position: refs/heads/master@{#361386}