Set exp id in the CP header if user is in Lo-Fi control.

Set exp id in the CP header if user is in Lo-Fi control.

This will help us do performance analysis on the web pages.

BUG=538208
Committed: https://crrev.com/5e0bd99c4011ab4592eb2ecefdae4430d08fdc41
Cr-Commit-Position: refs/heads/master@{#352095}

[tbansal1, 2015-10-01 01:02:40]

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

[tbansal1, 2015-10-01 01:02:40]

ptal. thanks.

[tbansal1, 2015-10-01 16:35:39]

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

[megjablon, 2015-10-01 18:54:44]

https://chromiumcodereview.appspot.com/1380933002/diff/1/components/data_redu...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://chromiumcodereview.appspot.com/1380933002/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:174:
bool regenerate = false;
Do we need this? It's more lines than just calling
RegenerateRequestHeaderValue() when we set regenerate = true;

https://chromiumcodereview.appspot.com/1380933002/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:187:
// User was not part of Lo-Fi control experiment, but now is.
We also need the case where the user is part of the active control experiment
and then isn't anymore which removes the experiment. 

Also, can we use IsInLoFiActiveControlExperiment instead? Otherwise it just
sounds like we're checking the field trial value.

https://chromiumcodereview.appspot.com/1380933002/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:200:
experiments_.clear();
Why is this needed? This is only called on initialization so there shouldn't be
any experiments yet.

https://chromiumcodereview.appspot.com/1380933002/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:207:
}
UpdateExperiments() is only called from Init(). Do we need to worry about the
Lo-Fi control experiment in UpdateExperiments() since it will be updated on the
first call to MaybeAddRequestHeader().

https://chromiumcodereview.appspot.com/1380933002/diff/1/components/data_redu...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
(right):

https://chromiumcodereview.appspot.com/1380933002/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:456:
bool lofi_experiment;
lofi_active_control ?

[tbansal1, 2015-10-02 16:50:43]

ptal. thanks.

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:174:
bool regenerate = false;
On 2015/10/01 18:54:44, megjablon wrote:
> Do we need this? It's more lines than just calling
> RegenerateRequestHeaderValue() when we set regenerate = true;

Done.

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:187:
// User was not part of Lo-Fi control experiment, but now is.
On 2015/10/01 18:54:44, megjablon wrote:
> We also need the case where the user is part of the active control experiment
> and then isn't anymore which removes the experiment. 
> 
> Also, can we use IsInLoFiActiveControlExperiment instead? Otherwise it just
> sounds like we're checking the field trial value.

Done.

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:200:
experiments_.clear();
On 2015/10/01 18:54:44, megjablon wrote:
> Why is this needed? This is only called on initialization so there shouldn't
be
> any experiments yet.

Done.

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:207:
}
On 2015/10/01 18:54:44, megjablon wrote:
> UpdateExperiments() is only called from Init(). Do we need to worry about the
> Lo-Fi control experiment in UpdateExperiments() since it will be updated on
the
> first call to MaybeAddRequestHeader().

Done.

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
(right):

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:456:
bool lofi_experiment;
On 2015/10/01 18:54:44, megjablon wrote:
> lofi_active_control ?

obsolete.

[megjablon, 2015-10-02 17:03:51]

lgtm % nit

https://codereview.chromium.org/1380933002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1380933002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:225:
// if user is in Control group, and connection is slow.
nit: 
Returns true if the session is in the Lo-Fi control experiment. This happens if
user is in Control group, and connection is slow.if the user is in Control
group, and the connection is slow.

[tbansal1, 2015-10-02 17:35:59]

https://codereview.chromium.org/1380933002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1380933002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:225:
// if user is in Control group, and connection is slow.
On 2015/10/02 17:03:51, megjablon wrote:
> nit: 
> Returns true if the session is in the Lo-Fi control experiment. This happens
if
> user is in Control group, and connection is slow.if the user is in Control
> group, and the connection is slow.

Done.

[tbansal1, 2015-10-02 18:50:48]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2015-10-02 18:50:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
megjablon@chromium.org
Link to the patchset: https://codereview.chromium.org/1380933002/#ps40001
(title: "Rebased, addressed comment")

[commit-bot: I haz the power, 2015-10-02 18:51:48]

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

[commit-bot: I haz the power, 2015-10-02 19:00:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-02 19:01:11]

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

[bengr, 2015-10-02 19:34:29]

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:224:
// Returns true if the session is in Lo-Fi control experiment. This happens
// Returns true if the session is in the control group of the Lo-Fi experiment
and Lo-Fi mode is activated.

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:226:
bool IsInLoFiControlExperiment() const;
I don't like the name, probably because I don't like the function because it
combines concerns. I think there should be two methods:
IsInLoFiExperimentControlGroup() and
IsLofiActive()

[tbansal1, 2015-10-02 20:28:38]

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1380933002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:226:
bool IsInLoFiControlExperiment() const;
On 2015/10/02 19:34:29, bengr wrote:
> I don't like the name, probably because I don't like the function because it
> combines concerns. I think there should be two methods:
> IsInLoFiExperimentControlGroup() and
> IsLofiActive()

It will be:
IsInLoFiControlFieldTrial() && IsLofiActive() && !IsLofiActiveViaFlags()

I thought it would be better to keep this logic within config.cc, and not spread
it out to different files. wdyt?