Issue 2907073003: [Chromoting] Add DataChannelManager to manage optional incoming data channels

Hzj_jie

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

3 years, 6 months ago (2017-05-29 01:12:10 UTC) #1

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-29 01:12:18 UTC) #2

Hzj_jie

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

3 years, 6 months ago (2017-05-29 01:30:52 UTC) #3

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-29 01:31:06 UTC) #4

Hzj_jie

Description was changed from ========== [Chromoting] Add DataChannelManager to manage optional incoming data channels DataChannelManager ...

3 years, 6 months ago (2017-05-29 02:07:07 UTC) #5

Hzj_jie

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

3 years, 6 months ago (2017-05-29 02:07:07 UTC) #6

Hzj_jie

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

3 years, 6 months ago (2017-05-29 02:07:58 UTC) #7

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-29 02:08:04 UTC) #8

Hzj_jie

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

3 years, 6 months ago (2017-05-29 02:11:50 UTC) #9

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-29 02:12:08 UTC) #10

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-29 02:51:13 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/394888)

3 years, 6 months ago (2017-05-29 02:51:14 UTC) #12

Hzj_jie

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

3 years, 6 months ago (2017-05-29 17:50:29 UTC) #13

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-29 17:50:39 UTC) #16

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-29 19:01:36 UTC) #20

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-05-29 19:01:37 UTC) #21

joedow

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

3 years, 6 months ago (2017-05-30 16:24:16 UTC) #22

joedow

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc File remoting/protocol/data_channel_handler.cc (right): https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc#newcode25 remoting/protocol/data_channel_handler.cc:25: DataChannelHandler::~DataChannelHandler() = default; Don't you want to call Close() ...

3 years, 6 months ago (2017-05-30 16:24:19 UTC) #23

Hzj_jie

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

3 years, 6 months ago (2017-05-30 19:04:31 UTC) #24

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-30 19:05:00 UTC) #25

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-30 19:26:49 UTC) #26

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xcode-clang/builds/110396)

3 years, 6 months ago (2017-05-30 19:26:50 UTC) #27

Hzj_jie

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

3 years, 6 months ago (2017-05-30 22:30:16 UTC) #29

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-30 22:31:02 UTC) #30

Hzj_jie

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc File remoting/protocol/data_channel_handler.cc (right): https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc#newcode25 remoting/protocol/data_channel_handler.cc:25: DataChannelHandler::~DataChannelHandler() = default; On 2017/05/30 16:24:17, joedow wrote: > ...

3 years, 6 months ago (2017-05-31 00:11:54 UTC) #31

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_handler.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:25:
DataChannelHandler::~DataChannelHandler() = default;
On 2017/05/30 16:24:17, joedow wrote:
> Don't you want to call Close() in the d'tor?

No, instead, destructor is called by Close() function. So a derived class would
either actively call Close() or rely on the MessagePipe to be closed.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:30: OnDisconnected();
On 2017/05/30 16:24:17, joedow wrote:
> Should this be called 'OnDisconnecting()'?  You are notifiying listeners of
the
> disconnect but the connection hasn't been disconnected yet (that happens on
line
> 34).

Since this is the only callback regarding to the disconnection, I have not
opinion on Disconnecting or Disconnected.
Updated.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:35:
base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
On 2017/05/30 16:24:17, joedow wrote:
> This feels weird, I'm not sure why the owner should control the handler's
> lifetime.

Because the handler should be deleted once the MessagePipe is closed, so
ensuring the consistent lifetime of DataChannelHandler and MessagePipe would be
the safest way to avoid any unexpected referring of destructed objects.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:42: if (is_connected_) {
On 2017/05/30 16:24:17, joedow wrote:
> You use your helper method above (connected()), it would be good to be
> consistent and use that here as well.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:51: std::unique_ptr<CompoundBuffer>
message) {}
On 2017/05/30 16:24:17, joedow wrote:
> Do these need to be implemented?  If they are optional, I think going with an
> observer pattern is better.  If they are required to implement the virtual
> methods, then you should add 'NOTIMPLEMENTED()' here to let them know they
need
> to override the methods.

Observer pattern seems not fitting this class: the DataChannelHandler implements
receiving the incoming message instead of observing them.

Using sending process statistic as an example: it does not expect to receive any
data from OnIncomingMessage(), but only uses Send() to send the data to the
peer, so it can simply ignore the override of this function.

