Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 551173004: Move PseudoTCP and channel auth out of LibjingleTransportFactory. (Closed)

Created:
5 years, 11 months ago by Sergey Ulanov
Modified:
5 years, 11 months ago
Reviewers:
Wez
CC:
chromium-reviews, chromoting-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@clean_dgrams
Project:
chromium
Visibility:
Public.

Description

Move PseudoTCP and channel auth out of LibjingleTransportFactory. Previously TransportFactory interface was responsible for creation and initialization of several protocol layers, including PseudoTCP and authentication (TLS). Simplified it so now it only creates raw datagram transport channel. PseudoTcpChannelFactory is now responsible for setting up PseudoTcpAdapter and AuthenticatingChannelFactory takes care of channel authentication. Also added DatagramChannelFactory for Datagram channels. This change will make it possible to replace PseudoTcpChannelFactory with an object that creates SCTP-based channels. Also fixed a bug in SslHmacChannelAuthenticator. It wasn't working properly when deleted from the callback. (base::Callback objects shouldn't be deleted while being called because when deleted they also destroy reference parameters values they are holding). BUG=402993 Committed: https://crrev.com/9cb142f0cb33af89ff478e69e803fdf32f09006e Cr-Commit-Position: refs/heads/master@{#294653}

Patch Set 1 #

Total comments: 62

Patch Set 2 : #

