Reduce the number of calls to external estimate provider

Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get
called multiple times per connection change. This CL ensures
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the logic easier to
understand.

BUG=604417
Committed: https://crrev.com/bb48cb54901af25602c71fed388756a76e8499c6
Cr-Commit-Position: refs/heads/master@{#396938}

[tbansal1, 2016-05-26 01:11:44]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-05-26 01:19:13]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2016-05-26 01:21:49]

Description was changed from

==========
Query external estimate provider exactly once per connection change

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

to

==========
Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get 
called multiple times per connection change. This CL ensures 
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the code easier to
understand.

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

[tbansal1, 2016-05-26 01:21:58]

Description was changed from

==========
Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get 
called multiple times per connection change. This CL ensures 
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the code easier to
understand.

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

to

==========
Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get 
called multiple times per connection change. This CL ensures 
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the logic easier to
understand.

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

[tbansal1, 2016-05-26 01:23:22]

Description was changed from

==========
Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get 
called multiple times per connection change. This CL ensures 
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the logic easier to
understand.

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

to

==========
Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get 
called multiple times per connection change. This CL ensures 
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the logic easier to
understand.

BUG=604417
==========

[tbansal1, 2016-05-26 01:23:41]

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

[tbansal1, 2016-05-26 01:23:42]

bengr: ptal. Thanks.

[tbansal1, 2016-05-26 02:48:58]

Patchset #3 (id:80001) has been deleted

[tbansal1, 2016-05-26 02:49:14]

Patchset #2 (id:60001) has been deleted

[tbansal1, 2016-05-26 02:55:58]

Patchset #2 (id:100001) has been deleted

[tbansal1, 2016-05-26 02:58:16]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2016-05-26 23:37:35]

Patchset #1 (id:120001) has been deleted

[bengr, 2016-05-27 19:58:42]

https://codereview.chromium.org/2010003002/diff/140001/chrome/browser/android...
File chrome/browser/android/net/external_estimate_provider_android.cc (right):

https://codereview.chromium.org/2010003002/diff/140001/chrome/browser/android...
chrome/browser/android/net/external_estimate_provider_android.cc:132: if
(!GetRTT(&rtt))
Shouldn't GetRTT not modify &rtt if it returns false?

https://codereview.chromium.org/2010003002/diff/140001/chrome/browser/android...
File chrome/browser/android/net/external_estimate_provider_android_unittest.cc
(left):

https://codereview.chromium.org/2010003002/diff/140001/chrome/browser/android...
chrome/browser/android/net/external_estimate_provider_android_unittest.cc:103:
// Updated once during NetworkQualityEstimator constructor and again,
Why is this no longer needed? (lines 94-118)

https://codereview.chromium.org/2010003002/diff/140001/net/nqe/external_estim...
File net/nqe/external_estimate_provider.h (right):

https://codereview.chromium.org/2010003002/diff/140001/net/nqe/external_estim...
net/nqe/external_estimate_provider.h:22: // Will be called with updated RTT,
downstream throughput (in kilobits per
nit: downstream throughput (in kilobits per second) and upstream throughput (in
kilobits per second) --> downstream and upstream throughput (both in kilobits
per second)

[tbansal1, 2016-05-27 23:15:32]

Patchset #2 (id:160001) has been deleted

[tbansal1, 2016-05-27 23:39:15]

bengr: ptal. Thanks.

https://codereview.chromium.org/2010003002/diff/140001/chrome/browser/android...
File chrome/browser/android/net/external_estimate_provider_android.cc (right):

https://codereview.chromium.org/2010003002/diff/140001/chrome/browser/android...
chrome/browser/android/net/external_estimate_provider_android.cc:132: if
(!GetRTT(&rtt))
On 2016/05/27 19:58:42, bengr wrote:
> Shouldn't GetRTT not modify &rtt if it returns false?

Done.

https://codereview.chromium.org/2010003002/diff/140001/chrome/browser/android...
File chrome/browser/android/net/external_estimate_provider_android_unittest.cc
(left):

https://codereview.chromium.org/2010003002/diff/140001/chrome/browser/android...
chrome/browser/android/net/external_estimate_provider_android_unittest.cc:103:
// Updated once during NetworkQualityEstimator constructor and again,
On 2016/05/27 19:58:42, bengr wrote:
> Why is this no longer needed? (lines 94-118)

The previous logic was complicated. It was querying external estimate provider
(EEP), and then EEP was calling back NQE, and then NQE was calling EEP again to
get RTT and bandwidth estimate.

That logic is simplified now: NQE calls EEP, and EEP callbacks NQE, and in the
callback it provides all the different estimates. So, some of the enums that
were tracking this complicated stuff are no longer required.

https://codereview.chromium.org/2010003002/diff/140001/net/nqe/external_estim...
File net/nqe/external_estimate_provider.h (right):

https://codereview.chromium.org/2010003002/diff/140001/net/nqe/external_estim...
net/nqe/external_estimate_provider.h:22: // Will be called with updated RTT,
downstream throughput (in kilobits per
On 2016/05/27 19:58:42, bengr wrote:
> nit: downstream throughput (in kilobits per second) and upstream throughput
(in
> kilobits per second) --> downstream and upstream throughput (both in kilobits
> per second)

Done.

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

Patchset #2 (id:180001) has been deleted

[bengr, 2016-05-31 17:32:54]

lgtm

[tbansal1, 2016-05-31 19:57:39]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-05-31 19:57:39]

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/2010003002/#ps220001
(title: "Rebased")

[commit-bot: I haz the power, 2016-05-31 19:58:36]

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

[commit-bot: I haz the power, 2016-05-31 21:38:18]

Description was changed from

==========
Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get 
called multiple times per connection change. This CL ensures 
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the logic easier to
understand.

BUG=604417
==========

to

==========
Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get 
called multiple times per connection change. This CL ensures 
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the logic easier to
understand.

BUG=604417
==========

[commit-bot: I haz the power, 2016-05-31 21:38:20]

Committed patchset #3 (id:220001)

[commit-bot: I haz the power, 2016-05-31 21:40:42]

Description was changed from

==========
Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get 
called multiple times per connection change. This CL ensures 
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the logic easier to
understand.

BUG=604417
==========

to

==========
Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get
called multiple times per connection change. This CL ensures
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the logic easier to
understand.

BUG=604417

Committed: https://crrev.com/bb48cb54901af25602c71fed388756a76e8499c6
Cr-Commit-Position: refs/heads/master@{#396938}
==========

[commit-bot: I haz the power, 2016-05-31 21:40:43]

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