If you have strong opinion, I can make this function as pure virtual, but I do
not think this is really necessary.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:59: is_connected_ = true;
On 2017/05/30 16:24:17, joedow wrote:
> DCHECK(!is_connected); to protect against multiple open calls?  It shouldn't
> happen but would cause odd problems if it did.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_handler.h (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:36: // should not be performed after
executing this function.
On 2017/05/30 16:24:18, joedow wrote:
> The last sentence could be more clear:
> "No operations should be performed after executing this function."

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:43: bool connected() const { return
is_connected_; }
On 2017/05/30 16:24:17, joedow wrote:
> How are 'connected()' and 'closed()' different?  This might be confusing for
> callers as I think your intention is that a closed pipe could be destroyed
soon
> but a caller could see that and try to connect again.
> 
> Can you use one connection state and then use DCHECKS to ensure the data
channel
> isn't re-connected as you try to delete it?  Another option would be to not
> delete it and allow the owner to Close/Destroy it as needed.  I'm not sure why
> this object needs to destroy itself.

I may rename "closed()" to "finialized()" to avoid any confusion.
The goal of this function is to ensure the derived classes have a way to track
the lifetime of itself even the pipe has never been opened.
Say an implementation uses a timer, it needs to stop the timer once the "this"
will be deleted later. This usually can be done by OnDisconnecting(). But once
the pipe has not been connected at all, the OnDisconnecting() function won't be
called.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:45: // Returns false before the channel
is connected.
On 2017/05/30 16:24:17, joedow wrote:
> I'd make this comment more generic, "Returns false if the message is not
sent".

Updated, but I still prefer to explicitly mention the reason of failure.
Otherwise the caller may not know what should be done once the function returns
false. i.e.
==========================
if (connected()) {
  bool r = Send();
  DCHECK(r);
}
==========================
should be perfectly fine.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:46: bool
Send(google::protobuf::MessageLite* message, const base::Closure& done);
On 2017/05/30 16:24:17, joedow wrote:
> Why is message a pointer and not a const ref, are you planning on changing it?

Because MessagePipe::Send() receives a pointer. It's questionable to use pointer
in MessagePipe::Send(): it may be helpful to merge the message with others,
though it's not likely to happen. But to avoid a copy, I need to use pointer
here.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:52: // connection or observe the
connection state.
On 2017/05/30 16:24:17, joedow wrote:
> Why use inheritance here instead of an observer pattern (which is more common
in
> our codebase)?

A derived class of DataChannelHandler handles data instead of observing data, a
typical implementation should both receive and send data. Observer pattern seems
not match the requirement of this class.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.cc:22: HandlerConstructor constructor) {
On 2017/05/30 16:24:18, joedow wrote:
> I think factory is a better term than constructor since you aren't registering
a
> constructor but rather a function that returns an instance of a handler.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.cc:27:
constructors_.push_back(std::make_pair(regex, constructor));
On 2017/05/30 16:24:18, joedow wrote:
> Do you want to check for double-registration (i.e. if two callers register the
> same regex, only one will get called, the other will not).
> 
> Perhaps using a Dictionary would be good here to check for message handler
> existence to prevent this type of problem.

Yes, I considered this issue before, but since regex is using here and the pipe
name is coming from the peer, we cannot guarantee no pipe name can match two or
more regexes.
i.e. both ".*" and ".+" match an unempty string.
I can add a DEBUG section in OnIncomingDataChannel to ensure only one match
happens.

Using prefix matching reduces the flexibility, so I prefer regex here.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.cc:34: for (auto& constructor :
constructors_) {
On 2017/05/30 16:24:18, joedow wrote:
> Using a dictionary would be cleaner here.  You could retrieve the factory
> instance using the message-type portion of the name and then use the factory
> directly (instead of using the second member of a pair).

A dictionary cannot handle prefix matches.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager.h (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.h:22: // DataChannelHandler
implementation register a factory function to create an
On 2017/05/30 16:24:18, joedow wrote:
> s/register/registers

Done.

Also the comment does not reflect the latest change: lifetime of
DataChannelHandler instances are not managed by DataChannelManager.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.h:24: // DataChannelHandler instances are
mananged by DataChannelManager instance.
On 2017/05/30 16:24:18, joedow wrote:
> s/mananged/managed

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.h:26: // channel by calling
DataChannelHandler::Close() function.
On 2017/05/30 16:24:18, joedow wrote:
> re-write lines 25/26 -> All handlers are closed when the session ends. 
Clients
> can also close a data channel manually by calling the close method.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.h:29: using HandlerConstructor =
base::Callback<void(
On 2017/05/30 16:24:18, joedow wrote:
> maybe rename HandlerConstructor to DataChannelHandlerFactory?
> 
> The function passed in might not construct a new instance, it would be useful
to
> be able to provide a singleton as well.

Term "DataChannel" seems a little bit redundant, I have updated the name into
HandlerFactory.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.h:38: // returns false when |regex| or
|constructor| are empty.
On 2017/05/30 16:24:18, joedow wrote:
> fix the last sentence:
> Returns true if the registration succeeds.
> 
> This is simpler and requires no maintenance if the param names change.

Same as DataChannelHandler::Send(), I still prefer to explicitly explain the
failure cases.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.h:39: bool RegisterHandlerFactory(const
std::string& regex,
On 2017/05/30 16:24:18, joedow wrote:
> A regex isn't needed is it?  Data channels have a standard format
> ({prefix}-{unique_pipe_id}).  Can't we register a strict substring for the
> prefix (aka message type)?  I don't think we would want to register a handler
> based on the pipe_id suffix right?
> 
> I think it would be less error prone to allow registration for the
message_type
> and use a starts_with match.

I do not think there is any restriction of the naming of data channels.
IMO, using regex does not increase the complexity (most of the clients can still
use a pure string for a full match) and ensure the safety (for the prefix-id
case, prefix-\d+ should be safer to avoid unexpected matches to prefix-ABC or
prefix-.)

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.h:44: // |name|.
On 2017/05/30 16:24:18, joedow wrote:
> The last sentence is a bit awkward.  It is totally reasonable that no factory
is
> registered for a message (consider having a older host connecting to a newer
> client).
> 
> Reword to:
> Returns true if a handler exists for the new data channel.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager_unittest.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager_unittest.cc:36: void
OnIncomingMessage(std::unique_ptr<CompoundBuffer> message) override {
On 2017/05/30 16:24:18, joedow wrote:
> nit: I think it is cleaner to break the impls out from the class declaration
for
> test code.  It can be in the same file still but does improve readability.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager_unittest.cc:73: void
TestDataChannelManager(bool asynchronous) {
On 2017/05/30 16:24:18, joedow wrote:
> This is a *big* test method that does a lot of stuff.  It would be better to
set
> up specific tests for each feature you are testing (i.e.):
> - test partial match
> - test full match
> - test full / partial match
> - test duplicate regex
> 
> etc.  This would make the tests more maintainable and easier to
fix/understand.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager_unittest.cc:120: ProcessResourceUsage
usage;
On 2017/05/30 16:24:18, joedow wrote:
> Can you avoid bringing in external dependencies into the test?  Can we just
> define a test message type at the top of this file and use that instead?

This is not doable since proto is automatically generated from .proto file. I
will add a new proto to avoid the dependency.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/fake_...
File remoting/protocol/fake_message_pipe.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_message_pipe.cc:28: ASSERT_TRUE(event_handler_ ==
nullptr);
On 2017/05/30 16:24:18, joedow wrote:
> You should avoid adding ASSERT statements in classes like this and opt for
using
> them inside the test itself.  Pushing asserts into the classes can make
> debugging tests more difficult and logs less readable.

But this class itself is for testing purpose only.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_message_pipe.cc:46: if (asynchronous_) {
On 2017/05/30 16:24:18, joedow wrote:
> Can you move the duplicated logic (sync vs. async) into a separate method.  It
> is very easy to update the logic for one and not the other given how this is
> laid out (same with the other methods below).

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/fake_...
File remoting/protocol/fake_message_pipe.h (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_message_pipe.h:25: // Sets |asynchronous| to true to make
all operations asynchronizedly.
On 2017/05/30 16:24:18, joedow wrote:
> I don't think this comment is needed.  The param name is self-explanatory.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_message_pipe.h:31:
std::unique_ptr<FakeMessagePipeWrapper> Wrap();
On 2017/05/30 16:24:18, joedow wrote:
> Why is this needed?  Someone who has access to this instance already knows its
> address and can use it as needed.  Also, returning a std_unique_ptr<> implies
> transferring ownership, but this method can be called multiple times which
means
> double freeing is a concern.

It returns an FakeMessagePipeWrapper which forwards all the operations to the
original FakeMessagePipe instance. It's safe to call this function multiple
times because no freeing happens to this instance. The reason of
FakeMessagePipeWrapper is because DataChannelHandler takes ownership of
MessagePipe.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_message_pipe.h:38: // Triggers
EventHandler::OnMessageReceived().
On 2017/05/30 16:24:18, joedow wrote:
> I would remove the impl detail comments since they can change and it is easy
to
> forget to update the header comments since they are divorced from the impl
> itself.  Instead write comments based on what the function's responsibilities
> are.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/fake_...
File remoting/protocol/fake_message_pipe_wrapper.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_message_pipe_wrapper.cc:16: : pipe_(pipe) {}
On 2017/05/30 16:24:18, joedow wrote:
> DCHECK(pipe_);
> 
> If there is a bug I think that would make it easier to track down later.

Done.

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-31 02:40:17 UTC) #32

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/382227)

3 years, 6 months ago (2017-05-31 02:40:18 UTC) #33

Sergey Ulanov

Haven't looked through the whole CL yet. Couple high-level comments about DataChannelHandler https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc File remoting/protocol/data_channel_handler.cc ...

3 years, 6 months ago (2017-05-31 19:49:03 UTC) #34

Haven't looked through the whole CL yet. Couple high-level comments about
DataChannelHandler

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_handler.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:25:
DataChannelHandler::~DataChannelHandler() = default;
On 2017/05/31 00:11:51, Hzj_jie wrote:
> On 2017/05/30 16:24:17, joedow wrote:
> > Don't you want to call Close() in the d'tor?
> 
> No, instead, destructor is called by Close() function. So a derived class
would
> either actively call Close() or rely on the MessagePipe to be closed.

This is a rather bug-prone approach, we should be careful when using it.
Particularly if an object owns can delete itself, then it shouldn't have any
public methods, see my next comment

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:35:
base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
On 2017/05/31 00:11:51, Hzj_jie wrote:
> On 2017/05/30 16:24:17, joedow wrote:
> > This feels weird, I'm not sure why the owner should control the handler's
> > lifetime.
> 
> Because the handler should be deleted once the MessagePipe is closed, so
> ensuring the consistent lifetime of DataChannelHandler and MessagePipe would
be
> the safest way to avoid any unexpected referring of destructed objects.

In order to send outgoing messages some other class will need keep a pointer to
DataChannelHandler. How would that class be notified when the channel is closed?
If this class is intended to be a base class for channel handlers that call
Send(), then Send() should be protected instead of public.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_handler.h (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:46: bool
Send(google::protobuf::MessageLite* message, const base::Closure& done);
On 2017/05/31 00:11:52, Hzj_jie wrote:
> On 2017/05/30 16:24:17, joedow wrote:
> > Why is message a pointer and not a const ref, are you planning on changing
it?
> 
> Because MessagePipe::Send() receives a pointer. It's questionable to use
pointer
> in MessagePipe::Send(): it may be helpful to merge the message with others,
> though it's not likely to happen. But to avoid a copy, I need to use pointer
> here.

Potentially MessagePipe::Send() can be changed to use const-ref.
MessageLite is an interface, and interfaces are normally passed by pointer.
AFAIR that's the reason it's currently passed by pointer. In most cases
const-ref args can be copied, but MessageLite can't.

Hzj_jie

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

3 years, 6 months ago (2017-05-31 20:49:38 UTC) #35

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-31 20:50:43 UTC) #36

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-31 22:25:17 UTC) #37

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/383012)

3 years, 6 months ago (2017-05-31 22:25:18 UTC) #38

Hzj_jie

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

3 years, 6 months ago (2017-05-31 22:32:03 UTC) #39

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-31 22:32:57 UTC) #40

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-31 23:12:41 UTC) #41

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/397320)

3 years, 6 months ago (2017-05-31 23:12:41 UTC) #42

Hzj_jie

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

3 years, 6 months ago (2017-05-31 23:36:49 UTC) #43

commit-bot: I haz the power

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

3 years, 6 months ago (2017-05-31 23:38:01 UTC) #44

Hzj_jie

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

3 years, 6 months ago (2017-06-01 01:06:27 UTC) #46

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-01 01:07:07 UTC) #49

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-01 04:12:49 UTC) #50

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-01 04:12:50 UTC) #51

