Expose NQE::OnEffectiveConnectionTypeChanged API

Expose NQE::OnEffectiveConnectionTypeChanged API

NQE maintains a list of thread-unsafe observers. It also
computes the effective connection type at regular intervals,
provided the device is actively receiving traffic from the
Internet. If there is a change in the effective connection
type, all the observers are notified.

BUG=613755
Committed: https://crrev.com/7c6ca1c7bccc7e61bc98adc9eda8a4ebd58b6423
Cr-Commit-Position: refs/heads/master@{#396976}

[tbansal1, 2016-05-23 20:37:25]

Description was changed from

==========
Expose NQE::OnEffectiveConnectionTypeChanged API

BUG=
==========

to

==========
Expose NQE::OnEffectiveConnectionTypeChanged API

NQE maintains a list of thread-unsafe observers. It also
computes the effective connection type at regular intervals,
provided the device is actively receiving traffic from the
Internet. If there is a change in the effective connection 
type, all the observers are notified.

BUG=613755
==========

[tbansal1, 2016-05-23 20:38:14]

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

[tbansal1, 2016-05-23 20:38:14]

bengr: PTAL. I still need to add hysteresis. If this looks good, I will file a
bug for that and add the hysteresis in the next CL.

[bengr, 2016-05-27 19:46:02]

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.cc:1100: // May be recompute the effective
connection type since a new RTT observation
May be -> Maybe

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.cc:1149: last_connection_change_ <
last_effective_connection_type_computation_) {
nit: Why is this < and not <=?

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.cc:1155: effective_connection_type_ =
GetEffectiveConnectionType();
If we never change type, don't we wind up computing this on every read?

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:80: // Observes changes in effective
connection type.
What is the contract? The consumer will be notified of an effective connection
type change no more than once every k seconds? Will the consumer be notified
immediately? Will consumers be notified of ect on actual connection changes?
Please document.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:300: void
SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock);
Add a comment.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:422: void
MaybeRecomputeEffectiveConnectionType();
Add a comment.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:424: void
NotifyObserversOfEffectiveConnectionTypeChanged();
Add a comment.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:456: std::unique_ptr<base::TickClock>
tick_clock_;
Add a comment.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:460: const base::TimeDelta
effective_connection_type_recomputation_interval_;
Is this for hysteresis or performance or both?

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:523: base::TimeTicks
last_effective_connection_type_computation_;
Should this be listed together with line 460?

[tbansal1, 2016-05-28 00:56:23]

Patchset #3 (id:40001) has been deleted

[tbansal1, 2016-05-28 01:20:42]

bengr: ptal. Thanks.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.cc:1100: // May be recompute the effective
connection type since a new RTT observation
On 2016/05/27 19:46:01, bengr wrote:
> May be -> Maybe

Done.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.cc:1149: last_connection_change_ <
last_effective_connection_type_computation_) {
On 2016/05/27 19:46:01, bengr wrote:
> nit: Why is this < and not <=?

Changed both to < for consistency. Also, added comments to better explain the
reason.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.cc:1155: effective_connection_type_ =
GetEffectiveConnectionType();
On 2016/05/27 19:46:01, bengr wrote:
> If we never change type, don't we wind up computing this on every read?

If there is no connection change, then there would be at most one computation in
any time-interval of duration
|effective_connection_type_recomputation_interval_|

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:80: // Observes changes in effective
connection type.
On 2016/05/27 19:46:01, bengr wrote:
> What is the contract? The consumer will be notified of an effective connection
> type change no more than once every k seconds? Will the consumer be notified
> immediately? Will consumers be notified of ect on actual connection changes?
> Please document.

Done.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:300: void
SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock);
On 2016/05/27 19:46:02, bengr wrote:
> Add a comment.

Done.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:422: void
MaybeRecomputeEffectiveConnectionType();
On 2016/05/27 19:46:02, bengr wrote:
> Add a comment.

Done.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:424: void
NotifyObserversOfEffectiveConnectionTypeChanged();
On 2016/05/27 19:46:02, bengr wrote:
> Add a comment.

Done.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:456: std::unique_ptr<base::TickClock>
tick_clock_;
On 2016/05/27 19:46:01, bengr wrote:
> Add a comment.

Done, but this one seems pretty self-explanatory.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:460: const base::TimeDelta
effective_connection_type_recomputation_interval_;
On 2016/05/27 19:46:02, bengr wrote:
> Is this for hysteresis or performance or both?