Total comments: 4

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+561 lines, -352 lines) Patch
M remoting/protocol/BUILD.gn View 1 2 3 4 3 chunks +6 lines, -0 lines 0 comments Download
M remoting/protocol/authenticator_test_base.h View 1 3 chunks +3 lines, -4 lines 0 comments Download
M remoting/protocol/authenticator_test_base.cc View 1 2 chunks +3 lines, -2 lines 0 comments Download
M remoting/protocol/channel_authenticator.h View 1 2 chunks +4 lines, -5 lines 0 comments Download
M remoting/protocol/channel_dispatcher_base.h View 1 2 chunks +2 lines, -2 lines 0 comments Download
M remoting/protocol/channel_dispatcher_base.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M remoting/protocol/channel_factory.h View 1 1 chunk +0 lines, -54 lines 0 comments Download
M remoting/protocol/channel_multiplexer.h View 1 2 chunks +5 lines, -5 lines 0 comments Download
M remoting/protocol/channel_multiplexer.cc View 1 1 chunk +1 line, -1 line 0 comments Download
A remoting/protocol/datagram_channel_factory.h View 1 2 3 4 5 1 chunk +49 lines, -0 lines 0 comments Download
M remoting/protocol/fake_authenticator.h View 1 1 chunk +3 lines, -5 lines 0 comments Download
M remoting/protocol/fake_authenticator.cc View 1 2 4 chunks +30 lines, -19 lines 0 comments Download
M remoting/protocol/fake_session.h View 1 3 chunks +5 lines, -5 lines 0 comments Download
M remoting/protocol/fake_session.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M remoting/protocol/jingle_session.h View 1 5 chunks +14 lines, -6 lines 0 comments Download
M remoting/protocol/jingle_session.cc View 1 6 chunks +27 lines, -15 lines 0 comments Download
M remoting/protocol/jingle_session_unittest.cc View 1 2 3 chunks +5 lines, -3 lines 0 comments Download
M remoting/protocol/libjingle_transport_factory.h View 1 chunk +1 line, -2 lines 0 comments Download
M remoting/protocol/libjingle_transport_factory.cc View 1 2 17 chunks +44 lines, -155 lines 0 comments Download
M remoting/protocol/protobuf_video_reader.h View 1 2 chunks +2 lines, -2 lines 0 comments Download
M remoting/protocol/protobuf_video_reader.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M remoting/protocol/protobuf_video_writer.h View 1 2 chunks +2 lines, -2 lines 0 comments Download
M remoting/protocol/protobuf_video_writer.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M remoting/protocol/protocol_mock_objects.h View 1 1 chunk +2 lines, -2 lines 0 comments Download
A remoting/protocol/pseudotcp_channel_factory.h View 1 1 chunk +52 lines, -0 lines 0 comments Download
A remoting/protocol/pseudotcp_channel_factory.cc View 1 2 1 chunk +100 lines, -0 lines 0 comments Download
A remoting/protocol/secure_channel_factory.h View 1 1 chunk +60 lines, -0 lines 0 comments Download
A remoting/protocol/secure_channel_factory.cc View 1 1 chunk +83 lines, -0 lines 0 comments Download
M remoting/protocol/session.h View 1 2 chunks +3 lines, -3 lines 0 comments Download
M remoting/protocol/ssl_hmac_channel_authenticator.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M remoting/protocol/ssl_hmac_channel_authenticator.cc View 1 1 chunk +11 lines, -3 lines 0 comments Download
M remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc View 1 3 chunks +11 lines, -5 lines 0 comments Download
A + remoting/protocol/stream_channel_factory.h View 1 2 3 4 5 3 chunks +14 lines, -14 lines 0 comments Download
M remoting/protocol/transport.h View 1 3 chunks +7 lines, -33 lines 0 comments Download
M remoting/remoting_srcs.gypi View 1 3 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 27 (10 generated)
Sergey Ulanov
5 years, 11 months ago (2014-09-09 01:49:18 UTC) #2
Wez
nit: You has a typo in your CL description :)
5 years, 11 months ago (2014-09-10 01:51:53 UTC) #4
Wez
https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authenticated_channel_factory.cc File remoting/protocol/authenticated_channel_factory.cc (right): https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authenticated_channel_factory.cc#newcode24 remoting/protocol/authenticated_channel_factory.cc:24: // CancelChannelCreation() is expected to be called before destruction. ...
5 years, 11 months ago (2014-09-10 02:29:27 UTC) #5
Sergey Ulanov
https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authenticated_channel_factory.cc File remoting/protocol/authenticated_channel_factory.cc (right): https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authenticated_channel_factory.cc#newcode24 remoting/protocol/authenticated_channel_factory.cc:24: // CancelChannelCreation() is expected to be called before destruction. ...
5 years, 11 months ago (2014-09-10 21:50:59 UTC) #6
Wez
LGTM - this is a great piece of cleanup - thanks for taking care of ...
5 years, 11 months ago (2014-09-10 22:19:42 UTC) #7
Sergey Ulanov
https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authenticated_channel_factory.cc File remoting/protocol/authenticated_channel_factory.cc (right): https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authenticated_channel_factory.cc#newcode24 remoting/protocol/authenticated_channel_factory.cc:24: // CancelChannelCreation() is expected to be called before destruction. ...
5 years, 11 months ago (2014-09-10 22:49:31 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/551173004/60001
5 years, 11 months ago (2014-09-10 22:51:27 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/65293) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54343) win_gpu ...
5 years, 11 months ago (2014-09-10 23:57:07 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/551173004/80001
5 years, 11 months ago (2014-09-11 18:24:15 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_rel/builds/14641)
5 years, 11 months ago (2014-09-11 19:06:23 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/551173004/100001
5 years, 11 months ago (2014-09-11 20:07:41 UTC) #20
commit-bot: I haz the power
Committed patchset #5 (id:100001) as e9adf5f95329906487e4777aa89daf2b5624e150
5 years, 11 months ago (2014-09-11 21:56:43 UTC) #21
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/28d886c967e016a5d5812be43cd5916f577c2e10 Cr-Commit-Position: refs/heads/master@{#294474}
5 years, 11 months ago (2014-09-11 22:11:58 UTC) #22
nasko
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/570463002/ by nasko@chromium.org. ...
5 years, 11 months ago (2014-09-11 22:36:24 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/551173004/120001
5 years, 11 months ago (2014-09-12 18:19:24 UTC) #25
commit-bot: I haz the power
Committed patchset #6 (id:120001) as f2553532c19671791ed8b213d8e8cc0ebe8efd51
5 years, 11 months ago (2014-09-12 20:49:07 UTC) #26
commit-bot: I haz the power
5 years, 11 months ago (2014-09-12 20:52:27 UTC) #27
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/9cb142f0cb33af89ff478e69e803fdf32f09006e
Cr-Commit-Position: refs/heads/master@{#294653}

Powered by Google App Engine
This is Rietveld 408576698