Notify NetworkDelegate when bytes have been received over the network.

Notify NetworkDelegate when bytes have been received over the network.

This will allow for network delegates to notify observers when network
usage occurs, for up-to-date accounting of data usage. See bug for more
details.

This change also removes the NetworkDelegate::OnRawBytesRead method,
replacing it with this new NetworkDelgate::OnNetworkBytesReceived
method.

BUG=518051
Committed: https://crrev.com/ce72c486271573ff5d636d19fedf7aca21df1b3a
Cr-Commit-Position: refs/heads/master@{#345163}

[sclittle, 2015-08-15 05:12:08]

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

[tbansal1, 2015-08-17 20:51:17]

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate.h
File net/base/network_delegate.h (right):

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate.h...
net/base/network_delegate.h:90: void NotifyNetworkBytesReceived(const
URLRequest& request, int64 bytes_read);
|bytes_read| or |bytes_received|? I believe you are using both.

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate.h...
net/base/network_delegate.h:206: // Called when bytes are received from the
network, such as after receiving
Would this also get triggered on requests that are served by localhost or by
cache?

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
net/url_request/url_request_job.cc:789: 
May be worth it to add a DCHECK:
DCHECK(!request_->was_cached()

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
net/url_request/url_request_test_util.cc:513: total_network_bytes_received_ +=
bytes_received;
nit: May be add the event to event_order_ for easier debugging.

[bengr, 2015-08-17 21:30:40]

https://codereview.chromium.org/1284993005/diff/1/net/base/layered_network_de...
File net/base/layered_network_delegate.h (right):

https://codereview.chromium.org/1284993005/diff/1/net/base/layered_network_de...
net/base/layered_network_delegate.h:67: int64 bytes_received) final;
Use int64_t and include <stdint.h>. See
https://code.google.com/p/chromium/codesearch#chromium/src/base/basictypes.h&...

https://codereview.chromium.org/1284993005/diff/1/net/base/layered_network_de...
net/base/layered_network_delegate.h:130: int64 bytes_received);
What does bytes_received mean? Does it include, e.g., SPDY framing, TLS
overhead, TCP retransmits, etc?

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate.h
File net/base/network_delegate.h (right):

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate.h...
net/base/network_delegate.h:90: void NotifyNetworkBytesReceived(const
URLRequest& request, int64 bytes_read);
On 2015/08/17 20:51:17, tbansal1 wrote:
> |bytes_read| or |bytes_received|? I believe you are using both.

FWIW, I like "received" and "sent".

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate.h...
net/base/network_delegate.h:208: // bytes measured at the application layer that
have been received over the
It's work some more comment space to be ultra specific about what this includes.
E.g., currently, TLS overhead is not included.

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate_i...
File net/base/network_delegate_impl.cc (right):

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate_i...
net/base/network_delegate_impl.cc:63: void
NetworkDelegateImpl::OnNetworkBytesReceived(const URLRequest& request,
What's the argument for this approach instead of having request own a struct of
data usage statistics?

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
net/url_request/url_request_job.cc:786: void
URLRequestJob::MaybeNotifyNetworkBytes() {
Is this for sent bytes too? If not, the name should be specific to received
bytes.

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
net/url_request/url_request_job.cc:793: // here to avoid reporting negative
network usage in these cases.
What happens to the bytes received between the last read and here?

[sclittle, 2015-08-17 23:35:14]

https://codereview.chromium.org/1284993005/diff/1/net/base/layered_network_de...
File net/base/layered_network_delegate.h (right):

https://codereview.chromium.org/1284993005/diff/1/net/base/layered_network_de...
net/base/layered_network_delegate.h:67: int64 bytes_received) final;
On 2015/08/17 21:30:39, bengr wrote:
> Use int64_t and include <stdint.h>. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/basictypes.h&...

Done.

https://codereview.chromium.org/1284993005/diff/1/net/base/layered_network_de...
net/base/layered_network_delegate.h:130: int64 bytes_received);
On 2015/08/17 21:30:39, bengr wrote:
> What does bytes_received mean? Does it include, e.g., SPDY framing, TLS
> overhead, TCP retransmits, etc?

