Split NQE to multiple files

Split NQE to multiple files

Split NetworkQualityEstimator to multiple files within
//net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

This CL only refactors the code. There is no functionality
change, although some new unittests have been added in
network_quality_observation_unittest.cc.

BUG=604091
Committed: https://crrev.com/b67539d93d0f6ec45311907e2cdf36e827913ec6
Cr-Commit-Position: refs/heads/master@{#393859}

[tbansal1, 2016-05-02 21:32:07]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-05-02 22:20:19]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2016-05-02 22:21:51]

Description was changed from

==========
w


w


w


nw

w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

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

to

==========
Split NQE to multiple files

Split NQE to multiple files within //net/nqe.

A new namespace has been added net::nqe::internal which
contains functions/classes internal to NQE.

BUG=604091
==========

[tbansal1, 2016-05-02 22:22:14]

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

[tbansal1, 2016-05-02 22:22:14]

bengr: PTAL. Thanks.

[tbansal1, 2016-05-02 22:23:34]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2016-05-02 22:25:07]

bengr: PTAL. Thanks.

This CL only refactors the code. There is no functionality change.

[tbansal1, 2016-05-02 22:25:36]

Description was changed from

==========
Split NQE to multiple files

Split NQE to multiple files within //net/nqe.

A new namespace has been added net::nqe::internal which
contains functions/classes internal to NQE.

BUG=604091
==========

to

==========
Split NQE to multiple files

Split NQE to multiple files within //net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

BUG=604091
==========

[tbansal1, 2016-05-02 22:26:42]

Description was changed from

==========
Split NQE to multiple files

Split NQE to multiple files within //net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

BUG=604091
==========

to

==========
Split NQE to multiple files

Split NQE to multiple files within //net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

This CL only refactors the code. There is no functionality 
change, although some new unittests have been added.

BUG=604091
==========

[tbansal1, 2016-05-03 00:14:05]

Description was changed from

==========
Split NQE to multiple files

Split NQE to multiple files within //net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

This CL only refactors the code. There is no functionality 
change, although some new unittests have been added.

BUG=604091
==========

to

==========
Split NQE to multiple files

Split NetworkQualityEstimator to multiple files within 
//net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

This CL only refactors the code. There is no functionality 
change, although some new unittests have been added.

BUG=604091
==========

