Added and implemented URLRequest::GetTotalSentBytes.

Added and implemented URLRequest::GetTotalSentBytes.

Added and implemented URLRequest::GetTotalSentBytes,
URLRequestJob::GetTotalSentBytes, and
HttpTransaction::GetTotalSentBytes, which count the number of bytes sent
over the network, similar to GetTotalReceivedBytes.

BUG=518897
Committed: https://crrev.com/fb24989ce3406cc7163ecd515b8aa0adc114e1de
Cr-Commit-Position: refs/heads/master@{#348243}

[sclittle, 2015-09-03 01:00:26]

Patchset #1 (id:1) has been deleted

[sclittle, 2015-09-03 01:02:04]

sclittle@chromium.org changed reviewers:
+ bengr@chromium.org, mmenke@chromium.org, pfeldman@chromium.org

[sclittle, 2015-09-03 01:02:05]

mmenke: net
pfeldman: chrome/browser/devtools
bengr: *

Thanks in advance!

[pfeldman, 2015-09-03 17:41:44]

devtools lgtm

[mmenke, 2015-09-04 15:21:49]

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_cache_uni...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:187: BoundNetLog(), NULL, NULL, NULL);
While you're here, mind switching these to be nullptr?  Same goes for the rest
of the modified lines

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7127: // Helper function to get the typical
request size in bytes of |transaction|.
Should note for both these functions that they assume HTTP/1.x

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7128: int64_t RequestSize(const MockTransaction&
transaction) {
TransactionSize and RequestSize are not good names for these methods - neither
maps to a clear meaning.

Maybe ExpectedTotalSentBytes() and ExpectedTotalReceivedBytes()?

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.h:328: // Total number of bytes sent on
streams for this transaction.
These comments aren't accurate - it's actually the number of bytes sent for all
previous HttpStreams for this transaction (All streams other than |stream_|? 
All discarded/destroyed HttpStreams?).

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:346: out.total_received_bytes =
trans->GetTotalReceivedBytes();
Nit:  Received is before sent everywhere else (In retrospect, we probably should
have put sent first everywhere, but not really worth going back and changing
everywhere.

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:399:
static_cast<int64_t>(line.size() + request_headers.ToString().size()),
We really should not be depending on GetFullRequestHeaders, or
headers.ToString().  The former isn't fully implemented, and the latter isn't
correct for SPDY or QUIC.

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:416: };
So I guess no tests that use this method use SPDY or QUIC?

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:14677: MockRead("HTTP/1.1 200
OK\r\n\r\n"), MockRead("hello world"),
Second mock read should be on next line...Or does git cl format insist it be
there?

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_transacti...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_transacti...
net/http/http_transaction_test_util.cc:412: sent_bytes_ +=
strlen(t->request_headers);
This logic seems ugly and pointless, given that we have to duplicate the exact
same logic in the cache unit tests.  Since these aren't real transactions, can
we just use some sort of bogus value, and expose it in the header, and then use
that for the cache tests?

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

https://codereview.chromium.org/1327763003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:139: EXPECT_EQ(51,
request->GetTotalReceivedBytes());
Can't we just calculate the received bytes from the mock reads.  We should also
add write expectations, and test those as well - seems more robust.

https://codereview.chromium.org/1327763003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:249: EXPECT_EQ(51,
request->GetTotalReceivedBytes());
Think these are worth a comment (Basically the opposite of the next comment -
should not include the redirect)

https://codereview.chromium.org/1327763003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:251: EXPECT_EQ(107,
network_delegate_.total_network_bytes_received());
We should do the same for bytes sent, to make sure the redirect is handled
properly.

[sclittle, 2015-09-04 22:03:10]

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_cache_uni...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:187: BoundNetLog(), NULL, NULL, NULL);
On 2015/09/04 15:21:48, mmenke wrote:
> While you're here, mind switching these to be nullptr?  Same goes for the rest
> of the modified lines

Done.

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7127: // Helper function to get the typical
request size in bytes of |transaction|.
On 2015/09/04 15:21:48, mmenke wrote:
> Should note for both these functions that they assume HTTP/1.x