See comment in network_delegate.h.

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate.h
File net/base/network_delegate.h (right):

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate.h...
net/base/network_delegate.h:90: void NotifyNetworkBytesReceived(const
URLRequest& request, int64 bytes_read);
On 2015/08/17 20:51:17, tbansal1 wrote:
> |bytes_read| or |bytes_received|? I believe you are using both.

Changed to bytes_received.

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate.h...
net/base/network_delegate.h:206: // Called when bytes are received from the
network, such as after receiving
On 2015/08/17 20:51:17, tbansal1 wrote:
> Would this also get triggered on requests that are served by localhost or by
> cache?

I've added a comment here. Right now, this would include localhost, but it
wouldn't include cached response bytes that didn't come over any network.

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate.h...
net/base/network_delegate.h:208: // bytes measured at the application layer that
have been received over the
On 2015/08/17 21:30:39, bengr wrote:
> It's work some more comment space to be ultra specific about what this
includes.
> E.g., currently, TLS overhead is not included.

I've added some more detail here, but I'm not exactly sure what is and isn't
included in URLRequest::GetTotalReceivedBytes.

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate_i...
File net/base/network_delegate_impl.cc (right):

https://codereview.chromium.org/1284993005/diff/1/net/base/network_delegate_i...
net/base/network_delegate_impl.cc:63: void
NetworkDelegateImpl::OnNetworkBytesReceived(const URLRequest& request,
On 2015/08/17 21:30:39, bengr wrote:
> What's the argument for this approach instead of having request own a struct
of
> data usage statistics?

This is just a way for consumers to be notified how many network bytes were
received, when they're received. We'll probably want to expose some data usage
statistics from the request as well, like GetTotalReceivedBytes() is right now.

The advantage of this style of notification is that consumers are notified when
bytes are read for a request, and they don't have to keep track of and compute
the delta bytes_received for a request in the consumer at each stage in the
request, e.g. between OnHeadersReceived() and OnRawBytesRead().

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
net/url_request/url_request_job.cc:786: void
URLRequestJob::MaybeNotifyNetworkBytes() {
On 2015/08/17 21:30:40, bengr wrote:
> Is this for sent bytes too? If not, the name should be specific to received
> bytes.

Eventually it will be for both sent and received, see my TODO on this method's
definition in the .h file.

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
net/url_request/url_request_job.cc:789: 
On 2015/08/17 20:51:17, tbansal1 wrote:
> May be worth it to add a DCHECK:
> DCHECK(!request_->was_cached()

It's possible that responses served from a local cache still used the network in
some way, such as revalidated appcache entries where was_cached=true and
GetTotalReceivedBytes reflects the bytes received in the 304 Not Modified
response that did come over the network.

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
net/url_request/url_request_job.cc:793: // here to avoid reporting negative
network usage in these cases.
On 2015/08/17 21:30:40, bengr wrote:
> What happens to the bytes received between the last read and here?

In URLRequestHttpJob, any remaining bytes received are reported shortly before
destroying the transaction.

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/1284993005/diff/1/net/url_request/url_request...
net/url_request/url_request_test_util.cc:513: total_network_bytes_received_ +=
bytes_received;
On 2015/08/17 20:51:17, tbansal1 wrote:
> nit: May be add the event to event_order_ for easier debugging.

Done.

[tbansal1, 2015-08-18 00:49:49]

lgtm

https://codereview.chromium.org/1284993005/diff/20001/net/url_request/url_req...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1284993005/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:160: EXPECT_EQ(51,
network_delegate_.total_network_bytes_received());
nit and optional:

It is not clear where 51 is coming from. So, may be something like:
std::string mock_read_header = "...";
std::string mock_read_body = "...";
and then
EXPECT_EQ(mock_read_header.length(),
request->received_response_content_length());
EXPECT_EQ(mock_read_header.length() + mock_read_body.length(),
network_delegate_.total_network_bytes_received());

https://codereview.chromium.org/1284993005/diff/20001/net/url_request/url_req...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/1284993005/diff/20001/net/url_request/url_req...
net/url_request/url_request_job.h:8: #include <stdint.h>
nit: I believe the preference is to separate .h from non .h includes by a line
break. Although, this is not religiously followed in the codebase.

[sclittle, 2015-08-18 18:04:57]

Patchset #3 (id:40001) has been deleted

[sclittle, 2015-08-18 18:06:16]

https://codereview.chromium.org/1284993005/diff/20001/net/url_request/url_req...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1284993005/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:160: EXPECT_EQ(51,
network_delegate_.total_network_bytes_received());
On 2015/08/18 00:49:49, tbansal1 wrote:
> nit and optional:
> 
> It is not clear where 51 is coming from. So, may be something like:
> std::string mock_read_header = "...";
> std::string mock_read_body = "...";
> and then
> EXPECT_EQ(mock_read_header.length(),
> request->received_response_content_length());
> EXPECT_EQ(mock_read_header.length() + mock_read_body.length(),
> network_delegate_.total_network_bytes_received());

The 51 is actually quite a bit more than headers.length() + body.length(), but
I've changed the checks to expect that the total_received_bytes are at least
headers.length() + body.length(), so that if total_received_bytes is made to be
more accurate in the future then it won't break these tests.

https://codereview.chromium.org/1284993005/diff/20001/net/url_request/url_req...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/1284993005/diff/20001/net/url_request/url_req...
net/url_request/url_request_job.h:8: #include <stdint.h>
On 2015/08/18 00:49:49, tbansal1 wrote:
> nit: I believe the preference is to separate .h from non .h includes by a line
> break. Although, this is not religiously followed in the codebase.

Done.

[sclittle, 2015-08-18 18:16:11]

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

[sclittle, 2015-08-18 18:16:11]

mmenke: *

Thanks in advance

[mmenke, 2015-08-18 18:33:13]

https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
net/base/network_delegate.cc:102: OnNetworkBytesReceived(request,
bytes_received);
What's the difference between a raw byte read and a network byte received?  Do
we really need both methods?

[sclittle, 2015-08-18 18:49:41]

https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
net/base/network_delegate.cc:102: OnNetworkBytesReceived(request,
bytes_received);
On 2015/08/18 18:33:12, mmenke wrote:
> What's the difference between a raw byte read and a network byte received?  Do
> we really need both methods?

The main difficulty would be getting the delta |bytes_received| over the
network. |bytes_read| in NotifyRawBytesRead is the delta raw response bytes
read, which isn't necessarily from over the network, and IIUC doesn't include
headers or other overhead, and raw response reads and network reads aren't 1:1.

Calling URLRequest::GetTotalReceivedBytes() on every raw byte read would require
the network delegate to keep track of that value for each request and compute
the delta network bytes_received itself.

[mmenke, 2015-08-18 18:56:07]

https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
net/base/network_delegate.cc:102: OnNetworkBytesReceived(request,
bytes_received);
On 2015/08/18 18:49:41, sclittle wrote:
> On 2015/08/18 18:33:12, mmenke wrote:
> > What's the difference between a raw byte read and a network byte received? 
Do
> > we really need both methods?
> 
> The main difficulty would be getting the delta |bytes_received| over the
> network. |bytes_read| in NotifyRawBytesRead is the delta raw response bytes
> read, which isn't necessarily from over the network, and IIUC doesn't include
> headers or other overhead, and raw response reads and network reads aren't
1:1.
> 
> Calling URLRequest::GetTotalReceivedBytes() on every raw byte read would
require
> the network delegate to keep track of that value for each request and compute
> the delta network bytes_received itself.

I'm strongly opposed to having two methods for this.  Can we modify
NotifyRawBytesRead to get what we want, without modifying consumers?  I'd be
fine with adding a bool indicating if the bytes came over the network or not,
but I don't think anything cares about non-network bytes, anyways.

[sclittle, 2015-08-18 20:13:42]

https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
net/base/network_delegate.cc:102: OnNetworkBytesReceived(request,
bytes_received);
On 2015/08/18 18:56:07, mmenke wrote:
> On 2015/08/18 18:49:41, sclittle wrote:
> > On 2015/08/18 18:33:12, mmenke wrote:
> > > What's the difference between a raw byte read and a network byte received?

> Do
> > > we really need both methods?
> > 
> > The main difficulty would be getting the delta |bytes_received| over the
> > network. |bytes_read| in NotifyRawBytesRead is the delta raw response bytes
> > read, which isn't necessarily from over the network, and IIUC doesn't
include
> > headers or other overhead, and raw response reads and network reads aren't
> 1:1.
> > 
> > Calling URLRequest::GetTotalReceivedBytes() on every raw byte read would
> require
> > the network delegate to keep track of that value for each request and
compute
> > the delta network bytes_received itself.
> 
> I'm strongly opposed to having two methods for this.  Can we modify
> NotifyRawBytesRead to get what we want, without modifying consumers?  I'd be
> fine with adding a bool indicating if the bytes came over the network or not,
> but I don't think anything cares about non-network bytes, anyways.

From a quick code search it looks like you're right, no network delegate seems
to care about non-network bytes read in OnRawBytesRead. We could modify
NotifyRawBytesRead to notify about only network bytes, or even replace
RawBytesRead with NetworkBytesReceived.

I think adding the bool would be weird though. AIUI, RawBytesRead and
NetworkBytesReceived are different conceptually - NetworkBytesReceived counts
bytes that were received over the network, and RawBytesRead counts response body
bytes that were read from these bytes received over the network. Chrome often
receives all the bytes from the network for a request, then reads the raw bytes
from those bit by bit, so there might be 1 chunk of 9KB NetworkBytesReceived
then 3 chunks of 3KB RawBytesRead.

WDYT?

[sclittle, 2015-08-18 20:20:26]

https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
net/base/network_delegate.cc:102: OnNetworkBytesReceived(request,
bytes_received);
On 2015/08/18 20:13:42, sclittle wrote:
> On 2015/08/18 18:56:07, mmenke wrote:
> > On 2015/08/18 18:49:41, sclittle wrote:
> > > On 2015/08/18 18:33:12, mmenke wrote:
> > > > What's the difference between a raw byte read and a network byte
received?
> 
> > Do
> > > > we really need both methods?
> > > 
> > > The main difficulty would be getting the delta |bytes_received| over the
> > > network. |bytes_read| in NotifyRawBytesRead is the delta raw response
bytes
> > > read, which isn't necessarily from over the network, and IIUC doesn't
> include
> > > headers or other overhead, and raw response reads and network reads aren't
> > 1:1.
> > > 
> > > Calling URLRequest::GetTotalReceivedBytes() on every raw byte read would
> > require
> > > the network delegate to keep track of that value for each request and
> compute
> > > the delta network bytes_received itself.
> > 
> > I'm strongly opposed to having two methods for this.  Can we modify
> > NotifyRawBytesRead to get what we want, without modifying consumers?  I'd be
> > fine with adding a bool indicating if the bytes came over the network or
not,
> > but I don't think anything cares about non-network bytes, anyways.
> 
> From a quick code search it looks like you're right, no network delegate seems
> to care about non-network bytes read in OnRawBytesRead. We could modify
> NotifyRawBytesRead to notify about only network bytes, or even replace
> RawBytesRead with NetworkBytesReceived.
> 
> I think adding the bool would be weird though. AIUI, RawBytesRead and
> NetworkBytesReceived are different conceptually - NetworkBytesReceived counts
> bytes that were received over the network, and RawBytesRead counts response
body
> bytes that were read from these bytes received over the network. Chrome often
> receives all the bytes from the network for a request, then reads the raw
bytes
> from those bit by bit, so there might be 1 chunk of 9KB NetworkBytesReceived
> then 3 chunks of 3KB RawBytesRead.
> 
> WDYT?

Another alternative might just be to add an int64 |network_bytes_received|
parameter to NotifyRawBytesRead, and accept the fact that sometimes either of
|response_bytes_read| or |network_bytes_received| could be 0.

[mmenke, 2015-08-18 20:27:47]

On 2015/08/18 20:20:26, sclittle wrote:
>
https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
> File net/base/network_delegate.cc (right):
> 
>
https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
> net/base/network_delegate.cc:102: OnNetworkBytesReceived(request,
> bytes_received);
> On 2015/08/18 20:13:42, sclittle wrote:
> > On 2015/08/18 18:56:07, mmenke wrote:
> > > On 2015/08/18 18:49:41, sclittle wrote:
> > > > On 2015/08/18 18:33:12, mmenke wrote:
> > > > > What's the difference between a raw byte read and a network byte
> received?
> > 
> > > Do
> > > > > we really need both methods?
> > > > 
> > > > The main difficulty would be getting the delta |bytes_received| over the
> > > > network. |bytes_read| in NotifyRawBytesRead is the delta raw response
> bytes
> > > > read, which isn't necessarily from over the network, and IIUC doesn't
> > include
> > > > headers or other overhead, and raw response reads and network reads
aren't
> > > 1:1.
> > > > 
> > > > Calling URLRequest::GetTotalReceivedBytes() on every raw byte read would
> > > require
> > > > the network delegate to keep track of that value for each request and
> > compute
> > > > the delta network bytes_received itself.
> > > 
> > > I'm strongly opposed to having two methods for this.  Can we modify
> > > NotifyRawBytesRead to get what we want, without modifying consumers?  I'd
be
> > > fine with adding a bool indicating if the bytes came over the network or
> not,
> > > but I don't think anything cares about non-network bytes, anyways.
> > 
> > From a quick code search it looks like you're right, no network delegate
seems
> > to care about non-network bytes read in OnRawBytesRead. We could modify
> > NotifyRawBytesRead to notify about only network bytes, or even replace
> > RawBytesRead with NetworkBytesReceived.
> > 
> > I think adding the bool would be weird though. AIUI, RawBytesRead and
> > NetworkBytesReceived are different conceptually - NetworkBytesReceived
counts
> > bytes that were received over the network, and RawBytesRead counts response
> body
> > bytes that were read from these bytes received over the network. Chrome
often
> > receives all the bytes from the network for a request, then reads the raw
> bytes
> > from those bit by bit, so there might be 1 chunk of 9KB NetworkBytesReceived
> > then 3 chunks of 3KB RawBytesRead.
> > 
> > WDYT?
> 
> Another alternative might just be to add an int64 |network_bytes_received|
> parameter to NotifyRawBytesRead, and accept the fact that sometimes either of
> |response_bytes_read| or |network_bytes_received| could be 0.

Since one only counts response body bytes, combining them like that seems a
little odd.  Suggest just renaming the current one to NetworkBytesRead (Or
Received - either's fine) and making it give what we want.  I'm fine with either
making it update once per URLRequestJob, or once per read - the latter is nice
in that it involves less work.  The former is nice in that it mirrors when we
call the NQE.

[sclittle, 2015-08-18 20:33:32]

On 2015/08/18 20:27:47, mmenke wrote:
> On 2015/08/18 20:20:26, sclittle wrote:
> >
>
https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
> > File net/base/network_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/1284993005/diff/60001/net/base/network_delega...
> > net/base/network_delegate.cc:102: OnNetworkBytesReceived(request,
> > bytes_received);
> > On 2015/08/18 20:13:42, sclittle wrote:
> > > On 2015/08/18 18:56:07, mmenke wrote:
> > > > On 2015/08/18 18:49:41, sclittle wrote:
> > > > > On 2015/08/18 18:33:12, mmenke wrote:
> > > > > > What's the difference between a raw byte read and a network byte
> > received?
> > > 
> > > > Do
> > > > > > we really need both methods?
> > > > > 
> > > > > The main difficulty would be getting the delta |bytes_received| over
the
> > > > > network. |bytes_read| in NotifyRawBytesRead is the delta raw response
> > bytes
> > > > > read, which isn't necessarily from over the network, and IIUC doesn't
> > > include
> > > > > headers or other overhead, and raw response reads and network reads
> aren't
> > > > 1:1.
> > > > > 
> > > > > Calling URLRequest::GetTotalReceivedBytes() on every raw byte read
would
> > > > require
> > > > > the network delegate to keep track of that value for each request and
> > > compute
> > > > > the delta network bytes_received itself.
> > > > 
> > > > I'm strongly opposed to having two methods for this.  Can we modify
> > > > NotifyRawBytesRead to get what we want, without modifying consumers? 
I'd
> be
> > > > fine with adding a bool indicating if the bytes came over the network or
> > not,
> > > > but I don't think anything cares about non-network bytes, anyways.
> > > 
> > > From a quick code search it looks like you're right, no network delegate
> seems
> > > to care about non-network bytes read in OnRawBytesRead. We could modify
> > > NotifyRawBytesRead to notify about only network bytes, or even replace
> > > RawBytesRead with NetworkBytesReceived.
> > > 
> > > I think adding the bool would be weird though. AIUI, RawBytesRead and
> > > NetworkBytesReceived are different conceptually - NetworkBytesReceived
> counts
> > > bytes that were received over the network, and RawBytesRead counts
response
> > body
> > > bytes that were read from these bytes received over the network. Chrome
> often
> > > receives all the bytes from the network for a request, then reads the raw
> > bytes
> > > from those bit by bit, so there might be 1 chunk of 9KB
NetworkBytesReceived
> > > then 3 chunks of 3KB RawBytesRead.
> > > 
> > > WDYT?
> > 
> > Another alternative might just be to add an int64 |network_bytes_received|
> > parameter to NotifyRawBytesRead, and accept the fact that sometimes either
of
> > |response_bytes_read| or |network_bytes_received| could be 0.
> 
> Since one only counts response body bytes, combining them like that seems a
> little odd.  Suggest just renaming the current one to NetworkBytesRead (Or
> Received - either's fine) and making it give what we want.  I'm fine with
either
> making it update once per URLRequestJob, or once per read - the latter is nice
> in that it involves less work.  The former is nice in that it mirrors when we
> call the NQE.

Got it, I'll rename the current one to NetworkBytesReceived. Thanks!

[sclittle, 2015-08-18 21:36:08]

> > Since one only counts response body bytes, combining them like that seems a
> > little odd.  Suggest just renaming the current one to NetworkBytesRead (Or
> > Received - either's fine) and making it give what we want.  I'm fine with
> either
> > making it update once per URLRequestJob, or once per read - the latter is
nice
> > in that it involves less work.  The former is nice in that it mirrors when
we
> > call the NQE.
> 
> Got it, I'll rename the current one to NetworkBytesReceived. Thanks!

On second thought, would it be OK to just add NetworkBytesReceived in this CL
and then remove RawBytesRead in a followup CL? Removing RawBytesRead touches
several network delegates in different parts of the Chrome codebase (most have
an empty implementation of it), and it seems like that would be easier for
reviewers as a separate CL.

[mmenke, 2015-08-18 21:38:25]

On 2015/08/18 21:36:08, sclittle wrote:
> > > Since one only counts response body bytes, combining them like that seems
a
> > > little odd.  Suggest just renaming the current one to NetworkBytesRead (Or
> > > Received - either's fine) and making it give what we want.  I'm fine with
> > either
> > > making it update once per URLRequestJob, or once per read - the latter is
> nice
> > > in that it involves less work.  The former is nice in that it mirrors when
> we
> > > call the NQE.
> > 
> > Got it, I'll rename the current one to NetworkBytesReceived. Thanks!
> 
> On second thought, would it be OK to just add NetworkBytesReceived in this CL
> and then remove RawBytesRead in a followup CL? Removing RawBytesRead touches
> several network delegates in different parts of the Chrome codebase (most have
> an empty implementation of it), and it seems like that would be easier for
> reviewers as a separate CL.

I'd rather not, just to make absolutely sure we can get rid of the new method. 
I've seen cases like this where we had to end up keeping both old and new ways
of doing things indefinitely.  Really don't want to end up like that here.  I
don't think we will, but I've learned to be paranoid.

[sclittle, 2015-08-19 00:01:23]

sclittle@chromium.org changed reviewers:
+ nick@chromium.org, rohitrao@chromium.org, sgurun@chromium.org

[sclittle, 2015-08-19 00:01:23]

I've replaced NetworkDelegate::OnRawBytesRead() with
NetworkDelegate::OnNetworkBytesReceived().

mmenke: net, cronet
nick: content, task_manager
rohitrao: ios
sgurun: webview

Thanks in advance!

[rohitrao (ping after 24h), 2015-08-19 00:06:15]

ios lgtm

[sgurun-gerrit only, 2015-08-19 01:50:09]

On 2015/08/19 00:06:15, rohitrao wrote:
> ios lgtm

aw lgtm

[mmenke, 2015-08-19 15:48:41]

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1545: MaybeNotifyNetworkBytes();
Why is this needed, when we call this in NotifyDone?  Cancellation?  I'd really
rather we not increase the coupling between this and its parent class.

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:137: reads[0].data_len +
reads[1].data_len);
We should be able to do EXPECT_EQ in these.

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:224:
TestNetworkBytesCancelledAfterHeaders) {
Maybe just a simple sanity test - start a request, cancel it, and make sure
nothing was received?  Just need to add a socket data provider with an
asynchronous connect, and no reads.

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:224:
TestNetworkBytesCancelledAfterHeaders) {
Maybe an HTTP/0.9 one, too?

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:795: // transaction in these cases.
Having the class have to know about that behavior makes for a really ugly API
contract for GetTotalReceivedBytes.

Can we have an API contract where we either just can't call it after
cancellation, or where it works after cancellation?

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:895: MaybeNotifyNetworkBytes();
Think it's worth commenting on the difference between what this notification is
sending, and bytes_read.

[sclittle, 2015-08-20 01:10:49]

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1545: MaybeNotifyNetworkBytes();
On 2015/08/19 15:48:41, mmenke wrote:
> Why is this needed, when we call this in NotifyDone?  Cancellation?  I'd
really
> rather we not increase the coupling between this and its parent class.

This notification is no longer needed now that
URLRequestHttpJob::GetTotalReceivedBytes() doesn't drop to 0 when the request is
cancelled.

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:137: reads[0].data_len +
reads[1].data_len);
On 2015/08/19 15:48:41, mmenke wrote:
> We should be able to do EXPECT_EQ in these.

Done.

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:224:
TestNetworkBytesCancelledAfterHeaders) {
On 2015/08/19 15:48:41, mmenke wrote:
> Maybe just a simple sanity test - start a request, cancel it, and make sure
> nothing was received?  Just need to add a socket data provider with an
> asynchronous connect, and no reads.

Done.

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:224:
TestNetworkBytesCancelledAfterHeaders) {
On 2015/08/19 15:48:41, mmenke wrote:
> Maybe an HTTP/0.9 one, too?

Done.

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:795: // transaction in these cases.
On 2015/08/19 15:48:41, mmenke wrote:
> Having the class have to know about that behavior makes for a really ugly API
> contract for GetTotalReceivedBytes.
> 
> Can we have an API contract where we either just can't call it after
> cancellation, or where it works after cancellation?

I've updated URLRequestHttpJob such that GetTotalReceivedBytes() doesn't drop to
0 after the transaction is destroyed.

https://codereview.chromium.org/1284993005/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.cc:895: MaybeNotifyNetworkBytes();
On 2015/08/19 15:48:41, mmenke wrote:
> Think it's worth commenting on the difference between what this notification
is
> sending, and bytes_read.

Done.

[mmenke, 2015-08-20 15:59:34]

LGTM, once my comment is addressed!

https://codereview.chromium.org/1284993005/diff/100001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1284993005/diff/100001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:142: "Content-Length:
12\r\n\r\n"),
Sorry, should have actually said what HTTP/0.9 looks like.  HTTP/0.9 is just:

MockRead reads[] = {
    MockRead("All your base are belong to us!"),
};

(i.e. no headers, no status line)

[sclittle, 2015-08-20 18:18:23]

https://codereview.chromium.org/1284993005/diff/100001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1284993005/diff/100001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:142: "Content-Length:
12\r\n\r\n"),
On 2015/08/20 15:59:33, mmenke wrote:
> Sorry, should have actually said what HTTP/0.9 looks like.  HTTP/0.9 is just:
> 
> MockRead reads[] = {
>     MockRead("All your base are belong to us!"),
> };
> 
> (i.e. no headers, no status line)

Done.

[mmenke, 2015-08-21 22:01:58]

In case you're waiting for me to sign off again, this still LGTM.

[ncarter (slow), 2015-08-24 17:36:08]

task_manager lgtm

[sclittle, 2015-08-24 18:04:13]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-08-24 18:04:14]

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

[commit-bot: I haz the power, 2015-08-24 18:04:44]

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

[commit-bot: I haz the power, 2015-08-24 18:22:25]

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

[commit-bot: I haz the power, 2015-08-24 18:22:26]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[sclittle, 2015-08-24 18:49:26]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-08-24 18:49:27]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
nick@chromium.org, tbansal@chromium.org, mmenke@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/1284993005/#ps140001
(title: "Removed empty cronet OnRawBytesRead implementation")

[commit-bot: I haz the power, 2015-08-24 18:49:41]

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

[commit-bot: I haz the power, 2015-08-24 20:21:07]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-08-24 20:21:53]

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