Implement ChannelMultiplexer.

Implement ChannelMultiplexer.

ChannelMultiplexer allows multiple logical channels to share a 
single underlying transport channel.

BUG=137135
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150484

[Wez, 2012-08-03 21:28:27]

Can you update the description to be clearer, please? e.g. "ChannelMultiplexer
allows multiple logical channels to share a single underlying transport channel"

[Sergey Ulanov, 2012-08-05 03:07:24]

On 2012/08/03 21:28:27, Wez wrote:
> Can you update the description to be clearer, please? e.g. "ChannelMultiplexer
> allows multiple logical channels to share a single underlying transport
channel"

Done

[Wez, 2012-08-06 23:14:07]

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto
File remoting/proto/mux.proto (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto#ne...
remoting/proto/mux.proto:13: message MuxPacket {
nit: MultiplexPacket

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto#ne...
remoting/proto/mux.proto:21: // of both values.
nit: I don't think you need the example here; it's fine just to say that every
MuxPacket includes the channel identifier, and that at least the first MuxPacket
for a given channel specifies the name, so that the receiver can build a mapping
of channel Ids.

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto#ne...
remoting/proto/mux.proto:21: // of both values.
nit: The downside to this approach is that receiver code can't switch() on the
channel ID in received MuxPackets, since the IDs are allocated by the sender,
not the receiver.

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto#ne...
remoting/proto/mux.proto:30: optional bytes data = 3;
What is the overhead to copying the actual message data into the MuxPacket
rather than having MuxPacket be a header, and indicating how many bytes of
message data follow?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_f...
File remoting/protocol/channel_factory.h (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_f...
remoting/protocol/channel_factory.h:20: class ChannelFactory : public
base::NonThreadSafe {
nit: Why is NonThreadSafe part of this interface?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_f...
remoting/protocol/channel_factory.h:30: virtual ~ChannelFactory() {}
Is the caller really responsible for the lifetime of ChannelFactory, or should
the dtor be private?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_f...
remoting/protocol/channel_factory.h:37: // AUTHENTICATED states.
nit: This comment refers to sessions and session states, which this interface
has no dependence on; presumably the rule is simply that the channels must be
destroyed before the factory, and they implicitly can't be created before it
exists?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_f...
remoting/protocol/channel_factory.h:37: // AUTHENTICATED states.
nit: Clarify that it's acceptable for implementations to succeed or fail
synchronously, invoking the callback before the call returns.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.cc (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:71: int id() { return id_; }
nit: local_id?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:72: int remote_id() { return
remote_id_; }
nit: Or received_id() and send_id()?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:245: socket_ = NULL;
nit: Remove the MuxChannel from the list of active channels at this point?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:269:
pending_packets_.front()->is_empty()) {
Do you need a while loop here? Under what circumstances will you ever have
multiple empty packets pending? You never push zero-size packets onto the
pending queue.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:285:
ChannelMultiplexer::MuxSocket::~MuxSocket() {
Thread check before OnSocketDestroyed()?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:319:
&ChannelMultiplexer::MuxSocket::OnWriteComplete, AsWeakPtr()));
nit: It's a little confusing that we set write_pending_ before DoWrite() and
then check it afterwards; it can only change if OnWriteComplete() is called-back
synchronously. Would it make sense to make the BufferedWriter's write semantics
match net::StreamSocket::Write() semantics more closely?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:344: cb.Run(net::ERR_FAILED);
Don't you need to do this after setting write_failed_, so that you won't
use-after-free if the caller tears down the MuxSocket in response to the
failure?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:346: write_failed_ = true;
nit: Do you actually need to keep a write_failed flag, or will/should
BufferedWriter guarantee to fail all subsequent writes?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:445: it->second->OnWriteFailed();
This triggers caller-supplied callbacks, which might tear things down; we should
clarify what it is safe to tear down in a write-failed callback, and what it
isn't.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:471: channel =
GetOrCreateChannel(packet->channel_name());
This has the disadvantage that if a peer sends lots of data for a channel name
we don't recognize then we'll create a MuxChannel and store the data, but
nothing will ever read it.

It's related to the more general problem with lack of flow control in the
multiplexer; if we multiplexed at the datagram layer then we could just drop
packets and let a higher-level transport protocol recover on a per-channel
basis, but since we're multiplexing at the stream level we ideally want
per-channel flow control to notify senders when we can't receive any more data
for a particular channel.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:480: const base::Closure& done_task) {
nit: Indentation.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.h (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:20: // |factory| is used to create
underlying sockets.
nit: You mean it's used to create the channel(s) upon which to multiplex?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:21: ChannelMultiplexer(ChannelFactory*
factory);
nit: Allow the multiplex channel name (or channel name suffix) to be supplied by
the caller, so that multiple multiplers can run over a single underlying
ChannelFactory, e.g. one multiplexing input & clipboard, one multiplexing audio
& video, etc.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:24: virtual void CreateStreamChannel(
nit: Precede with comment "ChannelFactory interface."

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:43: // Called by MuxChannel.
nit: Why is there a comment on this method, but not on the others?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:46: // The underlying socket.
nit: The channel over which to multiplex?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:56: std::map<std::string, MuxChannel*>
channels_;
nit: channels_by_name_

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:57: std::map<int, MuxChannel*>
channels_by_remote_id_;
nit: channels_by_id_

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:57: std::map<int, MuxChannel*>
channels_by_remote_id_;
nit: Why map name->channel and id->channel rather than name->id and id->channel,
which would make it clearer which structure "owns" the channel?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/session.h...
remoting/protocol/session.h:88: virtual void CancelChannelCreation(const
std::string& name) = 0;
Why does this not belong on ChannelFactory?

[Sergey Ulanov, 2012-08-07 20:12:22]

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto
File remoting/proto/mux.proto (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto#ne...
remoting/proto/mux.proto:13: message MuxPacket {
On 2012/08/06 23:14:07, Wez wrote:
> nit: MultiplexPacket

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto#ne...
remoting/proto/mux.proto:21: // of both values.
On 2012/08/06 23:14:07, Wez wrote:
> nit: I don't think you need the example here; it's fine just to say that every
> MuxPacket includes the channel identifier, and that at least the first
MuxPacket
> for a given channel specifies the name, so that the receiver can build a
mapping
> of channel Ids.

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto#ne...
remoting/proto/mux.proto:21: // of both values.
On 2012/08/06 23:14:07, Wez wrote:
> nit: The downside to this approach is that receiver code can't switch() on the
> channel ID in received MuxPackets, since the IDs are allocated by the sender,
> not the receiver.

It would make protocol more complicated if the IDs were assigned by the
receiver. Don't think it's worth worrying about.
We can assign fixed ID to each channel, but identifying channels by ID is not a
good approach.

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto#ne...
remoting/proto/mux.proto:30: optional bytes data = 3;
On 2012/08/06 23:14:07, Wez wrote:
> What is the overhead to copying the actual message data into the MuxPacket
> rather than having MuxPacket be a header, and indicating how many bytes of
> message data follow?

I don't think it's big enough to worry about since we don't intent to use this
channel to send very big messages. In any case, if it becomes a problem we can
hack protobuf message serialization/deserialization code such that it wouldn't
require copying that memory. And that would require changing on-wire format of
the messages.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_f...
File remoting/protocol/channel_factory.h (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_f...
remoting/protocol/channel_factory.h:20: class ChannelFactory : public
base::NonThreadSafe {
On 2012/08/06 23:14:07, Wez wrote:
> nit: Why is NonThreadSafe part of this interface?

To make it clear that this interface is not thread-safe - all methods should
always be called on a specific thread.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_f...
remoting/protocol/channel_factory.h:30: virtual ~ChannelFactory() {}
On 2012/08/06 23:14:07, Wez wrote:
> Is the caller really responsible for the lifetime of ChannelFactory, or should
> the dtor be private?

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_f...
remoting/protocol/channel_factory.h:37: // AUTHENTICATED states.
On 2012/08/06 23:14:07, Wez wrote:
> nit: This comment refers to sessions and session states, which this interface
> has no dependence on; presumably the rule is simply that the channels must be
> destroyed before the factory, and they implicitly can't be created before it
> exists?

Cleaned up that comment.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_f...
remoting/protocol/channel_factory.h:37: // AUTHENTICATED states.
On 2012/08/06 23:14:07, Wez wrote:
> nit: Clarify that it's acceptable for implementations to succeed or fail
> synchronously, invoking the callback before the call returns.

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.cc (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:71: int id() { return id_; }
On 2012/08/06 23:14:07, Wez wrote:
> nit: local_id?
renamed to send id

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:72: int remote_id() { return
remote_id_; }
On 2012/08/06 23:14:07, Wez wrote:
> nit: Or received_id() and send_id()?

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:245: socket_ = NULL;
On 2012/08/06 23:14:07, Wez wrote:
> nit: Remove the MuxChannel from the list of active channels at this point?

We need to keep this object in order to preserve name->id mapping. Otherwise it
would be hard to properly handle the case when the channel is destroyed and then
created again during a session. At least without making the protocol more
complicated.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:269:
pending_packets_.front()->is_empty()) {
On 2012/08/06 23:14:07, Wez wrote:
> Do you need a while loop here? Under what circumstances will you ever have
> multiple empty packets pending? You never push zero-size packets onto the
> pending queue.

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:285:
ChannelMultiplexer::MuxSocket::~MuxSocket() {
On 2012/08/06 23:14:07, Wez wrote:
> Thread check before OnSocketDestroyed()?

The class inherits from NonThreadSafe and ~NonThreadSafe checks that it runs on
the correct thread.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:319:
&ChannelMultiplexer::MuxSocket::OnWriteComplete, AsWeakPtr()));
On 2012/08/06 23:14:07, Wez wrote:
> nit: It's a little confusing that we set write_pending_ before DoWrite() and
> then check it afterwards; it can only change if OnWriteComplete() is
called-back
> synchronously. Would it make sense to make the BufferedWriter's write
semantics
> match net::StreamSocket::Write() semantics more closely?

The semantics used by BufferedSocketWriter is easier to use in most other case
as there is always only one place that handles the result. Don't think that
semantics mismatch here is a good reason to make buffered writer harder to use
in all other places.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:344: cb.Run(net::ERR_FAILED);
On 2012/08/06 23:14:07, Wez wrote:
> Don't you need to do this after setting write_failed_, so that you won't
> use-after-free if the caller tears down the MuxSocket in response to the
> failure?

Good point. Fixed.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:346: write_failed_ = true;
On 2012/08/06 23:14:07, Wez wrote:
> nit: Do you actually need to keep a write_failed flag, or will/should
> BufferedWriter guarantee to fail all subsequent writes?

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:445: it->second->OnWriteFailed();
On 2012/08/06 23:14:07, Wez wrote:
> This triggers caller-supplied callbacks, which might tear things down; we
should
> clarify what it is safe to tear down in a write-failed callback, and what it
> isn't.

Fixed this method to handle the case when multiplexer is destroyed from one of
the callbacks.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:471: channel =
GetOrCreateChannel(packet->channel_name());
On 2012/08/06 23:14:07, Wez wrote:
> This has the disadvantage that if a peer sends lots of data for a channel name
> we don't recognize then we'll create a MuxChannel and store the data, but
> nothing will ever read it.
> 
> It's related to the more general problem with lack of flow control in the
> multiplexer; if we multiplexed at the datagram layer then we could just drop
> packets and let a higher-level transport protocol recover on a per-channel
> basis, but since we're multiplexing at the stream level we ideally want
> per-channel flow control to notify senders when we can't receive any more data
> for a particular channel.

MessageReader doesn't try to read from the channel if there is any message that
has been read from the socket but haven't been processed. For multiplexer it
means that if one of the channels is not being read from then all other channels
will be blocked too. I.e. there is no per-channel flow control, but the reader
still throttles the stream if the receiving side can't keep up with the amount
data it receives.

It's not a perfect solution, but I don't think we need it to be perfect - it
will be thrown away when we switch to WebRTC/SCTP anyway.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:480: const base::Closure& done_task) {
On 2012/08/06 23:14:07, Wez wrote:
> nit: Indentation.

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.h (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:20: // |factory| is used to create
underlying sockets.
On 2012/08/06 23:14:07, Wez wrote:
> nit: You mean it's used to create the channel(s) upon which to multiplex?

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:21: ChannelMultiplexer(ChannelFactory*
factory);
On 2012/08/06 23:14:07, Wez wrote:
> nit: Allow the multiplex channel name (or channel name suffix) to be supplied
by
> the caller, so that multiple multiplers can run over a single underlying
> ChannelFactory, e.g. one multiplexing input & clipboard, one multiplexing
audio
> & video, etc.

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:24: virtual void CreateStreamChannel(
On 2012/08/06 23:14:07, Wez wrote:
> nit: Precede with comment "ChannelFactory interface."

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:43: // Called by MuxChannel.
On 2012/08/06 23:14:07, Wez wrote:
> nit: Why is there a comment on this method, but not on the others?

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:46: // The underlying socket.
On 2012/08/06 23:14:07, Wez wrote:
> nit: The channel over which to multiplex?

Done.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:56: std::map<std::string, MuxChannel*>
channels_;
On 2012/08/06 23:14:07, Wez wrote:
> nit: channels_by_name_

I think the current naming is better because it makes in clearer that channels_
contain all channels and "owns" them, while channels_by_remote_id_ is just an
auxiliary map

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:57: std::map<int, MuxChannel*>
channels_by_remote_id_;
On 2012/08/06 23:14:07, Wez wrote:
> nit: channels_by_id_

that would be confusing because each channel has two IDs and MuxChannel has id()
that is not what is used in this map.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:57: std::map<int, MuxChannel*>
channels_by_remote_id_;
On 2012/08/06 23:14:07, Wez wrote:
> nit: Why map name->channel and id->channel rather than name->id and
id->channel,
> which would make it clearer which structure "owns" the channel?

initially, when a channel is created, we don't know "receive id" for it - that
ID is assigned by the remote side. We could have id->name and name->channel maps
but that way we would have to do string map lookup on every incoming packet
which is slower.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/session.h...
remoting/protocol/session.h:88: virtual void CancelChannelCreation(const
std::string& name) = 0;
On 2012/08/06 23:14:07, Wez wrote:
> Why does this not belong on ChannelFactory?

Done.

[Sergey Ulanov, 2012-08-07 20:13:58]

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto
File remoting/proto/mux.proto (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/proto/mux.proto#ne...
remoting/proto/mux.proto:30: optional bytes data = 3;
On 2012/08/07 20:12:22, sergeyu wrote:
> On 2012/08/06 23:14:07, Wez wrote:
> > What is the overhead to copying the actual message data into the MuxPacket
> > rather than having MuxPacket be a header, and indicating how many bytes of
> > message data follow?
> 
> I don't think it's big enough to worry about since we don't intent to use this
> channel to send very big messages. In any case, if it becomes a problem we can
> hack protobuf message serialization/deserialization code such that it wouldn't
> require copying that memory. And that would require changing on-wire format of
> the messages. 
I meant it would _not_ require changing on-wire format of the message.

[Wez, 2012-08-07 21:44:43]

LGTM w/ nits.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.cc (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:285:
ChannelMultiplexer::MuxSocket::~MuxSocket() {
On 2012/08/07 20:12:22, sergeyu wrote:
> On 2012/08/06 23:14:07, Wez wrote:
> > Thread check before OnSocketDestroyed()?
> 
> The class inherits from NonThreadSafe and ~NonThreadSafe checks that it runs
on
> the correct thread.

Not until after OnSocketDestroyed() has already been called, though - the base
class dtor runs after the dtor of this class.

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.h (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:57: std::map<int, MuxChannel*>
channels_by_remote_id_;
On 2012/08/07 20:12:22, sergeyu wrote:
> On 2012/08/06 23:14:07, Wez wrote:
> > nit: channels_by_id_
> 
> that would be confusing because each channel has two IDs and MuxChannel has
id()
> that is not what is used in this map.

You've renamed the id in MuxChannel to receive_id, but in this may you've called
it remote_id?

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:57: std::map<int, MuxChannel*>
channels_by_remote_id_;
On 2012/08/07 20:12:22, sergeyu wrote:
> On 2012/08/06 23:14:07, Wez wrote:
> > nit: Why map name->channel and id->channel rather than name->id and
> id->channel,
> > which would make it clearer which structure "owns" the channel?
> 
> initially, when a channel is created, we don't know "receive id" for it - that
> ID is assigned by the remote side. We could have id->name and name->channel
maps
> but that way we would have to do string map lookup on every incoming packet
> which is slower.

I see what you mean. I'm beginning to wonder whether the ID optimization is
worth the additional complexity...

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.cc (right):

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:417: NOTREACHED();
Do you really want a NOTREACHED() here; it seems valid for a caller to play it
safe by calling CancelChannelCreation() for all channels they might ever have
tried to create, without caring whether they already succeeded.

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer_unittest.cc (right):

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer_unittest.cc:225: EXPECT_CALL(cb2,
OnDone(_))
nit: This expectation should really appear alongside the cb1 expectation, before
any mock methods actually get invoked.

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer_unittest.cc:253: EXPECT_CALL(cb2,
OnDone(net::ERR_FAILED));
Same here.

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer_unittest.cc:284:
.WillOnce(InvokeWithoutArgs(this, &ChannelMultiplexerTest::DeleteAll));
And here.

[Sergey Ulanov, 2012-08-07 22:25:14]

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.h (right):

http://codereview.chromium.org/10830046/diff/3001/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.h:57: std::map<int, MuxChannel*>
channels_by_remote_id_;
On 2012/08/07 21:44:44, Wez wrote:
> On 2012/08/07 20:12:22, sergeyu wrote:
> > On 2012/08/06 23:14:07, Wez wrote:
> > > nit: channels_by_id_
> > 
> > that would be confusing because each channel has two IDs and MuxChannel has
> id()
> > that is not what is used in this map.
> 
> You've renamed the id in MuxChannel to receive_id, but in this may you've
called
> it remote_id?

I renamed it to channels_by_receive_id_

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer.cc (right):

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer.cc:417: NOTREACHED();
On 2012/08/07 21:44:45, Wez wrote:
> Do you really want a NOTREACHED() here; it seems valid for a caller to play it
> safe by calling CancelChannelCreation() for all channels they might ever have
> tried to create, without caring whether they already succeeded.
Yes, you are right.

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
File remoting/protocol/channel_multiplexer_unittest.cc (right):

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer_unittest.cc:225: EXPECT_CALL(cb2,
OnDone(_))
On 2012/08/07 21:44:45, Wez wrote:
> nit: This expectation should really appear alongside the cb1 expectation,
before
> any mock methods actually get invoked.

Done.

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer_unittest.cc:253: EXPECT_CALL(cb2,
OnDone(net::ERR_FAILED));
On 2012/08/07 21:44:45, Wez wrote:
> Same here.

Done.

http://codereview.chromium.org/10830046/diff/1013/remoting/protocol/channel_m...
remoting/protocol/channel_multiplexer_unittest.cc:284:
.WillOnce(InvokeWithoutArgs(this, &ChannelMultiplexerTest::DeleteAll));
On 2012/08/07 21:44:45, Wez wrote:
> And here.

Done.

[commit-bot: I haz the power, 2012-08-07 22:25:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10830046/1013

[commit-bot: I haz the power, 2012-08-08 01:04:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10830046/12002

[commit-bot: I haz the power, 2012-08-08 02:03:12]

Change committed as 150484