Done.

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7128: int64_t RequestSize(const MockTransaction&
transaction) {
On 2015/09/04 15:21:48, mmenke wrote:
> TransactionSize and RequestSize are not good names for these methods - neither
> maps to a clear meaning.
> 
> Maybe ExpectedTotalSentBytes() and ExpectedTotalReceivedBytes()?

Done.

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.h:328: // Total number of bytes sent on
streams for this transaction.
On 2015/09/04 15:21:48, mmenke wrote:
> These comments aren't accurate - it's actually the number of bytes sent for
all
> previous HttpStreams for this transaction (All streams other than |stream_|? 
> All discarded/destroyed HttpStreams?).

Done.

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:346: out.total_received_bytes =
trans->GetTotalReceivedBytes();
On 2015/09/04 15:21:49, mmenke wrote:
> Nit:  Received is before sent everywhere else (In retrospect, we probably
should
> have put sent first everywhere, but not really worth going back and changing
> everywhere.

Done.

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:399:
static_cast<int64_t>(line.size() + request_headers.ToString().size()),
On 2015/09/04 15:21:49, mmenke wrote:
> We really should not be depending on GetFullRequestHeaders, or
> headers.ToString().  The former isn't fully implemented, and the latter isn't
> correct for SPDY or QUIC.

You're right, removed this. I forgot to remove this check here; it was replaced
with the check in SimpleGetHelper(MockRead, size_t) below.

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:416: };
On 2015/09/04 15:21:49, mmenke wrote:
> So I guess no tests that use this method use SPDY or QUIC?

That's correct.

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:14677: MockRead("HTTP/1.1 200
OK\r\n\r\n"), MockRead("hello world"),
On 2015/09/04 15:21:49, mmenke wrote:
> Second mock read should be on next line...Or does git cl format insist it be
> there?

git cl format did this.

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_transacti...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/1327763003/diff/20001/net/http/http_transacti...
net/http/http_transaction_test_util.cc:412: sent_bytes_ +=
strlen(t->request_headers);
On 2015/09/04 15:21:49, mmenke wrote:
> This logic seems ugly and pointless, given that we have to duplicate the exact
> same logic in the cache unit tests.  Since these aren't real transactions, can
> we just use some sort of bogus value, and expose it in the header, and then
use
> that for the cache tests?

Done.

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

https://codereview.chromium.org/1327763003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:139: EXPECT_EQ(51,
request->GetTotalReceivedBytes());
On 2015/09/04 15:21:49, mmenke wrote:
> Can't we just calculate the received bytes from the mock reads.  We should
also
> add write expectations, and test those as well - seems more robust.

Done.

https://codereview.chromium.org/1327763003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:249: EXPECT_EQ(51,
request->GetTotalReceivedBytes());
On 2015/09/04 15:21:49, mmenke wrote:
> Think these are worth a comment (Basically the opposite of the next comment -
> should not include the redirect)

Done.

https://codereview.chromium.org/1327763003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:251: EXPECT_EQ(107,
network_delegate_.total_network_bytes_received());
On 2015/09/04 15:21:49, mmenke wrote:
> We should do the same for bytes sent, to make sure the redirect is handled
> properly.

Yeah, that will come later in a separate CL when I add and implement
NetworkDelegate::OnNetworkBytesSent.

[bengr, 2015-09-08 18:20:10]

lgtm, with microscopic nit.

https://codereview.chromium.org/1327763003/diff/40001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1327763003/diff/40001/net/url_request/url_req...
net/url_request/url_request.h:367: // URLRequest -- i.e. the last redirect.
'-- i.e.' -> ', i.e.,'

[mmenke, 2015-09-10 15:15:38]

I'm sorry for slowness - was sick the last two days (And WFH today).  This LGTM.

https://codereview.chromium.org/1327763003/diff/40001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1327763003/diff/40001/net/url_request/url_req...
net/url_request/url_request.h:367: // URLRequest -- i.e. the last redirect.
Suggest "the last redirect" -> "reset on redirects, but not when multiple
roundtrips are used for range requests or auth."

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

https://codereview.chromium.org/1327763003/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:185:
TEST_F(URLRequestHttpJobWithMockSocketsTest, TestContentLengthAbortedRequest) {
While you're here, mind replacing Aborted with Failed?  Aborted generally means
cancelled with ERR_ABORTED, which is what the next test actually does.

[sclittle, 2015-09-10 18:03:58]

https://codereview.chromium.org/1327763003/diff/40001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1327763003/diff/40001/net/url_request/url_req...
net/url_request/url_request.h:367: // URLRequest -- i.e. the last redirect.
On 2015/09/10 15:15:38, mmenke wrote:
> Suggest "the last redirect" -> "reset on redirects, but not when multiple
> roundtrips are used for range requests or auth."

Done.

https://codereview.chromium.org/1327763003/diff/40001/net/url_request/url_req...
net/url_request/url_request.h:367: // URLRequest -- i.e. the last redirect.
On 2015/09/08 18:20:10, bengr wrote:
> '-- i.e.' -> ', i.e.,'

Done.

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

https://codereview.chromium.org/1327763003/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job_unittest.cc:185:
TEST_F(URLRequestHttpJobWithMockSocketsTest, TestContentLengthAbortedRequest) {
On 2015/09/10 15:15:38, mmenke wrote:
> While you're here, mind replacing Aborted with Failed?  Aborted generally
means
> cancelled with ERR_ABORTED, which is what the next test actually does.

Done.

[sclittle, 2015-09-10 18:04:16]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-09-10 18:04:16]

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

[commit-bot: I haz the power, 2015-09-10 18:04:59]

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

[commit-bot: I haz the power, 2015-09-10 19:04:50]

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

[commit-bot: I haz the power, 2015-09-10 19:04:51]

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

[sclittle, 2015-09-10 19:24:12]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-09-10 19:25:06]

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

[commit-bot: I haz the power, 2015-09-10 20:20:42]

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

[commit-bot: I haz the power, 2015-09-10 20:20:43]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sclittle, 2015-09-10 20:26:13]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-09-10 20:26:27]

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

[commit-bot: I haz the power, 2015-09-10 21:33:29]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-09-10 21:34:13]

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

[commit-bot: I haz the power, 2015-09-23 12:14:34]

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