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

Issue 10823323: Add support for multiplexed channels in remoting::protocol::Session interface. (Closed)

Created:
7 years, 9 months ago by Sergey Ulanov
Modified:
7 years, 9 months ago
Reviewers:
Wez
CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org
Visibility:
Public.

Description

Add support for multiplexed channels in remoting::protocol::Session interface. Now the Session interface has two methods that return channel factories - one for regular channels and one for multiplexed channels. Also refactored AudioReader and AudioWriter to inherit from ChannelDispatcherBase. BUG=137135 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152240

Patch Set 1 #

Total comments: 9

Patch Set 2 : #

Patch Set 3 : ~ #

Total comments: 23

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+230 lines, -221 lines) Patch
M remoting/protocol/audio_reader.h View 1 2 3 2 chunks +6 lines, -18 lines 0 comments Download
M remoting/protocol/audio_reader.cc View 2 chunks +4 lines, -30 lines 0 comments Download
M remoting/protocol/audio_writer.h View 1 2 3 2 chunks +9 lines, -25 lines 0 comments Download
M remoting/protocol/audio_writer.cc View 1 2 3 1 chunk +5 lines, -38 lines 0 comments Download
M remoting/protocol/channel_dispatcher_base.h View 2 chunks +2 lines, -1 line 0 comments Download
M remoting/protocol/channel_dispatcher_base.cc View 1 2 2 chunks +6 lines, -5 lines 0 comments Download
M remoting/protocol/channel_multiplexer.cc View 1 2 chunks +8 lines, -4 lines 0 comments Download
M remoting/protocol/channel_multiplexer_unittest.cc View 7 chunks +17 lines, -0 lines 0 comments Download
M remoting/protocol/connection_to_host.cc View 1 chunk +2 lines, -1 line 0 comments Download
M remoting/protocol/fake_session.h View 1 2 4 chunks +12 lines, -11 lines 0 comments Download
M remoting/protocol/fake_session.cc View 1 2 3 2 chunks +25 lines, -17 lines 0 comments Download
M remoting/protocol/jingle_session.h View 1 2 5 chunks +13 lines, -5 lines 0 comments Download
M remoting/protocol/jingle_session.cc View 1 2 3 5 chunks +45 lines, -28 lines 0 comments Download
M remoting/protocol/jingle_session_unittest.cc View 1 2 5 chunks +48 lines, -13 lines 0 comments Download
M remoting/protocol/protobuf_video_reader.h View 1 2 3 2 chunks +2 lines, -3 lines 0 comments Download
M remoting/protocol/protobuf_video_reader.cc View 1 2 3 1 chunk +7 lines, -6 lines 0 comments Download
M remoting/protocol/protobuf_video_writer.h View 1 2 3 2 chunks +2 lines, -3 lines 0 comments Download
M remoting/protocol/protobuf_video_writer.cc View 1 2 4 chunks +7 lines, -6 lines 0 comments Download
M remoting/protocol/protocol_mock_objects.h View 1 2 1 chunk +2 lines, -5 lines 0 comments Download
M remoting/protocol/session.h View 1 2 3 3 chunks +8 lines, -2 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
Sergey Ulanov
7 years, 9 months ago (2012-08-14 21:38:10 UTC) #1
Wez
http://codereview.chromium.org/10823323/diff/1/remoting/protocol/channel_multiplexer.cc File remoting/protocol/channel_multiplexer.cc (right): http://codereview.chromium.org/10823323/diff/1/remoting/protocol/channel_multiplexer.cc#newcode396 remoting/protocol/channel_multiplexer.cc:396: // If this is the first multiplexed channel then ...
7 years, 9 months ago (2012-08-15 00:52:23 UTC) #2
Sergey Ulanov
http://codereview.chromium.org/10823323/diff/1/remoting/protocol/channel_multiplexer.cc File remoting/protocol/channel_multiplexer.cc (right): http://codereview.chromium.org/10823323/diff/1/remoting/protocol/channel_multiplexer.cc#newcode396 remoting/protocol/channel_multiplexer.cc:396: // If this is the first multiplexed channel then ...
7 years, 9 months ago (2012-08-15 01:35:42 UTC) #3
Wez
http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h File remoting/protocol/session.h (right): http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h#newcode103 remoting/protocol/session.h:103: // multiplexed channels work over a single regular channel. ...
7 years, 9 months ago (2012-08-16 00:45:00 UTC) #4
Sergey Ulanov
http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h File remoting/protocol/session.h (right): http://codereview.chromium.org/10823323/diff/1/remoting/protocol/session.h#newcode104 remoting/protocol/session.h:104: virtual ChannelFactory* GetMainChannelFactory() = 0; On 2012/08/16 00:45:00, Wez ...
7 years, 9 months ago (2012-08-16 00:59:48 UTC) #5
Wez
LGTM w/ nits http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_reader.h File remoting/protocol/audio_reader.h (right): http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_reader.h#newcode29 remoting/protocol/audio_reader.h:29: typedef base::Callback<void(bool)> InitializedCallback; Remove this. http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_reader.h#newcode35 ...
7 years, 9 months ago (2012-08-18 00:30:35 UTC) #6
Sergey Ulanov
http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_reader.h File remoting/protocol/audio_reader.h (right): http://codereview.chromium.org/10823323/diff/2002/remoting/protocol/audio_reader.h#newcode29 remoting/protocol/audio_reader.h:29: typedef base::Callback<void(bool)> InitializedCallback; On 2012/08/18 00:30:36, Wez wrote: > ...
7 years, 9 months ago (2012-08-18 01:53:56 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10823323/14001
7 years, 9 months ago (2012-08-18 01:54:17 UTC) #8
commit-bot: I haz the power
Change committed as 152240
7 years, 9 months ago (2012-08-18 04:17:26 UTC) #9
Wez
7 years, 9 months ago (2012-08-18 21:44:05 UTC) #10
https://chromiumcodereview.appspot.com/10823323/diff/2002/remoting/protocol/a...
File remoting/protocol/audio_reader.h (right):

https://chromiumcodereview.appspot.com/10823323/diff/2002/remoting/protocol/a...
remoting/protocol/audio_reader.h:35: static scoped_ptr<AudioReader> Create(const
SessionConfig& config);
On 2012/08/18 01:53:56, sergeyu wrote:
> On 2012/08/18 00:30:36, Wez wrote:
> > nit: While you're improving this API, shouldn't Create() appear before the
> dtor
> > declaration, and have a single-line comment explaining how to configure
> > newly-created AudioReaders (e.g. it's no longer obvious how it learns about
> the
> > Session)
> 
> Strictly speaking the style guide says that constructor/destructor always go
> first. But we can look at this method as a named constructor.

Yes, I think that makes sense.

Powered by Google App Engine
This is Rietveld 408576698