Remove OnDataAvailable() notification when trailers are to be delivered

Remove OnDataAvailable() notification when trailers are to be delivered

This CL used the change that is suggested by mpw@ in
https://bugs.chromium.org/p/chromium/issues/detail?id=652935#c1.

Specifically, this CL removes
QuicChromiumClientStream::Delegate::OnDataAvailable()
notification if there is no byte can be read and trailers are to be delivered.

This CL adjusted unit tests to test the correct ordering of events.

BUG=652935
Committed: https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3
Cr-Commit-Position: refs/heads/master@{#423357}

[xunjieli, 2016-10-05 17:37:02]

Description was changed from

==========
temp

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

to

==========
Remove OnDataAvailable() notification when trailers are to be delivered

This CL removes
QuicChromiumClientStream::Delegate::OnDataAvailable()
notification if there is no byte can be read and trailers are to be delivered.


BUG=652935
==========

[xunjieli, 2016-10-05 17:37:15]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-10-05 17:37:21]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 17:37:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2016-10-05 17:39:05]

Description was changed from

==========
Remove OnDataAvailable() notification when trailers are to be delivered

This CL removes
QuicChromiumClientStream::Delegate::OnDataAvailable()
notification if there is no byte can be read and trailers are to be delivered.


BUG=652935
==========

to

==========
Remove OnDataAvailable() notification when trailers are to be delivered

This CL removes
QuicChromiumClientStream::Delegate::OnDataAvailable()
notification if there is no byte can be read and trailers are to be delivered.

This CL adjusted unit tests to test the correct ordering of events.

BUG=652935
==========

[xunjieli, 2016-10-05 17:39:05]

xunjieli@chromium.org changed reviewers:
+ mpw@chromium.org, zhongyi@chromium.org

[xunjieli, 2016-10-05 17:43:52]

Description was changed from

==========
Remove OnDataAvailable() notification when trailers are to be delivered

This CL removes
QuicChromiumClientStream::Delegate::OnDataAvailable()
notification if there is no byte can be read and trailers are to be delivered.

This CL adjusted unit tests to test the correct ordering of events.

BUG=652935
==========

to

==========
Remove OnDataAvailable() notification when trailers are to be delivered

This CL used the change that is suggested by mpw@ in 
https://bugs.chromium.org/p/chromium/issues/detail?id=652935#c1.

Specifically, this CL removes
QuicChromiumClientStream::Delegate::OnDataAvailable()
notification if there is no byte can be read and trailers are to be delivered.

This CL adjusted unit tests to test the correct ordering of events.

BUG=652935
==========

[xunjieli, 2016-10-05 17:47:19]

zhongyi, mpw@: PTAL. Thanks!

https://codereview.chromium.org/2393233002/diff/40001/net/quic/core/quic_spdy...
File net/quic/core/quic_spdy_stream.cc (right):

https://codereview.chromium.org/2393233002/diff/40001/net/quic/core/quic_spdy...
net/quic/core/quic_spdy_stream.cc:296: trailers_decompressed_ = true;
This line (and the line below) is moved above OnStreamFrame() so
FinishedReadingTrailers() can correctly report that we are waiting for trailers.

[mef, 2016-10-05 17:58:42]

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

[mef, 2016-10-05 17:58:42]

https://codereview.chromium.org/2393233002/diff/40001/net/quic/chromium/bidir...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2393233002/diff/40001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:254: FROM_HERE,
base::Bind(&BidirectionalStreamQuicImpl::NotifyDataRead,
Is this right thing to do? What if there is a lot of data buffered, but not read
by the client? NotifyDataRead with 0 length would loose it, right?

[xunjieli, 2016-10-05 18:10:30]

https://codereview.chromium.org/2393233002/diff/40001/net/quic/chromium/bidir...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2393233002/diff/40001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:254: FROM_HERE,
base::Bind(&BidirectionalStreamQuicImpl::NotifyDataRead,
On 2016/10/05 17:58:42, mef wrote:
> Is this right thing to do? What if there is a lot of data buffered, but not
read
> by the client? NotifyDataRead with 0 length would loose it, right? 

Ah, that's right. Thanks for catching this! Done.

[xunjieli, 2016-10-05 18:41:24]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 18:42:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 19:42:08]

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

[commit-bot: I haz the power, 2016-10-05 19:42:09]

Dry run: This issue passed the CQ dry run.

[kapishnikov, 2016-10-05 21:05:08]

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

[kapishnikov, 2016-10-05 21:05:08]

https://codereview.chromium.org/2393233002/diff/60001/net/quic/chromium/bidir...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2393233002/diff/60001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:266: DCHECK(read_buffer_);
Can we remove this DCHECK? The previous 'if' statement guarantees that
read_buffer_ is not null.

[mpw, 2016-10-05 21:49:59]

lgtm

LGTM

[xunjieli, 2016-10-05 21:56:28]

Thanks!

https://codereview.chromium.org/2393233002/diff/60001/net/quic/chromium/bidir...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2393233002/diff/60001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:266: DCHECK(read_buffer_);
On 2016/10/05 21:05:08, kapishnikov wrote:
> Can we remove this DCHECK? The previous 'if' statement guarantees that
> read_buffer_ is not null.

Done. Ah, you are right. Not sure why I added those..

[Zhongyi Shi, 2016-10-05 23:16:30]

lgtm! Thanks for fixing this :)