Hzj_jie

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc File remoting/protocol/data_channel_handler.cc (right): https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc#newcode25 remoting/protocol/data_channel_handler.cc:25: DataChannelHandler::~DataChannelHandler() = default; On 2017/05/31 19:49:02, Sergey Ulanov wrote: ...

3 years, 6 months ago (2017-06-01 15:43:34 UTC) #52

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_handler.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:25:
DataChannelHandler::~DataChannelHandler() = default;
On 2017/05/31 19:49:02, Sergey Ulanov wrote:
> On 2017/05/31 00:11:51, Hzj_jie wrote:
> > On 2017/05/30 16:24:17, joedow wrote:
> > > Don't you want to call Close() in the d'tor?
> > 
> > No, instead, destructor is called by Close() function. So a derived class
> would
> > either actively call Close() or rely on the MessagePipe to be closed.
> 
> This is a rather bug-prone approach, we should be careful when using it.
> Particularly if an object owns can delete itself, then it shouldn't have any
> public methods, see my next comment
only
The public methods are only public for tests. I can move them into protected
section and public these methods in FakeDataChannelHandler.

The only concern I have is that the possibility of lacking OnMessagePipeClosed()
function call from MessagePipe. Our current implementations do not have this
issue, but it's still worthy to be called out.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:35:
base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
On 2017/05/31 19:49:02, Sergey Ulanov wrote:
> On 2017/05/31 00:11:51, Hzj_jie wrote:
> > On 2017/05/30 16:24:17, joedow wrote:
> > > This feels weird, I'm not sure why the owner should control the handler's
> > > lifetime.
> > 
> > Because the handler should be deleted once the MessagePipe is closed, so
> > ensuring the consistent lifetime of DataChannelHandler and MessagePipe would
> be
> > the safest way to avoid any unexpected referring of destructed objects.
> 
> In order to send outgoing messages some other class will need keep a pointer
to
> DataChannelHandler. How would that class be notified when the channel is
closed?
> If this class is intended to be a base class for channel handlers that call
> Send(), then Send() should be protected instead of public.

Yes, Send() is not intentionally used by any other object, but
DataChannelHandler itself.
IMO, a typical implementation of DataChannelHandler should handle everything
itself.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_handler.h (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:46: bool
Send(google::protobuf::MessageLite* message, const base::Closure& done);
On 2017/05/31 19:49:02, Sergey Ulanov wrote:
> On 2017/05/31 00:11:52, Hzj_jie wrote:
> > On 2017/05/30 16:24:17, joedow wrote:
> > > Why is message a pointer and not a const ref, are you planning on changing
> it?
> > 
> > Because MessagePipe::Send() receives a pointer. It's questionable to use
> pointer
> > in MessagePipe::Send(): it may be helpful to merge the message with others,
> > though it's not likely to happen. But to avoid a copy, I need to use pointer
> > here.
> 
> Potentially MessagePipe::Send() can be changed to use const-ref.
> MessageLite is an interface, and interfaces are normally passed by pointer.
> AFAIR that's the reason it's currently passed by pointer. In most cases
> const-ref args can be copied, but MessageLite can't.

MessageLite is not copyable anyway, so using const ref seems still reasonable. I
can try it in a different change.

joedow

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc File remoting/protocol/data_channel_handler.cc (right): https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc#newcode30 remoting/protocol/data_channel_handler.cc:30: OnDisconnected(); On 2017/05/31 00:11:51, Hzj_jie wrote: > On 2017/05/30 ...

3 years, 6 months ago (2017-06-01 17:25:26 UTC) #53

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_handler.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:30: OnDisconnected();
On 2017/05/31 00:11:51, Hzj_jie wrote:
> On 2017/05/30 16:24:17, joedow wrote:
> > Should this be called 'OnDisconnecting()'?  You are notifiying listeners of
> the
> > disconnect but the connection hasn't been disconnected yet (that happens on
> line
> > 34).
> 
> Since this is the only callback regarding to the disconnection, I have not
> opinion on Disconnecting or Disconnected.
> Updated.

