Add MessagePipe interface. Use it in ChannelDispatcherBase.

Add MessagePipe interface. Use it in ChannelDispatcherBase.

In WebRTC data streams are message-based, so the message framing
mechanism use for the old protocol doesn't make sense for the
WebRTC-based protocol. The new MessagePipe represents an abstract
messaging channel. StreamMessagePipeAdapter implements framing for the
old protocol. A different MessagePipe is going to be used for
WebRTC-based protocol.

BUG=547158
Committed: https://crrev.com/f1005f6e964841763ae3792ff6aea399f9514a67
Cr-Commit-Position: refs/heads/master@{#373340}

[Sergey Ulanov, 2016-01-30 01:24:48]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2016-02-01 22:48:20]

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/channel_d...
File remoting/protocol/channel_dispatcher_base.h (right):

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:52: bool is_connected() { return
message_pipe() != nullptr; }
message_pipe() can never return null with this implementation. I think you
probably want an is_initialized() method on 
StreamMessagePipeAdapter and call that here (or track initialization of this
class explicitly).

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/stream_me...
File remoting/protocol/stream_message_pipe_adapter.cc (right):

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/stream_me...
remoting/protocol/stream_message_pipe_adapter.cc:44: return;
Call the error callback here? Otherwise the caller has no way of knowing that
done() will never be invoked. Alternatively, add a bool return if you don't want
to change the semantics of the error callback.

[Sergey Ulanov, 2016-02-02 21:00:46]

Patchset #2 (id:20001) has been deleted

[Sergey Ulanov, 2016-02-02 21:15:40]

Patchset #2 (id:40001) has been deleted

[Sergey Ulanov, 2016-02-02 21:15:53]

Patchset #2 (id:60001) has been deleted

[Sergey Ulanov, 2016-02-02 21:16:42]

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/channel_d...
File remoting/protocol/channel_dispatcher_base.h (right):

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/channel_d...
remoting/protocol/channel_dispatcher_base.h:52: bool is_connected() { return
message_pipe() != nullptr; }
On 2016/02/01 22:48:20, Jamie wrote:
> message_pipe() can never return null with this implementation. I think you
> probably want an is_initialized() method on 
> StreamMessagePipeAdapter and call that here (or track initialization of this
> class explicitly).

Replaced message_pipe_ with a scoped_ptr.

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/stream_me...
File remoting/protocol/stream_message_pipe_adapter.cc (right):

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/stream_me...
remoting/protocol/stream_message_pipe_adapter.cc:44: return;
On 2016/02/01 22:48:20, Jamie wrote:
> Call the error callback here? Otherwise the caller has no way of knowing that
> done() will never be invoked. Alternatively, add a bool return if you don't
want
> to change the semantics of the error callback.

I think just dropping the callback here is the right thing to do because the
message was never sent. Send() could return result, but it's not very useful as
the callers have nothing to do with the result. Normally once a channel gets to
the error state the connection will be torn down. In my next CL I'm going to
remove ChannelDispatcherBase::EventHandler::OnChannelError(), so all channel
errors will be handled using IceTransport::EventHandler::OnIceTransportError().
It doesn't make sense to have both as they both used to handle transport-level
errors.

[Sergey Ulanov, 2016-02-02 21:22:48]

https://codereview.chromium.org/1649063003/diff/100001/remoting/protocol/webr...
File remoting/protocol/webrtc_connection_to_client.cc (right):

