Create component to expose network usage stats to consumers.

Create component to expose network usage stats to consumers.

Creates the new component "data_usage" that contains a
DataUseAggregator. A global DataUseAggregator is owned by the IOThread.
The ChromeNetworkDelegate reports data usage to the DataUseAggregator,
which in turn aggregates it and reports to any registered observers.

TBR=brettw@chromium.org
BUG=518051
Committed: https://crrev.com/ae932be8cc8c618ae9ffefe720879c5ed82c38a5
Cr-Commit-Position: refs/heads/master@{#353137}

[sclittle, 2015-09-29 21:59:47]

Patchset #1 (id:1) has been deleted

[sclittle, 2015-09-29 22:12:46]

Patchset #1 (id:20001) has been deleted

[sclittle, 2015-09-29 22:15:39]

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

[sclittle, 2015-09-29 22:15:39]

PTAL for sanity check. I'll add tests and clean it up in a later patch set.

[sdefresne, 2015-09-30 08:52:50]

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
File components/data_usage/core/DEPS (right):

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/DEPS:2: "+base",
nit, drive-by: every component can depends on base, so you can remove this line.

[tbansal1, 2015-10-01 20:30:43]

https://codereview.chromium.org/1373373002/diff/40001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1373373002/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:582:
chrome_network_delegate->set_data_use_aggregator(
May be move this after tracking_profile4 (right now it is between
tracking_profile3 and tracking_profile4)

https://codereview.chromium.org/1373373002/diff/40001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1373373002/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:505: if (data_use_aggregator_) {
May be reverse the boolean condition to reduce indentation
if(!...)
  return;

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.cc:38: auto observer_it =
observers_.find(observer);
Using ObserverList  would be simpler and is probably the recommended way.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.cc:39: DCHECK(observer_it !=
observers_.end())
This DCHECK might be compiled out. If iterator is invalid, we should return.
Behavior of std::vector::erase() is undefined if the iterator is invalid (if I
am reading the documentation correctly :P).

http://www.cplusplus.com/reference/vector/vector/erase/

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.h (right):

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:23: struct DataUseDelta {
Might be easier to keep this in a separate CL, with this CL only containing the
plumbing?

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:23: struct DataUseDelta {
If we need vector of these structs, then may be these should be timestamped?

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:26: GURL url;
should these variables be all const?

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:43: virtual ~Observer();
destructor could be private.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:49: static
scoped_ptr<DataUseAggregator> Create();
I guess it would be okay for callers to directly call the constructor. It is
obvious that caller has the ownership of the created objected. wdyt?

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:78: std::set<Observer*>
observers_;
Why not ObserverList or ObserverListThreadSafe?

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:82: 
thread checker is missing.

[bengr, 2015-10-02 19:32:22]

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/O...
File components/data_usage/OWNERS (right):

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/O...
components/data_usage/OWNERS:3: tbansal@chromium.org
tbansal should be an owner, but he should be a committer first.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.cc:50: data_use_delta.url = url;
DataUseDelta could have a constructor for all of these. Would be less error
prone in that you wouldn't forget any.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.cc:60:
buffered_data_use_deltas_.push_back(data_use_delta);
Is there any limit to how much this class will buffer?

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.h (right):

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:23: struct DataUseDelta {
I'd be inclined to call this DataUse.

[sclittle, 2015-10-03 03:23:41]

Patchset #2 (id:60001) has been deleted

[sclittle, 2015-10-03 03:27:53]

https://codereview.chromium.org/1373373002/diff/40001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1373373002/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:582:
chrome_network_delegate->set_data_use_aggregator(
On 2015/10/01 20:30:43, tbansal1 wrote:
> May be move this after tracking_profile4 (right now it is between
> tracking_profile3 and tracking_profile4)

It's part of the setup for ChromeNetworkDelegate, so it looks like it has more
to do with tracking_profile3 ("InitAsync::ChromeNetworkDelegate") than
tracking_profile4 ("InitAsync::CreateGlobalHostResolver").

https://codereview.chromium.org/1373373002/diff/40001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1373373002/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:505: if (data_use_aggregator_) {
On 2015/10/01 20:30:43, tbansal1 wrote:
> May be reverse the boolean condition to reduce indentation
> if(!...)
>   return;

I'd prefer to avoid adding returns like that, since that would make it harder to
add new stuff to the end of OnNetworkBytesReceived.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/O...
File components/data_usage/OWNERS (right):

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/O...
components/data_usage/OWNERS:3: tbansal@chromium.org
On 2015/10/02 19:32:21, bengr wrote:
> tbansal should be an owner, but he should be a committer first.

OK, removed for now.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
File components/data_usage/core/DEPS (right):

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/DEPS:2: "+base",
On 2015/09/30 08:52:50, sdefresne wrote:
> nit, drive-by: every component can depends on base, so you can remove this
line.

Done.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.cc:38: auto observer_it =
observers_.find(observer);
On 2015/10/01 20:30:43, tbansal1 wrote:
> Using ObserverList  would be simpler and is probably the recommended way.

Done.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.cc:39: DCHECK(observer_it !=
observers_.end())
On 2015/10/01 20:30:43, tbansal1 wrote:
> This DCHECK might be compiled out. If iterator is invalid, we should return.
> Behavior of std::vector::erase() is undefined if the iterator is invalid (if I
> am reading the documentation correctly :P).
> 
> http://www.cplusplus.com/reference/vector/vector/erase/

Removed this, since the ObserverList already handles this.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.cc:50: data_use_delta.url = url;
On 2015/10/02 19:32:22, bengr wrote:
> DataUseDelta could have a constructor for all of these. Would be less error
> prone in that you wouldn't forget any.

Done.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.cc:60:
buffered_data_use_deltas_.push_back(data_use_delta);
On 2015/10/02 19:32:22, bengr wrote:
> Is there any limit to how much this class will buffer?

Right now, it doesn't buffer at all. We can impose a limit on how big the buffer
can get once buffering is implemented.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.h (right):

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:23: struct DataUseDelta {
On 2015/10/01 20:30:43, tbansal1 wrote:
> If we need vector of these structs, then may be these should be timestamped?

Done.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:23: struct DataUseDelta {
On 2015/10/02 19:32:22, bengr wrote:
> I'd be inclined to call this DataUse.

Renamed to DataUse.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:23: struct DataUseDelta {
On 2015/10/01 20:30:43, tbansal1 wrote:
> Might be easier to keep this in a separate CL, with this CL only containing
the
> plumbing?

This struct is part of the plumbing between DataUseAggregator and its observers.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:26: GURL url;
On 2015/10/01 20:30:43, tbansal1 wrote:
> should these variables be all const?

They can't be, since that would disable copy-assignment, and copy assignment is
needed to put them in a std::vector.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:43: virtual ~Observer();
On 2015/10/01 20:30:43, tbansal1 wrote:
> destructor could be private.

I'd prefer not to make it private, since that would make it impossible to have a
scoped_ptr<Observer>.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:49: static
scoped_ptr<DataUseAggregator> Create();
On 2015/10/01 20:30:43, tbansal1 wrote:
> I guess it would be okay for callers to directly call the constructor. It is
> obvious that caller has the ownership of the created objected. wdyt?

Sure, I'll switch it to using a factory-style Create function later if we need
to.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:78: std::set<Observer*>
observers_;
On 2015/10/01 20:30:43, tbansal1 wrote:
> Why not ObserverList or ObserverListThreadSafe?

Done.

https://codereview.chromium.org/1373373002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:82: 
On 2015/10/01 20:30:43, tbansal1 wrote:
> thread checker is missing.

Done.

[sclittle, 2015-10-03 03:28:51]

sclittle@chromium.org changed reviewers:
+ blundell@chromium.org, brettw@chromium.org, mmenke@chromium.org

[sclittle, 2015-10-03 03:28:52]

bengr, tbansal: *
mmenke: chrome/browser/*
blundell: components/*
brettw: '+url' in DEPS

Thanks in advance!

[blundell, 2015-10-05 07:39:44]

On 2015/10/03 03:28:52, sclittle wrote:
> bengr, tbansal: *
> mmenke: chrome/browser/*
> blundell: components/*
> brettw: '+url' in DEPS
> 
> Thanks in advance!

Is the intent for the data_usage component to be a layered component in the
future, i.e., have a content/ subdirectory? If not, there's no need for the
core/ subdirectory.

Looks good other than that question for top-level //components.

[mmenke, 2015-10-05 15:01:34]

On 2015/10/05 07:39:44, blundell wrote:
> On 2015/10/03 03:28:52, sclittle wrote:
> > bengr, tbansal: *
> > mmenke: chrome/browser/*
> > blundell: components/*
> > brettw: '+url' in DEPS
> > 
> > Thanks in advance!
> 
> Is the intent for the data_usage component to be a layered component in the
> future, i.e., have a content/ subdirectory? If not, there's no need for the
> core/ subdirectory.
> 
> Looks good other than that question for top-level //components.

Do we really want data_use_measurement and data_usage to be two separate
components?  Seems weird to have ChromeNetworkDelegate explicitly hook into two
different data use services.  Admittedly, the other one just looks at aggregate
usage, rather than usage over time, but still seems a bit silly.

[mmenke, 2015-10-05 15:43:16]

On 2015/10/05 15:01:34, mmenke wrote:
> On 2015/10/05 07:39:44, blundell wrote:
> > On 2015/10/03 03:28:52, sclittle wrote:
> > > bengr, tbansal: *
> > > mmenke: chrome/browser/*
> > > blundell: components/*
> > > brettw: '+url' in DEPS
> > > 
> > > Thanks in advance!
> > 
> > Is the intent for the data_usage component to be a layered component in the
> > future, i.e., have a content/ subdirectory? If not, there's no need for the
> > core/ subdirectory.
> > 
> > Looks good other than that question for top-level //components.
> 
> Do we really want data_use_measurement and data_usage to be two separate
> components?  Seems weird to have ChromeNetworkDelegate explicitly hook into
two
> different data use services.  Admittedly, the other one just looks at
aggregate
> usage, rather than usage over time, but still seems a bit silly.

One other option here is to make one component an observer of the other,
depending on whether you plan to pass URLRequest pointers around.

[sclittle, 2015-10-05 21:46:04]

On 2015/10/05 15:43:16, mmenke wrote:
> On 2015/10/05 15:01:34, mmenke wrote:
> > 
> > Do we really want data_use_measurement and data_usage to be two separate
> > components?  Seems weird to have ChromeNetworkDelegate explicitly hook into
> two
> > different data use services.  Admittedly, the other one just looks at
> aggregate
> > usage, rather than usage over time, but still seems a bit silly.
> 
> One other option here is to make one component an observer of the other,
> depending on whether you plan to pass URLRequest pointers around.

We don't want to hold onto URLRequest pointers, since then we can't buffer them
for aggregation/amortization and send notifications later.

The two components actually care about different things, since the
data_use_measurement component wants to measure message sizes sent/received by
different Chrome services, which means getting request/response sizes at
OnCompleted() instead of at OnNetworkBytesSent/Received().

Perhaps we could rename the existing data_use_measurement component to something
else in order to clarify this? Maybe something like chrome_service_data_use.

WDYT?

[sclittle, 2015-10-05 21:51:28]

On 2015/10/05 07:39:44, blundell wrote:
> On 2015/10/03 03:28:52, sclittle wrote:
> > bengr, tbansal: *
> > mmenke: chrome/browser/*
> > blundell: components/*
> > brettw: '+url' in DEPS
> > 
> > Thanks in advance!
> 
> Is the intent for the data_usage component to be a layered component in the
> future, i.e., have a content/ subdirectory? If not, there's no need for the
> core/ subdirectory.
> 
> Looks good other than that question for top-level //components.

We'll probably want an android/ subdirectory in the future, for android-specific
amortization implementation, and maybe also a content/ subdirectory too.

[mmenke, 2015-10-05 22:17:46]

On 2015/10/05 21:46:04, sclittle wrote:
> On 2015/10/05 15:43:16, mmenke wrote:
> > On 2015/10/05 15:01:34, mmenke wrote:
> > > 
> > > Do we really want data_use_measurement and data_usage to be two separate
> > > components?  Seems weird to have ChromeNetworkDelegate explicitly hook
into
> > two
> > > different data use services.  Admittedly, the other one just looks at
> > aggregate
> > > usage, rather than usage over time, but still seems a bit silly.
> > 
> > One other option here is to make one component an observer of the other,
> > depending on whether you plan to pass URLRequest pointers around.
> 
> We don't want to hold onto URLRequest pointers, since then we can't buffer
them
> for aggregation/amortization and send notifications later.
> 
> The two components actually care about different things, since the
> data_use_measurement component wants to measure message sizes sent/received by
> different Chrome services, which means getting request/response sizes at
> OnCompleted() instead of at OnNetworkBytesSent/Received().
> 
> Perhaps we could rename the existing data_use_measurement component to
something
> else in order to clarify this? Maybe something like chrome_service_data_use.
> 
> WDYT?

data_use_measurement could be used on every sent/received event.  Suppose it
does do a fair amount of appending of strings, which may make that problematic,
from a performance standpoint, though.  ChromeNetworkDelegate is slowly becoming
more and more of a mess, sadly.

I think the rename sounds reasonable.  Or maybe something like
service_network_use_metrics?

What exactly is the purpose of this class?  Who's going to want to monitor every
read/write?  Why do we need both these hooks and hook in the NQE directly?

[tbansal1, 2015-10-05 22:19:00]

On 2015/10/05 22:17:46, mmenke wrote:
> On 2015/10/05 21:46:04, sclittle wrote:
> > On 2015/10/05 15:43:16, mmenke wrote:
> > > On 2015/10/05 15:01:34, mmenke wrote:
> > > > 
> > > > Do we really want data_use_measurement and data_usage to be two separate
> > > > components?  Seems weird to have ChromeNetworkDelegate explicitly hook
> into
> > > two
> > > > different data use services.  Admittedly, the other one just looks at
> > > aggregate
> > > > usage, rather than usage over time, but still seems a bit silly.
> > > 
> > > One other option here is to make one component an observer of the other,
> > > depending on whether you plan to pass URLRequest pointers around.
> > 
> > We don't want to hold onto URLRequest pointers, since then we can't buffer
> them
> > for aggregation/amortization and send notifications later.
> > 
> > The two components actually care about different things, since the
> > data_use_measurement component wants to measure message sizes sent/received
by
> > different Chrome services, which means getting request/response sizes at
> > OnCompleted() instead of at OnNetworkBytesSent/Received().
> > 
> > Perhaps we could rename the existing data_use_measurement component to
> something
> > else in order to clarify this? Maybe something like chrome_service_data_use.
> > 
> > WDYT?
> 
> data_use_measurement could be used on every sent/received event.  Suppose it
> does do a fair amount of appending of strings, which may make that
problematic,
> from a performance standpoint, though.  ChromeNetworkDelegate is slowly
becoming
> more and more of a mess, sadly.
> 
> I think the rename sounds reasonable.  Or maybe something like
> service_network_use_metrics?
> 
> What exactly is the purpose of this class?  Who's going to want to monitor
every
> read/write?  Why do we need both these hooks and hook in the NQE directly?

NQE is only notified at the beginning/end of request. Not every read/write.

[mmenke, 2015-10-05 23:22:14]

On 2015/10/05 22:19:00, tbansal1 wrote:
> On 2015/10/05 22:17:46, mmenke wrote:
> > On 2015/10/05 21:46:04, sclittle wrote:
> > > On 2015/10/05 15:43:16, mmenke wrote:
> > > > On 2015/10/05 15:01:34, mmenke wrote:
> > > > > 
> > > > > Do we really want data_use_measurement and data_usage to be two
separate
> > > > > components?  Seems weird to have ChromeNetworkDelegate explicitly hook
> > into
> > > > two
> > > > > different data use services.  Admittedly, the other one just looks at
> > > > aggregate
> > > > > usage, rather than usage over time, but still seems a bit silly.
> > > > 
> > > > One other option here is to make one component an observer of the other,
> > > > depending on whether you plan to pass URLRequest pointers around.
> > > 
> > > We don't want to hold onto URLRequest pointers, since then we can't buffer
> > them
> > > for aggregation/amortization and send notifications later.
> > > 
> > > The two components actually care about different things, since the
> > > data_use_measurement component wants to measure message sizes
sent/received
> by
> > > different Chrome services, which means getting request/response sizes at
> > > OnCompleted() instead of at OnNetworkBytesSent/Received().
> > > 
> > > Perhaps we could rename the existing data_use_measurement component to
> > something
> > > else in order to clarify this? Maybe something like
chrome_service_data_use.
> > > 
> > > WDYT?
> > 
> > data_use_measurement could be used on every sent/received event.  Suppose it
> > does do a fair amount of appending of strings, which may make that
> problematic,
> > from a performance standpoint, though.  ChromeNetworkDelegate is slowly
> becoming
> > more and more of a mess, sadly.
> > 
> > I think the rename sounds reasonable.  Or maybe something like
> > service_network_use_metrics?
> > 
> > What exactly is the purpose of this class?  Who's going to want to monitor
> every
> > read/write?  Why do we need both these hooks and hook in the NQE directly?
> 
> NQE is only notified at the beginning/end of request. Not every read/write.

Right...And it produces network quality estimates.  It seems like the only
potential use for this API would be....to create network quality estimates.  So
I'm confused.

[sclittle, 2015-10-06 01:17:00]

On 2015/10/05 23:22:14, mmenke wrote:
> On 2015/10/05 22:19:00, tbansal1 wrote:
> > On 2015/10/05 22:17:46, mmenke wrote:
> > > 
> > > data_use_measurement could be used on every sent/received event.  Suppose
it
> > > does do a fair amount of appending of strings, which may make that
> > problematic,
> > > from a performance standpoint, though.  ChromeNetworkDelegate is slowly
> > becoming
> > > more and more of a mess, sadly.
> > > 
> > > I think the rename sounds reasonable.  Or maybe something like
> > > service_network_use_metrics?
> > > 
> > > What exactly is the purpose of this class?  Who's going to want to monitor
> > every
> > > read/write?  Why do we need both these hooks and hook in the NQE directly?
> > 
> > NQE is only notified at the beginning/end of request. Not every read/write.
> 
> Right...And it produces network quality estimates.  It seems like the only
> potential use for this API would be....to create network quality estimates. 
So
> I'm confused.

The data_usage::DataUseAggregator keeps track of individual reads/writes so that
it can amortize this data usage on supported platforms, e.g. using TrafficStats
on Android. Since TrafficStats gives overall byte counts for the Chrome app,
it's more accurate if we can account for partial in-progress responses. This
amortization isn't implemented yet, but it would involve buffering a bunch of
reads/writes then adjusting the byte counts according to the numbers from
TrafficStats, and then reporting the amortized data use to observers.

This doc has some more information:
https://docs.google.com/document/d/14KGVi5-4uuBhmxoCvp24QGmQiPSMWmYjeruDweIj0...

We could make the data_use_measurement component an observer of
data_usage::DataUseAggregator, but data_use_measurement would still need hooks
into OnCompleted() to report metrics on individual request/response message
sizes from services. It's tougher to get and keep track of the amortized data
usage for an entire URLRequest, since the request could take a long time and
multiple requests could be going on simultaneously, and the URLRequest might
have been destroyed by the time amortization is performed. We could implement
this later though.

[bengr, 2015-10-06 19:30:28]

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:584: true /* is_data_usage_off_the_record */);
Why the underscores?

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:505: if (data_use_aggregator_) {
How about:

if (!data_use_aggregator_)
  return;

...

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:518: if (data_use_aggregator_) {
Same here.

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate_unittest.cc:107:
++num_report_data_use_calls_;
Is this sufficient for testing? You could add the values to a vector and test
the vector size, among other things.

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate_unittest.cc:111:
++num_report_off_the_record_data_use_calls_;
Here too.

https://codereview.chromium.org/1373373002/diff/80001/components/data_usage/c...
File components/data_usage/core/data_use.h (right):

https://codereview.chromium.org/1373373002/diff/80001/components/data_usage/c...
components/data_usage/core/data_use.h:20: int tab_id,
Perhaps we should use a const std::string& tab_label instead, because we might
need to augment the tab annotation beyond an ID.

[tbansal1, 2015-10-06 20:13:15]

https://codereview.chromium.org/1373373002/diff/80001/components/data_usage/c...
File components/data_usage/core/data_use.h (right):

https://codereview.chromium.org/1373373002/diff/80001/components/data_usage/c...
components/data_usage/core/data_use.h:20: int tab_id,
On 2015/10/06 19:30:28, bengr wrote:
> Perhaps we should use a const std::string& tab_label instead, because we might
> need to augment the tab annotation beyond an ID.

There is a tab_id field in Chromium
(which is int32) which is unique across all tabs and is monotonically
increasing. 
We should keep that in the struct. Would tab annotation be maintained
by data_usage component? I was expecting it to be separate.

https://codereview.chromium.org/1373373002/diff/80001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.h (right):

https://codereview.chromium.org/1373373002/diff/80001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:8: #include <vector>
stdint.h include missing.

[sclittle, 2015-10-07 01:07:56]

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:584: true /* is_data_usage_off_the_record */);
On 2015/10/06 19:30:27, bengr wrote:
> Why the underscores?

Because that's what the argument name is, |is_data_usage_off_the_record|.

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:505: if (data_use_aggregator_) {
On 2015/10/06 19:30:27, bengr wrote:
> How about:
> 
> if (!data_use_aggregator_)
>   return;
> 
> ...
> 

We could, but that would make it harder to add new logic to the end of this
method in the future, and it would just have to be changed back then.

I don't mind changing this if you'd prefer the other way though.

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:518: if (data_use_aggregator_) {
On 2015/10/06 19:30:27, bengr wrote:
> Same here.

See above.

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate_unittest.cc:107:
++num_report_data_use_calls_;
On 2015/10/06 19:30:28, bengr wrote:
> Is this sufficient for testing? You could add the values to a vector and test
> the vector size, among other things.

Changed to keep track of the bytes. The main thing I'm trying to test here is
that on_the_record vs. off_the_record is respected.

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate_unittest.cc:111:
++num_report_off_the_record_data_use_calls_;
On 2015/10/06 19:30:28, bengr wrote:
> Here too.

Changed this to keep track of the bytes too.

https://codereview.chromium.org/1373373002/diff/80001/components/data_usage/c...
File components/data_usage/core/data_use.h (right):

https://codereview.chromium.org/1373373002/diff/80001/components/data_usage/c...
components/data_usage/core/data_use.h:20: int tab_id,
On 2015/10/06 20:13:15, tbansal1 wrote:
> On 2015/10/06 19:30:28, bengr wrote:
> > Perhaps we should use a const std::string& tab_label instead, because we
might
> > need to augment the tab annotation beyond an ID.
> 
> There is a tab_id field in Chromium
> (which is int32) which is unique across all tabs and is monotonically
> increasing. 
> We should keep that in the struct. Would tab annotation be maintained
> by data_usage component? I was expecting it to be separate.

Changed to int32_t according to tbansal's comment.

If we do need to handle tab annotations, then that annotation could either be a
separate field in this struct or just maintained by the observer that cares
about it.

https://codereview.chromium.org/1373373002/diff/80001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.h (right):

https://codereview.chromium.org/1373373002/diff/80001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:8: #include <vector>
On 2015/10/06 20:13:15, tbansal1 wrote:
> stdint.h include missing.

Done.

[mmenke, 2015-10-07 19:38:35]

On 2015/10/06 01:17:00, sclittle wrote:
> On 2015/10/05 23:22:14, mmenke wrote:
> > On 2015/10/05 22:19:00, tbansal1 wrote:
> > > On 2015/10/05 22:17:46, mmenke wrote:
> > > > 
> > > > data_use_measurement could be used on every sent/received event. 
Suppose
> it
> > > > does do a fair amount of appending of strings, which may make that
> > > problematic,
> > > > from a performance standpoint, though.  ChromeNetworkDelegate is slowly
> > > becoming
> > > > more and more of a mess, sadly.
> > > > 
> > > > I think the rename sounds reasonable.  Or maybe something like
> > > > service_network_use_metrics?
> > > > 
> > > > What exactly is the purpose of this class?  Who's going to want to
monitor
> > > every
> > > > read/write?  Why do we need both these hooks and hook in the NQE
directly?
> > > 
> > > NQE is only notified at the beginning/end of request. Not every
read/write.
> > 
> > Right...And it produces network quality estimates.  It seems like the only
> > potential use for this API would be....to create network quality estimates. 
> So
> > I'm confused.
> 
> The data_usage::DataUseAggregator keeps track of individual reads/writes so
that
> it can amortize this data usage on supported platforms, e.g. using
TrafficStats
> on Android. Since TrafficStats gives overall byte counts for the Chrome app,
> it's more accurate if we can account for partial in-progress responses. This
> amortization isn't implemented yet, but it would involve buffering a bunch of
> reads/writes then adjusting the byte counts according to the numbers from
> TrafficStats, and then reporting the amortized data use to observers.
> 
> This doc has some more information:
>
https://docs.google.com/document/d/14KGVi5-4uuBhmxoCvp24QGmQiPSMWmYjeruDweIj0...
> 
> We could make the data_use_measurement component an observer of
> data_usage::DataUseAggregator, but data_use_measurement would still need hooks
> into OnCompleted() to report metrics on individual request/response message
> sizes from services. It's tougher to get and keep track of the amortized data
> usage for an entire URLRequest, since the request could take a long time and
> multiple requests could be going on simultaneously, and the URLRequest might
> have been destroyed by the time amortization is performed. We could implement
> this later though.

LGTM, though I remain skeptical.

[bengr, 2015-10-07 20:54:26]

lgtm

[tbansal1, 2015-10-08 04:22:44]

lgtm

nits.

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:584: true /* is_data_usage_off_the_record */);
On 2015/10/07 01:07:55, sclittle wrote:
> On 2015/10/06 19:30:27, bengr wrote:
> > Why the underscores?
> 
> Because that's what the argument name is, |is_data_usage_off_the_record|.

nit, I think the right syntax is: /*is_data_usage_off_the_record=*/true
(at least I have been told so :P)

See this:
https://code.google.com/p/chromium/codesearch#search/&q=f:src/net%20=%5C*%5C/...

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
components/data_usage/core/data_use_aggregator.cc:47: // data usage calculations
if applicable.
DCHECK(thread_checker_.CalledOnValidThread());

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
File components/data_usage/core/data_use_aggregator.h (right):

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
components/data_usage/core/data_use_aggregator.h:31: virtual void
OnDataUse(const std::vector<DataUse>& data_use_sequence) = 0;
 DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
File components/data_usage/core/data_use_aggregator_unittest.cc (right):

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
components/data_usage/core/data_use_aggregator_unittest.cc:85:
TestObserver(DataUseAggregator* data_use_aggregator)
explicit

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
components/data_usage/core/data_use_aggregator_unittest.cc:229:
EXPECT_EQ(static_cast<size_t>(0), observed_data_use().size());
Just say 0U?

[blundell, 2015-10-08 07:17:30]

On 2015/10/05 21:51:28, sclittle wrote:
> On 2015/10/05 07:39:44, blundell wrote:
> > On 2015/10/03 03:28:52, sclittle wrote:
> > > bengr, tbansal: *
> > > mmenke: chrome/browser/*
> > > blundell: components/*
> > > brettw: '+url' in DEPS
> > > 
> > > Thanks in advance!
> > 
> > Is the intent for the data_usage component to be a layered component in the
> > future, i.e., have a content/ subdirectory? If not, there's no need for the
> > core/ subdirectory.
> > 
> > Looks good other than that question for top-level //components.
> 
> We'll probably want an android/ subdirectory in the future, for
android-specific
> amortization implementation, and maybe also a content/ subdirectory too.

lgtm

[sclittle, 2015-10-08 19:44:13]

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1373373002/diff/80001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:584: true /* is_data_usage_off_the_record */);
On 2015/10/08 04:22:44, tbansal1 wrote:
> On 2015/10/07 01:07:55, sclittle wrote:
> > On 2015/10/06 19:30:27, bengr wrote:
> > > Why the underscores?
> > 
> > Because that's what the argument name is, |is_data_usage_off_the_record|.
> 
> nit, I think the right syntax is: /*is_data_usage_off_the_record=*/true
> (at least I have been told so :P)
> 
> See this:
>
https://code.google.com/p/chromium/codesearch#search/&q=f:src/net%20=%5C*%5C/...

It looks like both styles are used, most of the cases I found are using the
"value /* arg_name */" format though:
https://code.google.com/p/chromium/codesearch#search/&q=lang:c%2B%2B%20%22/*%...

So I'll leave this as-is.

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
components/data_usage/core/data_use_aggregator.cc:47: // data usage calculations
if applicable.
On 2015/10/08 04:22:44, tbansal1 wrote:
> DCHECK(thread_checker_.CalledOnValidThread());

Done.

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
File components/data_usage/core/data_use_aggregator.h (right):

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
components/data_usage/core/data_use_aggregator.h:31: virtual void
OnDataUse(const std::vector<DataUse>& data_use_sequence) = 0;
On 2015/10/08 04:22:44, tbansal1 wrote:
>  DISALLOW_COPY_AND_ASSIGN?

The subclasses should DISALLOW_COPY_AND_ASSIGN themselves if appropriate, and
there's not much point in disallowing copy or assignment on a pure virtual class
like this, since it's impossible to construct a raw Observer anyways.

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
File components/data_usage/core/data_use_aggregator_unittest.cc (right):

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
components/data_usage/core/data_use_aggregator_unittest.cc:85:
TestObserver(DataUseAggregator* data_use_aggregator)
On 2015/10/08 04:22:44, tbansal1 wrote:
> explicit

Done.

https://codereview.chromium.org/1373373002/diff/100001/components/data_usage/...
components/data_usage/core/data_use_aggregator_unittest.cc:229:
EXPECT_EQ(static_cast<size_t>(0), observed_data_use().size());
On 2015/10/08 04:22:44, tbansal1 wrote:
> Just say 0U?

0U means unsigned int, but the exact size of size_t and unsigned int are
implementation defined, and they aren't necessarily the same. See
http://en.cppreference.com/w/cpp/types/size_t

So I'm keeping it as the static_cast for robustness here.

[sclittle, 2015-10-08 19:49:04]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-10-08 19:49:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
tbansal@chromium.org, bengr@chromium.org, mmenke@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1373373002/#ps120001
(title: "Addressed nits")

[commit-bot: I haz the power, 2015-10-08 19:51:01]

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

[commit-bot: I haz the power, 2015-10-08 20:53:59]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-10-08 20:54:52]

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