It is important as they have very different meanings.  Thanks for changing it.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_handler.h (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:43: bool connected() const { return
is_connected_; }
On 2017/05/31 00:11:51, Hzj_jie wrote:
> On 2017/05/30 16:24:17, joedow wrote:
> > How are 'connected()' and 'closed()' different?  This might be confusing for
> > callers as I think your intention is that a closed pipe could be destroyed
> soon
> > but a caller could see that and try to connect again.
> > 
> > Can you use one connection state and then use DCHECKS to ensure the data
> channel
> > isn't re-connected as you try to delete it?  Another option would be to not
> > delete it and allow the owner to Close/Destroy it as needed.  I'm not sure
why
> > this object needs to destroy itself.
> 
> I may rename "closed()" to "finialized()" to avoid any confusion.
> The goal of this function is to ensure the derived classes have a way to track
> the lifetime of itself even the pipe has never been opened.
> Say an implementation uses a timer, it needs to stop the timer once the "this"
> will be deleted later. This usually can be done by OnDisconnecting(). But once
> the pipe has not been connected at all, the OnDisconnecting() function won't
be
> called.

I think this is more confusing as finalized() is a C#/Java term and doesn't
apply here (there is no finalization queue).

Providing a method to check the state of a soon to be deleted instance is
dangerous.  This should be fixed by changing the design at a higher level I
think.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:45: // Returns false before the channel
is connected.
On 2017/05/31 00:11:52, Hzj_jie wrote:
> On 2017/05/30 16:24:17, joedow wrote:
> > I'd make this comment more generic, "Returns false if the message is not
> sent".
> 
> Updated, but I still prefer to explicitly mention the reason of failure.
> Otherwise the caller may not know what should be done once the function
returns
> false. i.e.
> ==========================
> if (connected()) {
>   bool r = Send();
>   DCHECK(r);
> }
> ==========================
> should be perfectly fine.

I think it is better to avoid impl details in the header comment.  If you want
to prevent people from calling Send before connecting, then DCHECK the state.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:46: bool
Send(google::protobuf::MessageLite* message, const base::Closure& done);
On 2017/06/01 15:43:34, Hzj_jie wrote:
> On 2017/05/31 19:49:02, Sergey Ulanov wrote:
> > On 2017/05/31 00:11:52, Hzj_jie wrote:
> > > On 2017/05/30 16:24:17, joedow wrote:
> > > > Why is message a pointer and not a const ref, are you planning on
changing
> > it?
> > > 
> > > Because MessagePipe::Send() receives a pointer. It's questionable to use
> > pointer
> > > in MessagePipe::Send(): it may be helpful to merge the message with
others,
> > > though it's not likely to happen. But to avoid a copy, I need to use
pointer
> > > here.
> > 
> > Potentially MessagePipe::Send() can be changed to use const-ref.
> > MessageLite is an interface, and interfaces are normally passed by pointer.
> > AFAIR that's the reason it's currently passed by pointer. In most cases
> > const-ref args can be copied, but MessageLite can't.
> 
> MessageLite is not copyable anyway, so using const ref seems still reasonable.
I
> can try it in a different change.

I didn't realize MessageLite was an interface, thanks Sergey.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:52: // connection or observe the
connection state.
On 2017/05/31 00:11:52, Hzj_jie wrote:
> On 2017/05/30 16:24:17, joedow wrote:
> > Why use inheritance here instead of an observer pattern (which is more
common
> in
> > our codebase)?
> 
> A derived class of DataChannelHandler handles data instead of observing data,
a
> typical implementation should both receive and send data. Observer pattern
seems
> not match the requirement of this class.

I missed that the first time around.  A better approach would be to invert this
relationship then.  Create an interface which is passed into an instance of the
DataChannelHandler.

That Interface will expose:
virtual void void OnIncomingMessage(std::unique_ptr<CompoundBuffer> message);
void OnConnected();
void OnDisconnecting();

The wrapper will own the instance of the class implementing the interface and
control its lifetime.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.cc:27:
constructors_.push_back(std::make_pair(regex, constructor));
On 2017/05/31 00:11:52, Hzj_jie wrote:
> On 2017/05/30 16:24:18, joedow wrote:
> > Do you want to check for double-registration (i.e. if two callers register
the
> > same regex, only one will get called, the other will not).
> > 
> > Perhaps using a Dictionary would be good here to check for message handler
> > existence to prevent this type of problem.
> 
> Yes, I considered this issue before, but since regex is using here and the
pipe
> name is coming from the peer, we cannot guarantee no pipe name can match two
or
> more regexes.
> i.e. both ".*" and ".+" match an unempty string.
> I can add a DEBUG section in OnIncomingDataChannel to ensure only one match
> happens.
> 
> Using prefix matching reduces the flexibility, so I prefer regex here.

Flexibility doesn't necessarily lead to a good API.  Can you think of a use case
where a regex is required?

Image searching for the handler for a certain message type.  In our existing
codebase, I know the name of the channel/message type and can search for the
string to see who handles it.  If it is a regex, that may not be true since they
can take any form.

If there is a solid use case, then a regex is OK, but otherwise I don't
personally see the value.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.cc:34: for (auto& constructor :
constructors_) {
On 2017/05/31 00:11:52, Hzj_jie wrote:
> On 2017/05/30 16:24:18, joedow wrote:
> > Using a dictionary would be cleaner here.  You could retrieve the factory
> > instance using the message-type portion of the name and then use the factory
> > directly (instead of using the second member of a pair).
> 
> A dictionary cannot handle prefix matches.

Sure it can.  The prefix is the name of the channel.  If we define a format for
channel names, say {name}::{instance_id} then the user of this interface passes
in "name".  Name is then a key in the Dict and you can check for uniqueness.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager.h (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.h:39: bool RegisterHandlerFactory(const
std::string& regex,
On 2017/05/31 00:11:52, Hzj_jie wrote:
> On 2017/05/30 16:24:18, joedow wrote:
> > A regex isn't needed is it?  Data channels have a standard format
> > ({prefix}-{unique_pipe_id}).  Can't we register a strict substring for the
> > prefix (aka message type)?  I don't think we would want to register a
handler
> > based on the pipe_id suffix right?
> > 
> > I think it would be less error prone to allow registration for the
> message_type
> > and use a starts_with match.
> 
> I do not think there is any restriction of the naming of data channels.
> IMO, using regex does not increase the complexity (most of the clients can
still
> use a pure string for a full match) and ensure the safety (for the prefix-id
> case, prefix-\d+ should be safer to avoid unexpected matches to prefix-ABC or
> prefix-.)

Can you think of a scenario where a regex is required?

Using something like 'channel-name' is simple and difficult to introduce bugs. 
Using regex for the channel name when the flexibility is not required just
overcomplicates usage of the class.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager_unittest.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager_unittest.cc:120: ProcessResourceUsage
usage;
On 2017/05/31 00:11:53, Hzj_jie wrote:
> On 2017/05/30 16:24:18, joedow wrote:
> > Can you avoid bringing in external dependencies into the test?  Can we just
> > define a test message type at the top of this file and use that instead?
> 
> This is not doable since proto is automatically generated from .proto file. I
> will add a new proto to avoid the dependency.

Thanks!

https://codereview.chromium.org/2907073003/diff/200001/remoting/protocol/data...
File remoting/protocol/data_channel_handler.cc (right):

https://codereview.chromium.org/2907073003/diff/200001/remoting/protocol/data...
remoting/protocol/data_channel_handler.cc:41:
DCHECK(thread_checker_.CalledOnValidThread());
If you DCHECK(connected()); here, you can prevent misuse of this method and
clean up your header comment.

Hzj_jie

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

3 years, 6 months ago (2017-06-01 18:39:18 UTC) #54

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-01 18:39:58 UTC) #55

Hzj_jie

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc File remoting/protocol/data_channel_handler.cc (right): https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_channel_handler.cc#newcode30 remoting/protocol/data_channel_handler.cc:30: OnDisconnected(); On 2017/06/01 17:25:25, joedow wrote: > On 2017/05/31 ...

3 years, 6 months ago (2017-06-01 19:32:09 UTC) #56

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_handler.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.cc:30: OnDisconnected();
On 2017/06/01 17:25:25, joedow wrote:
> On 2017/05/31 00:11:51, Hzj_jie wrote:
> > On 2017/05/30 16:24:17, joedow wrote:
> > > Should this be called 'OnDisconnecting()'?  You are notifiying listeners
of
> > the
> > > disconnect but the connection hasn't been disconnected yet (that happens
on
> > line
> > > 34).
> > 
> > Since this is the only callback regarding to the disconnection, I have not
> > opinion on Disconnecting or Disconnected.
> > Updated.
> 
> It is important as they have very different meanings.  Thanks for changing it.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_handler.h (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:43: bool connected() const { return
is_connected_; }
On 2017/06/01 17:25:25, joedow wrote:
> On 2017/05/31 00:11:51, Hzj_jie wrote:
> > On 2017/05/30 16:24:17, joedow wrote:
> > > How are 'connected()' and 'closed()' different?  This might be confusing
for
> > > callers as I think your intention is that a closed pipe could be destroyed
> > soon
> > > but a caller could see that and try to connect again.
> > > 
> > > Can you use one connection state and then use DCHECKS to ensure the data
> > channel
> > > isn't re-connected as you try to delete it?  Another option would be to
not
> > > delete it and allow the owner to Close/Destroy it as needed.  I'm not sure
> why
> > > this object needs to destroy itself.
> > 
> > I may rename "closed()" to "finialized()" to avoid any confusion.
> > The goal of this function is to ensure the derived classes have a way to
track
> > the lifetime of itself even the pipe has never been opened.
> > Say an implementation uses a timer, it needs to stop the timer once the
"this"
> > will be deleted later. This usually can be done by OnDisconnecting(). But
once
> > the pipe has not been connected at all, the OnDisconnecting() function won't
> be
> > called.
> 
> I think this is more confusing as finalized() is a C#/Java term and doesn't
> apply here (there is no finalization queue).
> 
> Providing a method to check the state of a soon to be deleted instance is
> dangerous.  This should be fixed by changing the design at a higher level I
> think.

I have changed the function name into closing().

This is helpful when using DataChannelHandler with Timer.
==============
Implementation::Implementation() {
  repeat_timer_.Start(FROM_HERE, interval, this, &Implementation::DoSomething);
}

void Implementation::DoSomething() {
  if (closing()) {
    repeat_timer_.Stop();
    return;
  }

  Send(...);
}
==============
Because the DeleteSoon() happens in the end of the thread task runner, but
repeat_timer_ posts task before it, this implementation is always safe. i.e.
PostTask() in current thread task runner once closing() is true is disallowed.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:45: // Returns false before the channel
is connected.
On 2017/06/01 17:25:26, joedow wrote:
> On 2017/05/31 00:11:52, Hzj_jie wrote:
> > On 2017/05/30 16:24:17, joedow wrote:
> > > I'd make this comment more generic, "Returns false if the message is not
> > sent".
> > 
> > Updated, but I still prefer to explicitly mention the reason of failure.
> > Otherwise the caller may not know what should be done once the function
> returns
> > false. i.e.
> > ==========================
> > if (connected()) {
> >   bool r = Send();
> >   DCHECK(r);
> > }
> > ==========================
> > should be perfectly fine.
> 
> I think it is better to avoid impl details in the header comment.  If you want
> to prevent people from calling Send before connecting, then DCHECK the state.

DCHECK() the state does not match the design of "an optional data channel". i.e.
Failed to send one or more data packets should not impact the result.

And we have bunch of cases to describe the situations once the function fails.
E.g.
https://cs.chromium.org/chromium/src/tools/gn/scope.h?q=bool.*%5C(.%2B%5C)&l=...
and
https://cs.chromium.org/chromium/src/third_party/webrtc/base/stream.h?dr=C&l=169.

It seems not like an implementation detail, but the explanation of the failure
reason.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:46: bool
Send(google::protobuf::MessageLite* message, const base::Closure& done);
On 2017/06/01 17:25:25, joedow wrote:
> On 2017/06/01 15:43:34, Hzj_jie wrote:
> > On 2017/05/31 19:49:02, Sergey Ulanov wrote:
> > > On 2017/05/31 00:11:52, Hzj_jie wrote:
> > > > On 2017/05/30 16:24:17, joedow wrote:
> > > > > Why is message a pointer and not a const ref, are you planning on
> changing
> > > it?
> > > > 
> > > > Because MessagePipe::Send() receives a pointer. It's questionable to use
> > > pointer
> > > > in MessagePipe::Send(): it may be helpful to merge the message with
> others,
> > > > though it's not likely to happen. But to avoid a copy, I need to use
> pointer
> > > > here.
> > > 
> > > Potentially MessagePipe::Send() can be changed to use const-ref.
> > > MessageLite is an interface, and interfaces are normally passed by
pointer.
> > > AFAIR that's the reason it's currently passed by pointer. In most cases
> > > const-ref args can be copied, but MessageLite can't.
> > 
> > MessageLite is not copyable anyway, so using const ref seems still
reasonable.
> I
> > can try it in a different change.
> 
> I didn't realize MessageLite was an interface, thanks Sergey.

Done.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_handler.h:52: // connection or observe the
connection state.
On 2017/06/01 17:25:25, joedow wrote:
> On 2017/05/31 00:11:52, Hzj_jie wrote:
> > On 2017/05/30 16:24:17, joedow wrote:
> > > Why use inheritance here instead of an observer pattern (which is more
> common
> > in
> > > our codebase)?
> > 
> > A derived class of DataChannelHandler handles data instead of observing
data,
> a
> > typical implementation should both receive and send data. Observer pattern
> seems
> > not match the requirement of this class.
> 
> I missed that the first time around.  A better approach would be to invert
this
> relationship then.  Create an interface which is passed into an instance of
the
> DataChannelHandler.
> 
> That Interface will expose:
> virtual void void OnIncomingMessage(std::unique_ptr<CompoundBuffer> message);
> void OnConnected();
> void OnDisconnecting();
> 
> The wrapper will own the instance of the class implementing the interface and
> control its lifetime.

I still do not see the difference,
1. who will control the lifetime of the wrapper?
2. How to send data through the interface?
3. DataChannelHandler itself handles the data, instead of sending and receiving
data: it's a MessagePipe::EventHandler, but not a MessagePipe. So I cannot quite
tell why we need to delegate the implementation to an interface.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.cc:27:
constructors_.push_back(std::make_pair(regex, constructor));
On 2017/06/01 17:25:26, joedow wrote:
> On 2017/05/31 00:11:52, Hzj_jie wrote:
> > On 2017/05/30 16:24:18, joedow wrote:
> > > Do you want to check for double-registration (i.e. if two callers register
> the
> > > same regex, only one will get called, the other will not).
> > > 
> > > Perhaps using a Dictionary would be good here to check for message handler
> > > existence to prevent this type of problem.
> > 
> > Yes, I considered this issue before, but since regex is using here and the
> pipe
> > name is coming from the peer, we cannot guarantee no pipe name can match two
> or
> > more regexes.
> > i.e. both ".*" and ".+" match an unempty string.
> > I can add a DEBUG section in OnIncomingDataChannel to ensure only one match
> > happens.
> > 
> > Using prefix matching reduces the flexibility, so I prefer regex here.
> 
> Flexibility doesn't necessarily lead to a good API.  Can you think of a use
case
> where a regex is required?
> 
> Image searching for the handler for a certain message type.  In our existing
> codebase, I know the name of the channel/message type and can search for the
> string to see who handles it.  If it is a regex, that may not be true since
they
> can take any form.
> 
> If there is a solid use case, then a regex is OK, but otherwise I don't
> personally see the value.

As I have mentioned in another comment, using regex won't increase the
complexity for regular full match, but provide a more stable matching mechanism
then using prefix.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.cc:34: for (auto& constructor :
constructors_) {
On 2017/06/01 17:25:26, joedow wrote:
> On 2017/05/31 00:11:52, Hzj_jie wrote:
> > On 2017/05/30 16:24:18, joedow wrote:
> > > Using a dictionary would be cleaner here.  You could retrieve the factory
> > > instance using the message-type portion of the name and then use the
factory
> > > directly (instead of using the second member of a pair).
> > 
> > A dictionary cannot handle prefix matches.
> 
> Sure it can.  The prefix is the name of the channel.  If we define a format
for
> channel names, say {name}::{instance_id} then the user of this interface
passes
> in "name".  Name is then a key in the Dict and you can check for uniqueness.

Then it will introduce another magic character or string (no matter - or ::) .
This seems not right to me because we are using an existing label()
(https://cs.chromium.org/chromium/src/third_party/webrtc/api/datachannelinterf...)
instead of a newly defined string. What if the magic string we are using is
disallowed in other data channel implementations?

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager.h (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.h:39: bool RegisterHandlerFactory(const
std::string& regex,
On 2017/06/01 17:25:26, joedow wrote:
> On 2017/05/31 00:11:52, Hzj_jie wrote:
> > On 2017/05/30 16:24:18, joedow wrote:
> > > A regex isn't needed is it?  Data channels have a standard format
> > > ({prefix}-{unique_pipe_id}).  Can't we register a strict substring for the
> > > prefix (aka message type)?  I don't think we would want to register a
> handler
> > > based on the pipe_id suffix right?
> > > 
> > > I think it would be less error prone to allow registration for the
> > message_type
> > > and use a starts_with match.
> > 
> > I do not think there is any restriction of the naming of data channels.
> > IMO, using regex does not increase the complexity (most of the clients can
> still
> > use a pure string for a full match) and ensure the safety (for the prefix-id
> > case, prefix-\d+ should be safer to avoid unexpected matches to prefix-ABC
or
> > prefix-.)
> 
> Can you think of a scenario where a regex is required?
> 
> Using something like 'channel-name' is simple and difficult to introduce bugs.

> Using regex for the channel name when the flexibility is not required just
> overcomplicates usage of the class.

By using regex, you can still use "channel-name" without any impact. But for
advanced usage, say "prefix-\d+" case I have mentioned in the previous comment,
it can provide a more stable match mechanism.

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager_unittest.cc (right):

https://codereview.chromium.org/2907073003/diff/80001/remoting/protocol/data_...
remoting/protocol/data_channel_manager_unittest.cc:120: ProcessResourceUsage
usage;
On 2017/06/01 17:25:26, joedow wrote:
> On 2017/05/31 00:11:53, Hzj_jie wrote:
> > On 2017/05/30 16:24:18, joedow wrote:
> > > Can you avoid bringing in external dependencies into the test?  Can we
just
> > > define a test message type at the top of this file and use that instead?
> > 
> > This is not doable since proto is automatically generated from .proto file.
I
> > will add a new proto to avoid the dependency.
> 
> Thanks!

Done.

https://codereview.chromium.org/2907073003/diff/200001/remoting/protocol/data...
File remoting/protocol/data_channel_handler.cc (right):

https://codereview.chromium.org/2907073003/diff/200001/remoting/protocol/data...
remoting/protocol/data_channel_handler.cc:41:
DCHECK(thread_checker_.CalledOnValidThread());
On 2017/06/01 17:25:26, joedow wrote:
> If you DCHECK(connected()); here, you can prevent misuse of this method and
> clean up your header comment.

This seems not a necessary requirement to the "optional data channel".

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-01 20:06:52 UTC) #57

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-01 20:06:53 UTC) #58

Sergey Ulanov

I agree with Joe that regular expression are not necessary for this feature. https://codereview.chromium.org/2907073003/diff/220001/remoting/protocol/data_channel_manager.h File ...

3 years, 6 months ago (2017-06-05 18:48:29 UTC) #59

Hzj_jie

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

3 years, 6 months ago (2017-06-06 01:02:55 UTC) #60

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-06 01:03:43 UTC) #61

Hzj_jie

https://codereview.chromium.org/2907073003/diff/220001/remoting/protocol/data_channel_manager.h File remoting/protocol/data_channel_manager.h (right): https://codereview.chromium.org/2907073003/diff/220001/remoting/protocol/data_channel_manager.h#newcode8 remoting/protocol/data_channel_manager.h:8: #include <map> On 2017/06/05 18:48:29, Sergey Ulanov wrote: > ...

3 years, 6 months ago (2017-06-06 03:21:52 UTC) #62

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-06 04:35:15 UTC) #63

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/311054)

3 years, 6 months ago (2017-06-06 04:35:16 UTC) #64

joedow

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data_channel_handler.cc File remoting/protocol/data_channel_handler.cc (right): https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data_channel_handler.cc#newcode35 remoting/protocol/data_channel_handler.cc:35: base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this); Why not just delete now instead of ...

3 years, 6 months ago (2017-06-06 23:33:26 UTC) #65

Hzj_jie

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

3 years, 6 months ago (2017-06-07 01:01:57 UTC) #66

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-07 01:02:16 UTC) #67

Hzj_jie

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

3 years, 6 months ago (2017-06-07 01:08:34 UTC) #68

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-07 01:08:58 UTC) #69

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-07 02:38:31 UTC) #71

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-07 02:38:32 UTC) #72

