Simplify the QuicDispatcher::OnCanWrite logic

Simplify the QuicDispatcher::OnCanWrite logic

Before a refactoring, connections used to be able to add themselves to
QuicDispatcher::write_blocked_list_ even when they weren't
write-blocked, making it necessary to give each connection only one
attempt to write. However, this is no longer the case. Thus, the
QuicDispatcher::OnCanWrite can now simply keep going until either no
work is left or a connection gets write-blocked, and it's guaranteed
that no connection will come up twice (since if it gets added back to
write_blocked_list_, it is now blocked and the loop terminates). I've
added a longer explanatory comment and inserted a stronger check to make
sure the condition holds.

Simplify QuicDispatcher::OnCanWrite logic

Most of the changes were already in net/quic/quic_dispatcher.*. Merged
missing changes.

Merge internal change: 70043318

R=rch@chromium.org

[Ryan Hamilton, 2014-06-27 14:28:39]

lgtm

[wtc, 2014-06-27 17:08:01]

Patch set 1 LGTM. I suggest some small changes.

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
File net/tools/quic/quic_dispatcher.cc (right):

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
net/tools/quic/quic_dispatcher.cc:351: // Return without adding the session to
the blocked list, to avoid infinite

I believe |blocked| is a QuicConnection, not a QuicSession. If so, the comment
should say "connection" or "blocked writer" instead of "session".

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
File net/tools/quic/quic_dispatcher.h (right):

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
net/tools/quic/quic_dispatcher.h:100: virtual void
OnWriteBlocked(QuicBlockedWriterInterface* blocked) OVERRIDE;

Nit: This virtual method comes from the QuicServerSessionVisitor interface. The
parameter is named |writer| in that interface. If we think |blocked| is a better
name, we should rename the parameter throughout.

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/test_tools/qu...
File net/tools/quic/test_tools/quic_dispatcher_peer.cc (right):

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/test_tools/qu...
net/tools/quic/test_tools/quic_dispatcher_peer.cc:56: 

Nit: delete one blank line.

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/test_tools/qu...
File net/tools/quic/test_tools/quic_dispatcher_peer.h (right):

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/test_tools/qu...
net/tools/quic/test_tools/quic_dispatcher_peer.h:39: static
QuicDispatcher::WriteBlockedList* GetWriteBlockedList(

We can further lock down insertions to QuicDispatcher::write_blocked_list_ by
making this getter method return a const QuicDispatcher::WriteBlockedList*. But
quic_dispatcher_peer.h needs to erase connections from
QuicDispatcher::write_blocked_list_, so we can't do this.

[dmziegler, 2014-06-27 17:26:42]

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
File net/tools/quic/quic_dispatcher.h (right):

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
net/tools/quic/quic_dispatcher.h:100: virtual void
OnWriteBlocked(QuicBlockedWriterInterface* blocked) OVERRIDE;
On 2014/06/27 17:08:01, wtc wrote:
> 
> Nit: This virtual method comes from the QuicServerSessionVisitor interface.
The
> parameter is named |writer| in that interface. If we think |blocked| is a
better
> name, we should rename the parameter throughout.

I called it |blocked| in order to disambiguate it from |writer_|.

[ramant (doing other things), 2014-06-27 19:18:40]

PTAL.

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
File net/tools/quic/quic_dispatcher.cc (right):

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
net/tools/quic/quic_dispatcher.cc:351: // Return without adding the session to
the blocked list, to avoid infinite
On 2014/06/27 17:08:01, wtc wrote:
> 
> I believe |blocked| is a QuicConnection, not a QuicSession. If so, the comment
> should say "connection" or "blocked writer" instead of "session".

Done.

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
File net/tools/quic/quic_dispatcher.h (right):

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
net/tools/quic/quic_dispatcher.h:100: virtual void
OnWriteBlocked(QuicBlockedWriterInterface* blocked) OVERRIDE;
On 2014/06/27 17:08:01, wtc wrote:
> 
> Nit: This virtual method comes from the QuicServerSessionVisitor interface.
The
> parameter is named |writer| in that interface. If we think |blocked| is a
better
> name, we should rename the parameter throughout.

Changed it blocked_writer everywhere after talking with dmziegler.

Done.

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/quic_dispatch...
net/tools/quic/quic_dispatcher.h:100: virtual void
OnWriteBlocked(QuicBlockedWriterInterface* blocked) OVERRIDE;
On 2014/06/27 17:26:42, dmziegler wrote:
> On 2014/06/27 17:08:01, wtc wrote:
> > 
> > Nit: This virtual method comes from the QuicServerSessionVisitor interface.
> The
> > parameter is named |writer| in that interface. If we think |blocked| is a
> better
> > name, we should rename the parameter throughout.
> 
> I called it |blocked| in order to disambiguate it from |writer_|.

Acknowledged.

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/test_tools/qu...
File net/tools/quic/test_tools/quic_dispatcher_peer.cc (right):

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/test_tools/qu...
net/tools/quic/test_tools/quic_dispatcher_peer.cc:56: 
On 2014/06/27 17:08:01, wtc wrote:
> 
> Nit: delete one blank line.

Done.

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/test_tools/qu...
File net/tools/quic/test_tools/quic_dispatcher_peer.h (right):

https://codereview.chromium.org/359653003/diff/1/net/tools/quic/test_tools/qu...
net/tools/quic/test_tools/quic_dispatcher_peer.h:39: static
QuicDispatcher::WriteBlockedList* GetWriteBlockedList(
On 2014/06/27 17:08:01, wtc wrote:
> 
> We can further lock down insertions to QuicDispatcher::write_blocked_list_ by
> making this getter method return a const QuicDispatcher::WriteBlockedList*.
But
> quic_dispatcher_peer.h needs to erase connections from
> QuicDispatcher::write_blocked_list_, so we can't do this.

Acknowledged.

[wtc, 2014-06-30 23:48:05]

Patch set 2 LGTM. Thanks!