NQE: Cleanup observation buffer code

NQE: Cleanup observation buffer code

Rename network_quality_observation_unittest.cc to
observation_buffer_unittest.cc, and add a couple of more
tests.

BUG=649887
Committed: https://crrev.com/d10c267c2912344db828ffb3927024a5b83f3643
Cr-Commit-Position: refs/heads/master@{#422280}

[tbansal1, 2016-09-23 23:40:48]

Description was changed from

==========
ps

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

to

==========
observation buffer code cleanup

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

[tbansal1, 2016-09-23 23:42:41]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-09-24 00:06:43]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2016-09-24 00:09:01]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2016-09-24 00:24:20]

Description was changed from

==========
observation buffer code cleanup

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

to

==========
Observation buffer code cleanup

Rename network_quality_observation_unittest.cc to 
observation_buffer_unittest.cc, and add a couple of more 
tests.

BUG=649887
==========

[tbansal1, 2016-09-24 00:24:35]

Description was changed from

==========
Observation buffer code cleanup

Rename network_quality_observation_unittest.cc to 
observation_buffer_unittest.cc, and add a couple of more 
tests.

BUG=649887
==========

to

==========
NQE: Cleanup observation buffer code

Rename network_quality_observation_unittest.cc to 
observation_buffer_unittest.cc, and add a couple of more 
tests.

BUG=649887
==========

[tbansal1, 2016-09-24 00:24:57]

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

[tbansal1, 2016-09-24 00:24:58]

ryansturm: ptal. thanks.

[RyanSturm, 2016-09-26 18:44:56]

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
File net/nqe/observation_buffer.h (right):

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer.h:16: #include "base/time/default_tick_clock.h"
This pulls in base/time/tick_clock.h, but you should probably include it as well
due to the unique_ptr declared below.

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer.h:19: #include
"net/nqe/network_quality_observation.h"
Is network_quality_observation.h needed?

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer.h:77: ValueType* result,
nit: Would you mind moving result to be the last parameter if it is an out
param?