Hzj_jie

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data_channel_handler.cc File remoting/protocol/data_channel_handler.cc (right): https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data_channel_handler.cc#newcode35 remoting/protocol/data_channel_handler.cc:35: base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this); On 2017/06/06 23:33:25, joedow wrote: > Why ...

3 years, 6 months ago (2017-06-07 17:31:25 UTC) #73

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
File remoting/protocol/data_channel_handler.cc (right):

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_handler.cc:35:
base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
On 2017/06/06 23:33:25, joedow wrote:
> Why not just delete now instead of doing it async?  You reset the pipe already
> so I'm not sure what else would block here.
> 
> Close should never be called twice as you don't know how quickly the instance
> will be deleted and once it is, your member methods above won't protect
against
> a crash.

If the DataChannelHandler works with a timer and the timer can only be stopped
in OnDisconnecting, deleting immediately here is not safe. We can ensure posting
task after OnDisconnecting() is disallowed, but any tasks posted before it can
be executed safely.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_handler.cc:47: return false;
On 2017/06/06 23:33:25, joedow wrote:
> Why don't you want to DCHECK if someone tries to send before the pipe is
> connected?  Returning a bool is safe, but I would probably end up DCHECK'ING
in
> the calling code to prevent bugs (which kicks the can down the road).
> 
> When is it ok to send when the pipe is not connected?

