Added and implemented HttpStream::GetTotalSentBytes for basic streams.

Added and implemented HttpStream::GetTotalSentBytes for basic streams.

Added HttpStream::GetTotalSentBytes, which counts the number of bytes
sent over the network, similar to HttpStream::GetTotalReceivedBytes.

This CL implements this method for basic HTTP streams; implementations
for SPDY and QUIC streams will be added in later CLs.

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

[sclittle, 2015-08-31 22:00:55]

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

[sclittle, 2015-08-31 22:00:56]

bengr, mmenke: *

Thanks in advance!

[mmenke, 2015-08-31 22:03:30]

Thanks for splitting this up!  I'll get to this tomorrow.

[mmenke, 2015-08-31 22:23:13]

First pass comments, want to think a bit more about tests.  I think this looks
really good.

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream....
net/http/http_basic_stream.cc:91: if (parser())
This check isn't necessary (And while you're here, could you remove the
corresponding one from HttpBasicStream::GetTotalReceivedBytes?)

https://codereview.chromium.org/1314783007/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9: #include <stdint.h>
nit:  Know it was like this before, but there should be a blank line between C
and C++ headers.

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
File net/http/http_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
net/http/http_stream_parser_unittest.cc:65: int64_t WritesSize(const MockWrite
writes[], size_t length) {
Suggest calling this CountWriteBytes.

WriteBytes matches the HttpStreamParser accessor's name.  "Count" indicates how
it's getting them.

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
net/http/http_stream_parser_unittest.cc:65: int64_t WritesSize(const MockWrite
writes[], size_t length) {
Maybe make this part of SequencedSocketData instead?  Or do your other tests not
need something similar?  If you do this, should add a similar read method, even
though you aren't currently using it.

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
net/http/http_stream_parser_unittest.cc:1038: ASSERT_EQ(OK,
parser.SendRequest("GET / HTTP/1.1\r\n", HttpRequestHeaders(),
These should probably just be EXPECTs - at least the tendency in net/ is to only
use ASSERT if the later lines will crash if the ASSERT fails, which I don't
believe is the case here.

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
net/http/http_stream_parser_unittest.cc:1177: TEST(HttpStreamParser,
SentBytesChunkedPost) {
Rather than duplicate some of these larger tests, suggest just adding the new
check to the old tests.  I don't think we get a whole lot by separating them
out.  I feel that more basic tests should go earlier, so if you go with this,
should move the your earlier tests earlier in the file, too.

[sclittle, 2015-09-01 01:21:48]

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream....
net/http/http_basic_stream.cc:91: if (parser())
On 2015/08/31 22:23:13, mmenke wrote:
> This check isn't necessary (And while you're here, could you remove the
> corresponding one from HttpBasicStream::GetTotalReceivedBytes?)

This check is needed for GetTotalSent/ReceivedBytes to be called before
InitializeStream is called. I don't know if anything actually calls
GetTotalReceivedBytes before the stream is initialized, but the
HttpStream::GetTotalSent/ReceivedBytes interface doesn't forbid it and 0 is the
correct value to return in that case, so I think we should support that here.

WDYT?

https://codereview.chromium.org/1314783007/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9: #include <stdint.h>
On 2015/08/31 22:23:13, mmenke wrote:
> nit:  Know it was like this before, but there should be a blank line between C
> and C++ headers.

Done.

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
File net/http/http_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
net/http/http_stream_parser_unittest.cc:65: int64_t WritesSize(const MockWrite
writes[], size_t length) {
On 2015/08/31 22:23:13, mmenke wrote:
> Maybe make this part of SequencedSocketData instead?  Or do your other tests
not
> need something similar?  If you do this, should add a similar read method,
even
> though you aren't currently using it.

OK, I've created CountReadBytes and CountWriteBytes helper functions in
socket_test_util.

I don't want to make it part of the SequencedSocketData or the
SocketDataProvider, since the data provider shouldn't be responsible for
determining how many of the MockWrites/Reads *should* have been used.

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
net/http/http_stream_parser_unittest.cc:1038: ASSERT_EQ(OK,
parser.SendRequest("GET / HTTP/1.1\r\n", HttpRequestHeaders(),
On 2015/08/31 22:23:13, mmenke wrote:
> These should probably just be EXPECTs - at least the tendency in net/ is to
only
> use ASSERT if the later lines will crash if the ASSERT fails, which I don't
> believe is the case here.

Done.

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
net/http/http_stream_parser_unittest.cc:1177: TEST(HttpStreamParser,
SentBytesChunkedPost) {
On 2015/08/31 22:23:13, mmenke wrote:
> Rather than duplicate some of these larger tests, suggest just adding the new
> check to the old tests.  I don't think we get a whole lot by separating them
> out.  I feel that more basic tests should go earlier, so if you go with this,
> should move the your earlier tests earlier in the file, too.

Done.

[Bence, 2015-09-01 13:45:29]

bnc@chromium.org changed reviewers:
+ bnc@chromium.org

[Bence, 2015-09-01 13:45:30]

LGTM modulo comments.

Optional nit in description: title can be "Implement..." instead of "Add and
implement" for brevity.

Optional nit in description: s/counts the number of bytes/counts the bytes/. 
Again, for brevity.

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
File net/http/http_basic_stream.h (right):

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
net/http/http_basic_stream.h:71: int64_t GetTotalSentBytes() const override;
GetTotalReceivedBytes and GetTotalSentBytes should have the same return type. 
Ideally, either GetTotalReceivedBytes() should be changed to int64_t before this
CL lands, or GetTotalSentBytes() should be int64 for now and the two changed
together in a follow-up CL.  But I understand that that's a lot of extra work,
so I would be okay with this CL as it is as long as you make sure to change
GetTotalReceivedBytes() to int64_t soon after this lands.  (In any case, please
don't change it in this CL.)

Same for HttpStreamParses members and accessors.

[mmenke, 2015-09-01 17:39:43]

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream....
net/http/http_basic_stream.cc:91: if (parser())
On 2015/09/01 01:21:47, sclittle wrote:
> On 2015/08/31 22:23:13, mmenke wrote:
> > This check isn't necessary (And while you're here, could you remove the
> > corresponding one from HttpBasicStream::GetTotalReceivedBytes?)
> 
> This check is needed for GetTotalSent/ReceivedBytes to be called before
> InitializeStream is called. I don't know if anything actually calls
> GetTotalReceivedBytes before the stream is initialized, but the
> HttpStream::GetTotalSent/ReceivedBytes interface doesn't forbid it and 0 is
the
> correct value to return in that case, so I think we should support that here.
> 
> WDYT?

The same can be said of pretty much all these methods.  I don't see a path where
it is possible to call this with a NULL parser.

My feeling is that we shouldn't check for things that don't happen.  If we see
crashes here without the check, we should figure out why, and make sure we have
a test for that case.

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
File net/http/http_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
net/http/http_stream_parser_unittest.cc:65: int64_t WritesSize(const MockWrite
writes[], size_t length) {
On 2015/09/01 01:21:48, sclittle wrote:
> On 2015/08/31 22:23:13, mmenke wrote:
> > Maybe make this part of SequencedSocketData instead?  Or do your other tests
> not
> > need something similar?  If you do this, should add a similar read method,
> even
> > though you aren't currently using it.
> 
> OK, I've created CountReadBytes and CountWriteBytes helper functions in
> socket_test_util.
> 
> I don't want to make it part of the SequencedSocketData or the
> SocketDataProvider, since the data provider shouldn't be responsible for
> determining how many of the MockWrites/Reads *should* have been used.

I actually disagree here - seems like a utility function to query what was
passed to it...  My main reason to prefer it is I don't really like passing
length around.

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
File net/http/http_basic_stream.h (right):

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
net/http/http_basic_stream.h:71: int64_t GetTotalSentBytes() const override;
On 2015/09/01 13:45:29, Bence wrote:
> GetTotalReceivedBytes and GetTotalSentBytes should have the same return type. 
> Ideally, either GetTotalReceivedBytes() should be changed to int64_t before
this
> CL lands, or GetTotalSentBytes() should be int64 for now and the two changed
> together in a follow-up CL.  But I understand that that's a lot of extra work,
> so I would be okay with this CL as it is as long as you make sure to change
> GetTotalReceivedBytes() to int64_t soon after this lands.  (In any case,
please
> don't change it in this CL.)
> 
> Same for HttpStreamParses members and accessors.

I completely missed that, thanks for catching it.  I agree that we should be
consistent here.  I suggest just sticking with int64 for now (Or if you want to
change the other one in a preliminary CL, I'm fine with that, too.

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

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream....
net/http/http_basic_stream.cc:91: if (parser())
On 2015/09/01 17:39:43, mmenke wrote:
> On 2015/09/01 01:21:47, sclittle wrote:
> > On 2015/08/31 22:23:13, mmenke wrote:
> > > This check isn't necessary (And while you're here, could you remove the
> > > corresponding one from HttpBasicStream::GetTotalReceivedBytes?)
> > 
> > This check is needed for GetTotalSent/ReceivedBytes to be called before
> > InitializeStream is called. I don't know if anything actually calls
> > GetTotalReceivedBytes before the stream is initialized, but the
> > HttpStream::GetTotalSent/ReceivedBytes interface doesn't forbid it and 0 is
> the
> > correct value to return in that case, so I think we should support that
here.
> > 
> > WDYT?
> 
> The same can be said of pretty much all these methods.  I don't see a path
where
> it is possible to call this with a NULL parser.
> 
> My feeling is that we shouldn't check for things that don't happen.  If we see
> crashes here without the check, we should figure out why, and make sure we
have
> a test for that case.

Removing the check from GetTotalReceivedBytes causes a bunch of tests to crash,
so it looks like it does get called before stream initialization sometimes. I
don't want to re architect that, especially not in this CL, so do you mind if I
keep the check in here?

For example, there's a DCHECK in HttpNetworkTransaction to ensure that the
stream's GetTotalReceivedBytes is not carried over when a stream is renewed.
This would crash since GetTotalReceivedBytes is called before the renewed stream
is initialized:
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
File net/http/http_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_stream_parser...
net/http/http_stream_parser_unittest.cc:65: int64_t WritesSize(const MockWrite
writes[], size_t length) {
On 2015/09/01 17:39:43, mmenke wrote:
> On 2015/09/01 01:21:48, sclittle wrote:
> > On 2015/08/31 22:23:13, mmenke wrote:
> > > Maybe make this part of SequencedSocketData instead?  Or do your other
tests
> > not
> > > need something similar?  If you do this, should add a similar read method,
> > even
> > > though you aren't currently using it.
> > 
> > OK, I've created CountReadBytes and CountWriteBytes helper functions in
> > socket_test_util.
> > 
> > I don't want to make it part of the SequencedSocketData or the
> > SocketDataProvider, since the data provider shouldn't be responsible for
> > determining how many of the MockWrites/Reads *should* have been used.
> 
> I actually disagree here - seems like a utility function to query what was
> passed to it...  My main reason to prefer it is I don't really like passing
> length around.

Often the length is necessary - a single SocketDataProvider can satisfy multiple
requests if the socket is reused. At that point, it's useless to get the total
data_len of all the MockWrites from the SocketDataProvider, since only a subset
of the MockWrites apply to a single request.

None of the tests in this file reuse sockets like that, but many of the tests in
http_network_transaction_unittest do. I'm planning to make the
http_network_transaction tests switch over to using these helper functions in
the later CL when I plumb the GetTotalSentBytes through the HttpTransaction up
to the URLRequest.

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
File net/http/http_basic_stream.h (right):

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
net/http/http_basic_stream.h:71: int64_t GetTotalSentBytes() const override;
On 2015/09/01 17:39:43, mmenke wrote:
> On 2015/09/01 13:45:29, Bence wrote:
> > GetTotalReceivedBytes and GetTotalSentBytes should have the same return
type. 
> > Ideally, either GetTotalReceivedBytes() should be changed to int64_t before
> this
> > CL lands, or GetTotalSentBytes() should be int64 for now and the two changed
> > together in a follow-up CL.  But I understand that that's a lot of extra
work,
> > so I would be okay with this CL as it is as long as you make sure to change
> > GetTotalReceivedBytes() to int64_t soon after this lands.  (In any case,
> please
> > don't change it in this CL.)
> > 
> > Same for HttpStreamParses members and accessors.
> 
> I completely missed that, thanks for catching it.  I agree that we should be
> consistent here.  I suggest just sticking with int64 for now (Or if you want
to
> change the other one in a preliminary CL, I'm fine with that, too.

Sure. Do you mind if I keep GetTotalSentBytes as int64_t in this CL, and change
over GetTotalReceivedBytes from int64 to int64_t in a separate CL that I'll put
together right now and send out in parallel?

[mmenke, 2015-09-01 20:09:07]

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
File net/http/http_basic_stream.h (right):

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
net/http/http_basic_stream.h:71: int64_t GetTotalSentBytes() const override;
On 2015/09/01 20:03:00, sclittle wrote:
> On 2015/09/01 17:39:43, mmenke wrote:
> > On 2015/09/01 13:45:29, Bence wrote:
> > > GetTotalReceivedBytes and GetTotalSentBytes should have the same return
> type. 
> > > Ideally, either GetTotalReceivedBytes() should be changed to int64_t
before
> > this
> > > CL lands, or GetTotalSentBytes() should be int64 for now and the two
changed
> > > together in a follow-up CL.  But I understand that that's a lot of extra
> work,
> > > so I would be okay with this CL as it is as long as you make sure to
change
> > > GetTotalReceivedBytes() to int64_t soon after this lands.  (In any case,
> > please
> > > don't change it in this CL.)
> > > 
> > > Same for HttpStreamParses members and accessors.
> > 
> > I completely missed that, thanks for catching it.  I agree that we should be
> > consistent here.  I suggest just sticking with int64 for now (Or if you want
> to
> > change the other one in a preliminary CL, I'm fine with that, too.
> 
> Sure. Do you mind if I keep GetTotalSentBytes as int64_t in this CL, and
change
> over GetTotalReceivedBytes from int64 to int64_t in a separate CL that I'll
put
> together right now and send out in parallel?

Works for me

[bengr, 2015-09-01 23:48:26]

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
net/http/http_basic_stream.cc:91: if (parser())
For both Sent and Received, is there the potential to miss some bytes if
!parser()? If so, what are the specific cases?

[sclittle, 2015-09-01 23:57:23]

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/1314783007/diff/20001/net/http/http_basic_str...
net/http/http_basic_stream.cc:91: if (parser())
On 2015/09/01 23:48:26, bengr wrote:
> For both Sent and Received, is there the potential to miss some bytes if
> !parser()? If so, what are the specific cases?

When RenewStreamForAuth is called, then parser() will be deleted, and
GetTotalReceived/SentBytes will drop to 0. The HttpNetworkTransaction already
accounts for this properly, and there's a DCHECK in
URLRequestJob::NotifyNetworkBytesReceived checking that the
GetTotalReceivedBytes visible to higher layers doesn't decrease.

[Bence, 2015-09-02 12:37:22]

> Sure. Do you mind if I keep GetTotalSentBytes as int64_t in this CL, and
change
> over GetTotalReceivedBytes from int64 to int64_t in a separate CL that I'll
put
> together right now and send out in parallel?

Sounds good to me, but I wouldn't bother crafting that other CL before this one
lands, to avoid extra work with merge conflicts.

[mmenke, 2015-09-02 17:36:54]

LGTM

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream....
net/http/http_basic_stream.cc:91: if (parser())
On 2015/09/01 20:03:00, sclittle wrote:
> On 2015/09/01 17:39:43, mmenke wrote:
> > On 2015/09/01 01:21:47, sclittle wrote:
> > > On 2015/08/31 22:23:13, mmenke wrote:
> > > > This check isn't necessary (And while you're here, could you remove the
> > > > corresponding one from HttpBasicStream::GetTotalReceivedBytes?)
> > > 
> > > This check is needed for GetTotalSent/ReceivedBytes to be called before
> > > InitializeStream is called. I don't know if anything actually calls
> > > GetTotalReceivedBytes before the stream is initialized, but the
> > > HttpStream::GetTotalSent/ReceivedBytes interface doesn't forbid it and 0
is
> > the
> > > correct value to return in that case, so I think we should support that
> here.
> > > 
> > > WDYT?
> > 
> > The same can be said of pretty much all these methods.  I don't see a path
> where
> > it is possible to call this with a NULL parser.
> > 
> > My feeling is that we shouldn't check for things that don't happen.  If we
see
> > crashes here without the check, we should figure out why, and make sure we
> have
> > a test for that case.
> 
> Removing the check from GetTotalReceivedBytes causes a bunch of tests to
crash,
> so it looks like it does get called before stream initialization sometimes. I
> don't want to re architect that, especially not in this CL, so do you mind if
I
> keep the check in here?
> 
> For example, there's a DCHECK in HttpNetworkTransaction to ensure that the
> stream's GetTotalReceivedBytes is not carried over when a stream is renewed.
> This would crash since GetTotalReceivedBytes is called before the renewed
stream
> is initialized:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...

Do you know if it's needed for anything but the DCHECK?  (Fine as-is for now,
but I plan on significantly reworking that path, so I may change things here, at
some point).

[sclittle, 2015-09-02 18:32:37]

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/1314783007/diff/1/net/http/http_basic_stream....
net/http/http_basic_stream.cc:91: if (parser())
On 2015/09/02 17:36:54, mmenke wrote:
> On 2015/09/01 20:03:00, sclittle wrote:
> > On 2015/09/01 17:39:43, mmenke wrote:
> > > On 2015/09/01 01:21:47, sclittle wrote:
> > > > On 2015/08/31 22:23:13, mmenke wrote:
> > > > > This check isn't necessary (And while you're here, could you remove
the
> > > > > corresponding one from HttpBasicStream::GetTotalReceivedBytes?)
> > > > 
> > > > This check is needed for GetTotalSent/ReceivedBytes to be called before
> > > > InitializeStream is called. I don't know if anything actually calls
> > > > GetTotalReceivedBytes before the stream is initialized, but the
> > > > HttpStream::GetTotalSent/ReceivedBytes interface doesn't forbid it and 0
> is
> > > the
> > > > correct value to return in that case, so I think we should support that
> > here.
> > > > 
> > > > WDYT?
> > > 
> > > The same can be said of pretty much all these methods.  I don't see a path
> > where
> > > it is possible to call this with a NULL parser.
> > > 
> > > My feeling is that we shouldn't check for things that don't happen.  If we
> see
> > > crashes here without the check, we should figure out why, and make sure we
> > have
> > > a test for that case.
> > 
> > Removing the check from GetTotalReceivedBytes causes a bunch of tests to
> crash,
> > so it looks like it does get called before stream initialization sometimes.
I
> > don't want to re architect that, especially not in this CL, so do you mind
if
> I
> > keep the check in here?
> > 
> > For example, there's a DCHECK in HttpNetworkTransaction to ensure that the
> > stream's GetTotalReceivedBytes is not carried over when a stream is renewed.
> > This would crash since GetTotalReceivedBytes is called before the renewed
> stream
> > is initialized:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...
> 
> Do you know if it's needed for anything but the DCHECK?  (Fine as-is for now,
> but I plan on significantly reworking that path, so I may change things here,
at
> some point).

I don't know of anything else but that DCHECK, but there's nothing stopping code
from calling GetTotalReceivedBytes for a not-yet-initialized stream, especially
since InitializeStream() is allowed to return ERR_IO_PENDING.

[sclittle, 2015-09-02 18:33:00]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-09-02 18:33:00]

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

[commit-bot: I haz the power, 2015-09-02 18:33:24]

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

[commit-bot: I haz the power, 2015-09-02 19:40:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-02 19:41:16]

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