[bengr, 2016-05-03 21:59:45]

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

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality.h:30: const int32_t kInvalidThroughput = 0;
Why doesn't have a corresponding method? E.g., InvalidThroughput(). Why is the
return an int32_t instead of an int64_t? Why is 0 invalid? Can't throughput be
0?

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality.h:40: int32_t downstream_throughput_kbps);
You use int32_t and int64_t in different parts of the code. Can we move
everything to one or the other?

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality.h:47: const base::TimeDelta& rtt() const { return rtt_;
}
Which RTT is this?

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality.h:56: // Estimated round trip time.
Again, which RTT?

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

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_observation.h:80: class NET_EXPORT ObservationBuffer {
Why not move each of these structs/class to a separate file?

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

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_observation_source.h:10: // On Android, a Java
counterpart will be generated for this enum.
Can the generator handle blank lines? If so I'd add one between each pair of
values.

[tbansal1, 2016-05-04 22:07:23]

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

[tbansal1, 2016-05-04 22:07:24]

bengr: PTAL at net/nqe/*
mmenke: PTAL at  net/net.gypi

Thanks.

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

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality.h:30: const int32_t kInvalidThroughput = 0;
On 2016/05/03 21:59:45, bengr wrote:
> Why doesn't have a corresponding method? E.g., InvalidThroughput(). 
Method was required for RTT because there can't be static fields in classes that
have non-basic data types. It is okay to have static int32 object but not static
TimeDelta object.
> Why is the
> return an int32_t instead of an int64_t?
Because throughput is int32 field.
> Why is 0 invalid? Can't throughput be
> 0?
0 is invalid because currently NQE takes a throughput sample only when there is
some network activity.

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality.h:40: int32_t downstream_throughput_kbps);
On 2016/05/03 21:59:45, bengr wrote:
> You use int32_t and int64_t in different parts of the code. Can we move
> everything to one or the other?

I do not see int64_t anywhere in this CL. 
It might be in the other throughput CL. If
that's what you are referring to, then throughput is int32_t  kbps
but the byte or bit count is int64_t.

The latter (byte or bit count) has to be int64_t. It would be
overkill to make throughput (in kbps) a 64 bit variable.

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality.h:47: const base::TimeDelta& rtt() const { return rtt_;
}
On 2016/05/03 21:59:45, bengr wrote:
> Which RTT is this?

Fixed. NetworkQuality structure needs to be expanded so it can hold on transport
layer RTT. I am planning to do that in a future CL.

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality.h:56: // Estimated round trip time.
On 2016/05/03 21:59:45, bengr wrote:
> Again, which RTT?

Done.

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

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_observation.h:80: class NET_EXPORT ObservationBuffer {
On 2016/05/03 21:59:45, bengr wrote:
> Why not move each of these structs/class to a separate file?

Done.

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

https://codereview.chromium.org/1942893002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_observation_source.h:10: // On Android, a Java
counterpart will be generated for this enum.
On 2016/05/03 21:59:45, bengr wrote:
> Can the generator handle blank lines? If so I'd add one between each pair of
> values.

Done.

[mmenke, 2016-05-05 14:50:56]

You need to update cronet and net's BUILD.gn files as well.  net.gypi is used
for both gn and gyp builds, so believe you're fine, there.

[tbansal1, 2016-05-05 17:58:18]

ptal. thanks.

[tbansal1, 2016-05-06 02:36:06]

Patchset #4 (id:120001) has been deleted

[mmenke, 2016-05-06 15:04:04]

https://codereview.chromium.org/1942893002/diff/140001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1942893002/diff/140001/components/cronet/andr...
components/cronet/android/BUILD.gn:299: "//third_party/jsr-305:jsr_305_javalib",
Should network_quality_observation_source_java be a dependency here?  It is in
the gyp file, or is it not needed there?  Or is there a reason it's only needed
for gyp?

[tbansal1, 2016-05-06 17:32:55]

mmneke: PTAL. Thanks.

https://codereview.chromium.org/1942893002/diff/140001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1942893002/diff/140001/components/cronet/andr...
components/cronet/android/BUILD.gn:299: "//third_party/jsr-305:jsr_305_javalib",
On 2016/05/06 15:04:04, mmenke wrote:
> Should network_quality_observation_source_java be a dependency here?  It is in
> the gyp file, or is it not needed there?  Or is there a reason it's only
needed
> for gyp?

Very likely it was not necessary in gyp file (Similar to http_cache_type_java).
I am not 100% sure because I did not try building it locally after commenting
that out.

Since gyp has been deprecated for Android, I am not sure if it is worth fixing
it.
https://groups.google.com/a/google.com/forum/#!searchin/cronet-team/gyp|sort:...

[mmenke, 2016-05-06 18:11:28]

Ok, LGTM

[tbansal1, 2016-05-06 18:57:56]

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

[tbansal1, 2016-05-06 18:57:57]

mef: PTAL at components/cronet*
Thanks.

[tbansal1, 2016-05-06 18:58:16]

Description was changed from

==========
Split NQE to multiple files

Split NetworkQualityEstimator to multiple files within 
//net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

This CL only refactors the code. There is no functionality 
change, although some new unittests have been added.

BUG=604091
==========

to

==========
Split NQE to multiple files

Split NetworkQualityEstimator to multiple files within 
//net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

This CL only refactors the code. There is no functionality 
change, although some new unittests have been added in
network_quality_observation_unittest.cc.

BUG=604091
==========

[mmenke, 2016-05-06 19:17:03]

On 2016/05/06 18:57:57, tbansal1 wrote:
> mef: PTAL at components/cronet*
> Thanks.

Think I'm still a cronet OWNER, though perhaps I shouldn't be.

[tbansal1, 2016-05-06 19:39:25]

tbansal@chromium.org changed reviewers:
- mef@chromium.org

[tbansal1, 2016-05-06 19:39:26]

yes, you are. Moving mef@ to CC.

[tbansal1, 2016-05-06 23:24:56]

Patchset #5 (id:160001) has been deleted

[bengr, 2016-05-13 23:36:16]

lgtm

[tbansal1, 2016-05-16 17:48:08]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-05-16 17:48:08]

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

[commit-bot: I haz the power, 2016-05-16 17:48:55]

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

[commit-bot: I haz the power, 2016-05-16 17:54:23]

Description was changed from

==========
Split NQE to multiple files

Split NetworkQualityEstimator to multiple files within 
//net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

This CL only refactors the code. There is no functionality 
change, although some new unittests have been added in
network_quality_observation_unittest.cc.

BUG=604091
==========

to

==========
Split NQE to multiple files

Split NetworkQualityEstimator to multiple files within 
//net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

This CL only refactors the code. There is no functionality 
change, although some new unittests have been added in
network_quality_observation_unittest.cc.

BUG=604091
==========

[commit-bot: I haz the power, 2016-05-16 17:54:24]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2016-05-16 17:56:39]

Description was changed from

==========
Split NQE to multiple files

Split NetworkQualityEstimator to multiple files within 
//net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

This CL only refactors the code. There is no functionality 
change, although some new unittests have been added in
network_quality_observation_unittest.cc.

BUG=604091
==========

to

==========
Split NQE to multiple files

Split NetworkQualityEstimator to multiple files within
//net/nqe.

A new namespace net::nqe::internal has been added which
contains functions/classes internal to NQE.

This CL only refactors the code. There is no functionality
change, although some new unittests have been added in
network_quality_observation_unittest.cc.

BUG=604091

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

[commit-bot: I haz the power, 2016-05-16 17:56:40]

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