https://codereview.chromium.org/1649063003/diff/100001/remoting/protocol/webr...
remoting/protocol/webrtc_connection_to_client.cc:171: if (control_dispatcher_ &&
control_dispatcher_->is_connected() &&
This change needs to be in this CL because it fixes an issue that crashes the
host with this change.

[Jamie, 2016-02-02 22:43:10]

lgtm

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/stream_me...
File remoting/protocol/stream_message_pipe_adapter.cc (right):

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/stream_me...
remoting/protocol/stream_message_pipe_adapter.cc:44: return;
On 2016/02/02 21:16:42, Sergey Ulanov wrote:
> On 2016/02/01 22:48:20, Jamie wrote:
> > Call the error callback here? Otherwise the caller has no way of knowing
that
> > done() will never be invoked. Alternatively, add a bool return if you don't
> want
> > to change the semantics of the error callback.
> 
> I think just dropping the callback here is the right thing to do because the
> message was never sent. Send() could return result, but it's not very useful
as
> the callers have nothing to do with the result. Normally once a channel gets
to
> the error state the connection will be torn down. In my next CL I'm going to
> remove ChannelDispatcherBase::EventHandler::OnChannelError(), so all channel
> errors will be handled using
IceTransport::EventHandler::OnIceTransportError().
> It doesn't make sense to have both as they both used to handle transport-level
> errors.

Based on off-line discussion, it sounds like there are other, asynchronous,
situations that could result in the callback not being invoked. That being the
case, I agree that returning a bool is not the right thing to do. Can you update
the comment on MessagePipe::Send to make it clear that the callback will not be
invoked if the message is never sent?

https://codereview.chromium.org/1649063003/diff/100001/remoting/protocol/chan...
File remoting/protocol/channel_dispatcher_base.h (right):

https://codereview.chromium.org/1649063003/diff/100001/remoting/protocol/chan...
remoting/protocol/channel_dispatcher_base.h:56: bool is_connected() { return
!!message_pipe(); }
I think the previous "!= nullptr" was clearer.

[Sergey Ulanov, 2016-02-03 18:45:27]

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/stream_me...
File remoting/protocol/stream_message_pipe_adapter.cc (right):

https://codereview.chromium.org/1649063003/diff/1/remoting/protocol/stream_me...
remoting/protocol/stream_message_pipe_adapter.cc:44: return;
On 2016/02/02 22:43:10, Jamie wrote:
> On 2016/02/02 21:16:42, Sergey Ulanov wrote:
> > On 2016/02/01 22:48:20, Jamie wrote:
> > > Call the error callback here? Otherwise the caller has no way of knowing
> that
> > > done() will never be invoked. Alternatively, add a bool return if you
don't
> > want
> > > to change the semantics of the error callback.
> > 
> > I think just dropping the callback here is the right thing to do because the
> > message was never sent. Send() could return result, but it's not very useful
> as
> > the callers have nothing to do with the result. Normally once a channel gets
> to
> > the error state the connection will be torn down. In my next CL I'm going to
> > remove ChannelDispatcherBase::EventHandler::OnChannelError(), so all channel
> > errors will be handled using
> IceTransport::EventHandler::OnIceTransportError().
> > It doesn't make sense to have both as they both used to handle
transport-level
> > errors.
> 
> Based on off-line discussion, it sounds like there are other, asynchronous,
> situations that could result in the callback not being invoked. That being the
> case, I agree that returning a bool is not the right thing to do. Can you
update
> the comment on MessagePipe::Send to make it clear that the callback will not
be
> invoked if the message is never sent?

Done.

https://codereview.chromium.org/1649063003/diff/100001/remoting/protocol/chan...
File remoting/protocol/channel_dispatcher_base.h (right):

https://codereview.chromium.org/1649063003/diff/100001/remoting/protocol/chan...
remoting/protocol/channel_dispatcher_base.h:56: bool is_connected() { return
!!message_pipe(); }
On 2016/02/02 22:43:10, Jamie wrote:
> I think the previous "!= nullptr" was clearer.

Done.

[Sergey Ulanov, 2016-02-03 18:45:32]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-02-03 18:45:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1649063003/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-02-03 18:47:40]

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

[commit-bot: I haz the power, 2016-02-03 19:22:16]

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

[commit-bot: I haz the power, 2016-02-03 19:22:17]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Sergey Ulanov, 2016-02-03 20:09:48]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-02-03 20:09:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1649063003/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-02-03 20:11:46]

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

[commit-bot: I haz the power, 2016-02-03 21:11:38]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-02-03 21:13:41]

Description was changed from

==========
Add MessagePipe interface. Use it in ChannelDispatcherBase.

In WebRTC data streams are message-based, so the message framing
mechanism use for the old protocol doesn't make sense for the
WebRTC-based protocol. The new MessagePipe represents an abstract
messaging channel. StreamMessagePipeAdapter implements framing for the
old protocol. A different MessagePipe is going to be used for
WebRTC-based protocol.

BUG=547158
==========

to

==========
Add MessagePipe interface. Use it in ChannelDispatcherBase.

In WebRTC data streams are message-based, so the message framing
mechanism use for the old protocol doesn't make sense for the
WebRTC-based protocol. The new MessagePipe represents an abstract
messaging channel. StreamMessagePipeAdapter implements framing for the
old protocol. A different MessagePipe is going to be used for
WebRTC-based protocol.

BUG=547158

Committed: https://crrev.com/f1005f6e964841763ae3792ff6aea399f9514a67
Cr-Commit-Position: refs/heads/master@{#373340}
==========

[commit-bot: I haz the power, 2016-02-03 21:13:42]

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