It also relates to the potential usage of timer: the derived classes do not need
to check connected() in the timer callback before Send() if it does not care
about whether the data can be sent.
In another side, if we are using DCHECK() here, we force the derived classes to
check connected() before calling Send().

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
File remoting/protocol/data_channel_handler.h (right):

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_handler.h:28: // A base class to handle data from
a named data channel. This class manages the
On 2017/06/06 23:33:25, joedow wrote:
> nit: Replace 'data channel' with 'MessagePipe'.  Otherwise we should define
what
> a DataChannel is somewhere (or call it WebRtcDataChannelHandler)
> 
> I'd like to be specific and consistent with the terms used so we don't have to
> update the comments if we decide to use it elsewhere.

Done.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_handler.h:31: class DataChannelHandler : public
MessagePipe::EventHandler {
On 2017/06/06 23:33:25, joedow wrote:
> Consider renaming to something like MessagePipeHandlerBase since the class
isn't
> specific to a Data Channel.  That way it will also be listed next to the
> MessagePipe class in the directory structure which will show that they are
> strongly related.

I think NamedMessagePipeHandler would be better, as there is not a name for a
typical MessagePipe.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_handler.h:34: // This class is not virtual, but
it is an no-op instance. So hide the
On 2017/06/06 23:33:25, joedow wrote:
> This class isn't a no-op instance though, it does work and handles instance
> lifetime.  Could this comment be updated to state that callers should create
> instances of the derived types instead of the base class which is why the
c'tor
> is hidden?

Done.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_handler.h:37: std::unique_ptr<MessagePipe> pipe);
On 2017/06/06 23:33:25, joedow wrote:
> Would it makes sense to use a zero-param c'tor and instead add an
'Initialize()'
> method which is passed the params?
> 
> Using this means the derived class needs to have these values passed in as
well,
> however if you used a non-virtual init() method, then the derived class would
> never see the actual values here and would need to use the protected methods
to
> access the pipe name.

