Trigger LoFi based on NQE and use hysteresis to prevent frequent changes in network quality

Reads network quality provided by the NQE and compares it
with field trial parameters to determine if the network is
prohibitively slow.

Hysteresis is added so that at least k seconds transpire
before the state of the network (prohibitively slow, or
not) is determined again. k is a parameter of the field
trial.

BUG=470596
Committed: https://crrev.com/99083a7907bb738b4c8a66e0ed2aff394024b5ea
Cr-Commit-Position: refs/heads/master@{#333609}

[tbansal1, 2015-06-05 22:28:54]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2015-06-05 22:45:21]

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

[tbansal1, 2015-06-05 22:45:21]

ptal.

[bengr, 2015-06-09 00:40:10]

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:200:
auto_lofi_maximum_kbps_(UINT64_MAX),
I thought these were going to be int64_t's. Also #include <stdint.h>

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:383:
if (network_quality_last_updated_ != base::TimeTicks() &&
if (!network_quality_last_updated_.is_null() && ...

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:390:
network_prohibitively_slow_ = false;
I was confused by this. Maybe move it down so tha tyou have:

if (network_quality.peak_throughput_kbps > 0 && ... ) {
  network_prohibitively_slow_ = true;
} else {
  network_prohibitively_slow_ = false;
}
return network_prohibitively_slow_;

Alternatively, you can do:

network_prohibitively_slow_ = network_quality.peak_throughput_kbps > 0 && ... ;
return network_prohibitively_slow_;

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

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:394:
// True iff network was detected as prohibitively slow when the quality was
// True iff the network was determined to be prohibitively slow when the network
quality was last updated.

When is it updated?

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc
(right):

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:55:
const net::NetworkQualityEstimator* network_quality_estimator) {
Why no more const?

[tbansal1, 2015-06-09 02:48:45]

ptal.

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:200:
auto_lofi_maximum_kbps_(UINT64_MAX),
On 2015/06/09 00:40:09, bengr wrote:
> I thought these were going to be int64_t's. Also #include <stdint.h>

Thats currently happening in a separate CL:
https://codereview.chromium.org/1164713004/
It is changing to int32_t.

stdint.h is included in drp_config.h

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:383:
if (network_quality_last_updated_ != base::TimeTicks() &&
On 2015/06/09 00:40:09, bengr wrote:
> if (!network_quality_last_updated_.is_null() && ...

Done.

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:390:
network_prohibitively_slow_ = false;
On 2015/06/09 00:40:09, bengr wrote:
> I was confused by this. Maybe move it down so tha tyou have:
> 
> if (network_quality.peak_throughput_kbps > 0 && ... ) {
>   network_prohibitively_slow_ = true;
> } else {
>   network_prohibitively_slow_ = false;
> }
> return network_prohibitively_slow_;
> 
> Alternatively, you can do:
> 
> network_prohibitively_slow_ = network_quality.peak_throughput_kbps > 0 && ...
;
> return network_prohibitively_slow_;

Done.

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

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:394:
// True iff network was detected as prohibitively slow when the quality was
On 2015/06/09 00:40:09, bengr wrote:
> // True iff the network was determined to be prohibitively slow when the
network
> quality was last updated.
> 
> When is it updated?

Done.

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc
(right):

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:55:
const net::NetworkQualityEstimator* network_quality_estimator) {
On 2015/06/09 00:40:09, bengr wrote:
> Why no more const?

the base class function is not a const anymore (because now it updates member
variables).
This can't be const either because signatures need to match.

[tbansal1, 2015-06-09 02:49:32]

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

[tbansal1, 2015-06-09 02:49:33]

mmenke@chromium.org: Please review changes in 
network_quality_estimator.h

[tbansal1, 2015-06-09 02:50:01]

mmenke@chromium.org: Please review changes in 

network_quality_estimator.h

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

Please be sure to not apply hysteresis across network connection changes. E.g.,
if a user is on a bad network and switches to a good one, we should immediately
consider changing to the good state.

[mmenke, 2015-06-09 17:26:06]

mmenke@chromium.org changed reviewers:
+ pauljensen@chromium.org

[mmenke, 2015-06-09 17:26:07]

[+pauljensen]:  Hey Paul, mind taking these on?  Despite my inability not to
comment, I'd like to get out of the critical path on these CLs.

[tbansal1, 2015-06-09 17:47:03]

ptal. Updated to not apply hysteresis across network change events.

[pauljensen, 2015-06-09 17:49:03]

network_quality_estimator.h lgtm

[bengr, 2015-06-09 21:18:52]

lgtm, but please update the CL description. Here's what I would write (presuming
this is true):

Reads network quality provided by the NQE and compares it
with field trial parameters to determine if the network is
prohibitively slow.

Hysteresis is added so that at least k seconds transpire before the state of the
network (prohibitively slow, or not) is determined again. k is a parameter of
the field trial.

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:200:
auto_lofi_maximum_kbps_(UINT64_MAX),
On 2015/06/09 02:48:45, tbansal1 wrote:
> On 2015/06/09 00:40:09, bengr wrote:
> > I thought these were going to be int64_t's. Also #include <stdint.h>
> 
> Thats currently happening in a separate CL:
> https://codereview.chromium.org/1164713004/
> It is changing to int32_t.
> 
> stdint.h is included in drp_config.h

Acknowledged.

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc
(right):

https://codereview.chromium.org/1153543016/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:55:
const net::NetworkQualityEstimator* network_quality_estimator) {
On 2015/06/09 02:48:45, tbansal1 wrote:
> On 2015/06/09 00:40:09, bengr wrote:
> > Why no more const?
> 
> the base class function is not a const anymore (because now it updates member
> variables).
> This can't be const either because signatures need to match.

Acknowledged.

[tbansal1, 2015-06-09 21:29:34]

Updated the issue description and title.

[tbansal1, 2015-06-09 21:29:43]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2015-06-09 21:30:08]

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

[commit-bot: I haz the power, 2015-06-09 23:44:29]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-06-09 23:45:19]

Patchset 4 (id:??) landed as
https://crrev.com/99083a7907bb738b4c8a66e0ed2aff394024b5ea
Cr-Commit-Position: refs/heads/master@{#333609}