Performance. Hysteresis means we notify observers only if we are definitely sure
that connection type has changed.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.h:523: base::TimeTicks
last_effective_connection_type_computation_;
On 2016/05/27 19:46:01, bengr wrote:
> Should this be listed together with line 460?

Done.

[bengr, 2016-05-31 17:39:25]

lgtm with nits.

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2007713002/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator.cc:1155: effective_connection_type_ =
GetEffectiveConnectionType();
On 2016/05/28 01:20:42, tbansal1 wrote:
> On 2016/05/27 19:46:01, bengr wrote:
> > If we never change type, don't we wind up computing this on every read?
> 
> If there is no connection change, then there would be at most one computation
in
> any time-interval of duration
> |effective_connection_type_recomputation_interval_|

Acknowledged.

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:83: // Notifies the observer of change in
the effective connection type.
of -> of a

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:84: // NetworkQualityEstimator computes
effective connection type once in every
computes -> computes the

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:92: virtual void
OnEffectiveConnectionTypeChanged(
This is not threadsafe, right? Say so.

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:435: // Notify observers of change in
effective connection type.
of -> of a

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:468: // Tick clock used by network quality
estimator.
by -> by the

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:472: // connection type.
Add "Non-zero as a performance optimization."

[tbansal1, 2016-05-31 21:40:26]

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:83: // Notifies the observer of change in
the effective connection type.
On 2016/05/31 17:39:25, bengr wrote:
> of -> of a 

Done.

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:84: // NetworkQualityEstimator computes
effective connection type once in every
On 2016/05/31 17:39:25, bengr wrote:
> computes -> computes the

Done.

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:92: virtual void
OnEffectiveConnectionTypeChanged(
On 2016/05/31 17:39:25, bengr wrote:
> This is not threadsafe, right? Say so.

Done.

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:435: // Notify observers of change in
effective connection type.
On 2016/05/31 17:39:24, bengr wrote:
> of -> of a

Done.

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:468: // Tick clock used by network quality
estimator.
On 2016/05/31 17:39:24, bengr wrote:
> by -> by the

Done.

https://codereview.chromium.org/2007713002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:472: // connection type.
On 2016/05/31 17:39:25, bengr wrote:
> Add "Non-zero as a performance optimization."

Done.

[tbansal1, 2016-05-31 21:46:55]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-05-31 21:46:56]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2007713002/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-05-31 21:47:25]

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

[commit-bot: I haz the power, 2016-05-31 23:42:43]

Description was changed from

==========
Expose NQE::OnEffectiveConnectionTypeChanged API

NQE maintains a list of thread-unsafe observers. It also
computes the effective connection type at regular intervals,
provided the device is actively receiving traffic from the
Internet. If there is a change in the effective connection 
type, all the observers are notified.

BUG=613755
==========

to

==========
Expose NQE::OnEffectiveConnectionTypeChanged API

NQE maintains a list of thread-unsafe observers. It also
computes the effective connection type at regular intervals,
provided the device is actively receiving traffic from the
Internet. If there is a change in the effective connection 
type, all the observers are notified.

BUG=613755
==========

[commit-bot: I haz the power, 2016-05-31 23:42:45]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-05-31 23:44:13]

Description was changed from

==========
Expose NQE::OnEffectiveConnectionTypeChanged API

NQE maintains a list of thread-unsafe observers. It also
computes the effective connection type at regular intervals,
provided the device is actively receiving traffic from the
Internet. If there is a change in the effective connection 
type, all the observers are notified.

BUG=613755
==========

to

==========
Expose NQE::OnEffectiveConnectionTypeChanged API

NQE maintains a list of thread-unsafe observers. It also
computes the effective connection type at regular intervals,
provided the device is actively receiving traffic from the
Internet. If there is a change in the effective connection
type, all the observers are notified.

BUG=613755

Committed: https://crrev.com/7c6ca1c7bccc7e61bc98adc9eda8a4ebd58b6423
Cr-Commit-Position: refs/heads/master@{#396976}
==========

[commit-bot: I haz the power, 2016-05-31 23:44:14]

Patchset 6 (id:??) landed as
https://crrev.com/7c6ca1c7bccc7e61bc98adc9eda8a4ebd58b6423
Cr-Commit-Position: refs/heads/master@{#396976}