Keeping a copy of |name| in derived classes is not that bad: not encouraged, but
without significant harmfulness.
Keeping a raw pointer of |pipe| is definitely wrong and can be simply found.

The concern of two-phase construction is that this pattern is not supported by
language. So there is not guarantee that Initialize() function can be called
only once. (Calling it twice is definitely not the right thing to do.)

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_handler.h:62: 
On 2017/06/06 23:33:25, joedow wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_handler.h:65: virtual void OnDisconnecting();
On 2017/06/06 23:33:25, joedow wrote:
> Can these be pure virtual?  Is the only benefit that you can create instances
> for testing?

I believe in most the case, only one or two of them are useful.
- If the implementation sends data regularly: OnConnected() and
OnDisconnecting() are useful.
- If the implementation receives data: OnIncomingMessage() and OnDisconnecting()
are useful.
- If the implementation sends after receives: OnIncomingMessage() is useful.
- If the implementation receives after sends: OnConnected() and
OnIncomingMessage() is useful.

So indeed I cannot find a case to use all of these three functions.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_handler.h:68: friend class
base::DeleteHelper<DataChannelHandler>;
On 2017/06/06 23:33:25, joedow wrote:
> Why is DeleteHelper needed here?  I haven't used it before so curious why it
is
> required.

Because the destructor is protected (to avoid unexpected "delete object;"),
TaskRunner::DeleteSoon cannot access it. The DeleteHelper is the functor to do
the real destruction work, so we need to give the friendship to it.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
File remoting/protocol/data_channel_manager.cc (right):

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_manager.cc:11: #include
"remoting/protocol/data_channel_handler.h"
On 2017/06/06 23:33:25, joedow wrote:
> Remove this header since it isn't used here.

Done.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_manager.cc:20: const std::string& regex,
On 2017/06/06 23:33:25, joedow wrote:
> replace 'regex' with 'prefix'