https://codereview.chromium.org/2393233002/diff/80001/net/quic/core/quic_spdy...
File net/quic/core/quic_spdy_stream.h (right):

https://codereview.chromium.org/2393233002/diff/80001/net/quic/core/quic_spdy...
net/quic/core/quic_spdy_stream.h:194: 
You will need to create a internal CL to port the changes in quic/core back to
the internal code base to keep the code in sync.

[xunjieli, 2016-10-05 23:20:23]

On 2016/10/05 23:16:30, Zhongyi Shi wrote:
> lgtm! Thanks for fixing this :)
> 
>
https://codereview.chromium.org/2393233002/diff/80001/net/quic/core/quic_spdy...
> File net/quic/core/quic_spdy_stream.h (right):
> 
>
https://codereview.chromium.org/2393233002/diff/80001/net/quic/core/quic_spdy...
> net/quic/core/quic_spdy_stream.h:194: 
> You will need to create a internal CL to port the changes in quic/core back to
> the internal code base to keep the code in sync.

Thanks! I will do a internal CL to merge the changes to quic/core.

[xunjieli, 2016-10-05 23:20:39]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-10-05 23:20:40]

The patchset sent to the CQ was uploaded after l-g-t-m from mpw@chromium.org
Link to the patchset: https://codereview.chromium.org/2393233002/#ps80001
(title: "Address Andrei's comments")

[commit-bot: I haz the power, 2016-10-05 23:21:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 00:46:06]

Description was changed from

==========
Remove OnDataAvailable() notification when trailers are to be delivered

This CL used the change that is suggested by mpw@ in 
https://bugs.chromium.org/p/chromium/issues/detail?id=652935#c1.

Specifically, this CL removes
QuicChromiumClientStream::Delegate::OnDataAvailable()
notification if there is no byte can be read and trailers are to be delivered.

This CL adjusted unit tests to test the correct ordering of events.

BUG=652935
==========

to

==========
Remove OnDataAvailable() notification when trailers are to be delivered

This CL used the change that is suggested by mpw@ in 
https://bugs.chromium.org/p/chromium/issues/detail?id=652935#c1.

Specifically, this CL removes
QuicChromiumClientStream::Delegate::OnDataAvailable()
notification if there is no byte can be read and trailers are to be delivered.

This CL adjusted unit tests to test the correct ordering of events.

BUG=652935
==========

[commit-bot: I haz the power, 2016-10-06 00:46:07]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-10-06 00:48:18]

Description was changed from

==========
Remove OnDataAvailable() notification when trailers are to be delivered

This CL used the change that is suggested by mpw@ in 
https://bugs.chromium.org/p/chromium/issues/detail?id=652935#c1.

Specifically, this CL removes
QuicChromiumClientStream::Delegate::OnDataAvailable()
notification if there is no byte can be read and trailers are to be delivered.

This CL adjusted unit tests to test the correct ordering of events.

BUG=652935
==========

to

==========
Remove OnDataAvailable() notification when trailers are to be delivered

This CL used the change that is suggested by mpw@ in
https://bugs.chromium.org/p/chromium/issues/detail?id=652935#c1.

Specifically, this CL removes
QuicChromiumClientStream::Delegate::OnDataAvailable()
notification if there is no byte can be read and trailers are to be delivered.

This CL adjusted unit tests to test the correct ordering of events.

BUG=652935

Committed: https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3
Cr-Commit-Position: refs/heads/master@{#423357}
==========

[commit-bot: I haz the power, 2016-10-06 00:48:19]

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