Only make this change if you fix all the out-params in this file.

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer.h:122: void
SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock) {
nit: Maybe add tick_clock as a parameter to the constructor. I prefer to avoid
ForTesting methods when possible.

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
File net/nqe/observation_buffer_unittest.cc (right):

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer_unittest.cc:16: #include
"net/nqe/network_quality_observation_source.h"
#include "net/nqe/network_quality_observation.h"

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer_unittest.cc:23: namespace internal {
nit: Why did you move to internal? Is that really providing any benefit?

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer_unittest.cc:40: // Verify that the percentiles are
correctly computed when different
"correctly" seems to imply that the values are decreasing at the correct rate.
It looks like you are checking that the values are decreasing, but not
necessarily at the right rate. Since you know this is geometric summing could
you consider adding a a case with rate .5 that checks that the most recent item
is worth more than half, and another case with rate .75 or 1 where the most
recent item is worth less than half.

If this is out of scope, just change the comment to be more accurate: "Verify
that the percentiles are monotonically non-decreasing when a weight is applied"

I also haven't looked at other tests in this file that might already check this
functionality.

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer_unittest.cc:57:
tick_clock_ptr->Advance(base::TimeDelta::FromSeconds(100));
Can you call Advance in the for loop above and instead of now +
base::TimeDelta::FromSeconds(i) call NowTicks() every iteration?

[tbansal1, 2016-09-28 01:50:39]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 01:51:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 02:02:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 02:02:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbansal1, 2016-09-28 16:35:01]

Patchset #2 (id:80001) has been deleted

[tbansal1, 2016-09-28 16:41:49]

Patchset #2 (id:100001) has been deleted

[tbansal1, 2016-09-28 16:42:32]

ryansturm: ptal. thanks.

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
File net/nqe/observation_buffer.h (right):

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer.h:16: #include "base/time/default_tick_clock.h"
On 2016/09/26 18:44:55, Ryan Sturm wrote:
> This pulls in base/time/tick_clock.h, but you should probably include it as
well
> due to the unique_ptr declared below.

Done.

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer.h:19: #include
"net/nqe/network_quality_observation.h"
On 2016/09/26 18:44:55, Ryan Sturm wrote:
> Is network_quality_observation.h needed?

Yes. See Line 174:
std::deque<Observation<ValueType>> observations_;

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer.h:77: ValueType* result,
On 2016/09/26 18:44:55, Ryan Sturm wrote:
> nit: Would you mind moving result to be the last parameter if it is an out
> param?
> 
> Only make this change if you fix all the out-params in this file.

Added TODO.

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer.h:122: void
SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock) {
On 2016/09/26 18:44:55, Ryan Sturm wrote:
> nit: Maybe add tick_clock as a parameter to the constructor. I prefer to avoid
> ForTesting methods when possible.

Seems like net tends to use ForTesting() rather than passing it in constructor:
https://cs.chromium.org/search/?q=f:net+clock.*testing+-f:nqe&sq=package:chro...

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
File net/nqe/observation_buffer_unittest.cc (right):

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer_unittest.cc:16: #include
"net/nqe/network_quality_observation_source.h"
On 2016/09/26 18:44:55, Ryan Sturm wrote:
> #include "net/nqe/network_quality_observation.h"

Done.

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer_unittest.cc:23: namespace internal {
On 2016/09/26 18:44:55, Ryan Sturm wrote:
> nit: Why did you move to internal? Is that really providing any benefit?

It avoids the need to prefix internal:: to some of the classes and data
structures, and makes code less verbose.

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer_unittest.cc:40: // Verify that the percentiles are
correctly computed when different
On 2016/09/26 18:44:55, Ryan Sturm wrote:
> "correctly" seems to imply that the values are decreasing at the correct rate.
> It looks like you are checking that the values are decreasing, but not
> necessarily at the right rate. Since you know this is geometric summing could
> you consider adding a a case with rate .5 that checks that the most recent
item
> is worth more than half, and another case with rate .75 or 1 where the most
> recent item is worth less than half.
> 
> If this is out of scope, just change the comment to be more accurate: "Verify
> that the percentiles are monotonically non-decreasing when a weight is
applied"
> 
> I also haven't looked at other tests in this file that might already check
this
> functionality.

Done.

https://codereview.chromium.org/2367143002/diff/60001/net/nqe/observation_buf...
net/nqe/observation_buffer_unittest.cc:57:
tick_clock_ptr->Advance(base::TimeDelta::FromSeconds(100));
On 2016/09/26 18:44:55, Ryan Sturm wrote:
> Can you call Advance in the for loop above and instead of now +
> base::TimeDelta::FromSeconds(i) call NowTicks() every iteration?

Done.

[tbansal1, 2016-09-28 16:42:38]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 16:43:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-09-28 17:14:23]

lgtm. You could add more testing for testing more exactly the weighted
exponential behavior, but this should be sufficient as is.

[tbansal1, 2016-09-28 17:21:15]

On 2016/09/28 17:14:23, Ryan Sturm wrote:
> lgtm. You could add more testing for testing more exactly the weighted
> exponential behavior, but this should be sufficient as is.

Test PercentileDifferentTimestamps also covers part of this. It
adds 50 observations with very recent timestamp, and another
50 observations with very old timestamp. Then,   the test verifies that median
values are closer to the values in the recent observations (and
older observations have less weight).

[tbansal1, 2016-09-28 17:21:34]

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

[tbansal1, 2016-09-28 17:21:34]

bengr: ptal. Thanks.

[commit-bot: I haz the power, 2016-09-28 17:47:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 17:47:25]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[bengr, 2016-09-30 18:17:28]

lgtm

[tbansal1, 2016-09-30 19:42:02]

Patchset #3 (id:140001) has been deleted

[tbansal1, 2016-09-30 19:42:32]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 19:43:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-09-30 19:58:51]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 19:59:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-09-30 19:59:20]

Patchset #3 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-09-30 21:14:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 21:14:48]

Dry run: Exceeded global retry quota

[tbansal1, 2016-09-30 21:59:12]

Patchset #3 (id:180001) has been deleted

[tbansal1, 2016-09-30 21:59:37]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 22:00:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 22:50:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 22:50:42]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-09-30 22:54:01]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-09-30 22:54:01]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2367143002/#ps200001
(title: "ps")

[commit-bot: I haz the power, 2016-09-30 22:54:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 02:09:24]

Description was changed from

==========
NQE: Cleanup observation buffer code

Rename network_quality_observation_unittest.cc to 
observation_buffer_unittest.cc, and add a couple of more 
tests.

BUG=649887
==========

to

==========
NQE: Cleanup observation buffer code

Rename network_quality_observation_unittest.cc to 
observation_buffer_unittest.cc, and add a couple of more 
tests.

BUG=649887
==========

[commit-bot: I haz the power, 2016-10-01 02:09:26]

Committed patchset #3 (id:200001)

[commit-bot: I haz the power, 2016-10-01 02:11:31]

Description was changed from

==========
NQE: Cleanup observation buffer code

Rename network_quality_observation_unittest.cc to 
observation_buffer_unittest.cc, and add a couple of more 
tests.

BUG=649887
==========

to

==========
NQE: Cleanup observation buffer code

Rename network_quality_observation_unittest.cc to
observation_buffer_unittest.cc, and add a couple of more
tests.

BUG=649887

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

[commit-bot: I haz the power, 2016-10-01 02:11:32]

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