Done.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_manager.cc:22: if (regex.empty() || !constructor)
{
On 2017/06/06 23:33:25, joedow wrote:
> Just curious, shouldn't calling this method with an empty prefix or empty
> factory callback be a critical error?
> 
> I think the if statement should be replaced with:
> DCHECK(!prefix.empty());
> DCHECK(constructor);
> 
> This method should fail hard if the caller does not call it correctly to make
> debugging easier.

Done.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_manager.cc:33: #if DCHECK_IS_ON()
On 2017/06/06 23:33:25, joedow wrote:
> I think you can remove this block now that you aren't using regexes anymore. 
As
> long as we put the channel-prefixes in a common place then any overlap should
be
> obvious.

Done.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
File remoting/protocol/data_channel_manager.h (right):

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_manager.h:17: class DataChannelHandler;
On 2017/06/06 23:33:25, joedow wrote:
> remove this fwd declare since it isn't used.

Done.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_manager.h:24: // data channel manually by calling
DataChannelHandler::Close() function.
On 2017/06/06 23:33:25, joedow wrote:
> I don't think the DCM should include any impl details about the DCH.  For
> instance the bit about calling Close() is already out of date as that method
is
> protected so a client can't call it directly.
> 
> Can you update the class description to be specific to the DCM?

The client means "the client of DataChannelHandler". But anyway, this comment is
do out of date. I have updated it.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_manager.h:32: ~DataChannelManager();
On 2017/06/06 23:33:25, joedow wrote:
> Would it make sense to expose a static function and return an instance to a
> singleton here?  You should never have more than one of these per session /
host
> lifetime.

The lifetime of a DataChannelManager should be consistent with ClientSession.
But the later one is not a singleton object in the lifetime of the host.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_manager.h:34: // Registers a factory function to
create a DataChannelHandler to handle a new
On 2017/06/06 23:33:25, joedow wrote:
> As discussed, it may not create a DataChannelHandler.  Can you update the
> comments in the header to be specific to the DCM class?

Done.

https://codereview.chromium.org/2907073003/diff/240001/remoting/protocol/data...
remoting/protocol/data_channel_manager.h:42: // Returns true if a handler exists
for the new data channel.
On 2017/06/06 23:33:25, joedow wrote:
> nit: Doesn't the function return true if a handler was created for the
incoming
> request (not just that one exists)?

Done.

Hzj_jie

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

3 years, 6 months ago (2017-06-08 23:44:25 UTC) #74

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-08 23:45:09 UTC) #75

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-09 01:55:08 UTC) #76

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/314035)

3 years, 6 months ago (2017-06-09 01:55:09 UTC) #77

joedow

lgtm https://codereview.chromium.org/2907073003/diff/300001/remoting/protocol/data_channel_manager.h File remoting/protocol/data_channel_manager.h (right): https://codereview.chromium.org/2907073003/diff/300001/remoting/protocol/data_channel_manager.h#newcode35 remoting/protocol/data_channel_manager.h:35: // Executes the registerd callback to handle the ...

3 years, 6 months ago (2017-06-09 03:29:47 UTC) #79

Hzj_jie

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

3 years, 6 months ago (2017-06-09 17:40:09 UTC) #80

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-09 17:40:47 UTC) #81

Hzj_jie

https://codereview.chromium.org/2907073003/diff/300001/remoting/protocol/data_channel_manager.h File remoting/protocol/data_channel_manager.h (right): https://codereview.chromium.org/2907073003/diff/300001/remoting/protocol/data_channel_manager.h#newcode35 remoting/protocol/data_channel_manager.h:35: // Executes the registerd callback to handle the new ...

3 years, 6 months ago (2017-06-09 17:52:54 UTC) #82

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-09 19:07:32 UTC) #83

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-09 19:07:33 UTC) #84

Hzj_jie

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org Link to the ...

3 years, 6 months ago (2017-06-09 23:02:43 UTC) #86

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-09 23:03:21 UTC) #87

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 320001, "attempt_start_ts": 1497049362628340, "parent_rev": "5e80aa85a179f05d664a003574ad1bcd2e7353e8", "commit_rev": "4a15f818c5fdbd1e2c44bf102047ecd9b2c166e2"}

3 years, 6 months ago (2017-06-09 23:08:32 UTC) #88

commit-bot: I haz the power

Description was changed from ========== [Chromoting] Add DataChannelManager to manage optional incoming data channels DataChannelManager ...

3 years, 6 months ago (2017-06-09 23:08:46 UTC) #89

commit-bot: I haz the power

Committed patchset #8 (id:320001) as https://chromium.googlesource.com/chromium/src/+/4a15f818c5fdbd1e2c44bf102047ecd9b2c166e2

3 years, 6 months ago (2017-06-09 23:08:51 UTC) #90

findit-for-me

Findit (https://goo.gl/kROfz5) identified this CL at revision 478442 as the culprit for failures in the ...

3 years, 6 months ago (2017-06-09 23:53:51 UTC) #91

Devlin

A revert of this CL (patchset #8 id:320001) has been created in https://codereview.chromium.org/2928133005/ by rdevlin.cronin@chromium.org. ...

3 years, 6 months ago (2017-06-10 01:06:33 UTC) #92

Hzj_jie

Description was changed from ========== [Chromoting] Add DataChannelManager to manage optional incoming data channels DataChannelManager ...

3 years, 6 months ago (2017-06-11 05:20:57 UTC) #93

Hzj_jie

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

3 years, 6 months ago (2017-06-11 21:07:45 UTC) #94

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-11 21:07:50 UTC) #95

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-11 22:33:53 UTC) #96

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-11 22:33:54 UTC) #97

Hzj_jie

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org Link to the ...

3 years, 6 months ago (2017-06-11 22:58:42 UTC) #99

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-11 22:58:56 UTC) #100

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 340001, "attempt_start_ts": 1497221921781150, "parent_rev": "0c16a25942e36255b85605f000115c7cb325207b", "commit_rev": "f81a3b57195b0cfde497915d8297b760111420fb"}

3 years, 6 months ago (2017-06-11 23:03:39 UTC) #101

commit-bot: I haz the power

Description was changed from ========== [Chromoting] Add DataChannelManager to manage optional incoming data channels DataChannelManager ...

3 years, 6 months ago (2017-06-11 23:03:55 UTC) #102

commit-bot: I haz the power

3 years, 6 months ago (2017-06-11 23:03:58 UTC) #103

Message was sent while issue was closed.

Committed patchset #9 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/f81a3b57195b0cfde497915d8297...

Issue 2907073003: [Chromoting] Add DataChannelManager to manage optional incoming data channels (Closed)

Description

Patch Set 1 #

Patch Set 2 : Resolve review comments #

Patch Set 3 : Resolve review comments #

Patch Set 4 : Resolve review comments #

Patch Set 5 : Resolve review comments #

Patch Set 6 : Resolve review comments #

Patch Set 7 : Remove return value of Send() function and asynchronously destruction. #

Patch Set 8 : Update several comments #

Patch Set 9 : Fix test failure without DCHECK #

Messages