Issue 580243003: Cleanup Fake* classes in remoting/protocol

Issue 580243003: Cleanup Fake* classes in remoting/protocol (Closed)

Created:
6 years, 3 months ago by Sergey Ulanov

Modified:
6 years, 2 months ago

Reviewers:
Wez

CC:
chromium-reviews, chromoting-reviews_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@sctp

Project:
chromium

Visibility:
Public.

More Reviews

Description

Cleanup Fake* classes in remoting/protocol 1. Renamed FakeSocket to FakeStreamSocket and FakeUdpSocket to FakeDatagramSocket, 2. Added factories for both stream and datagram fake sockets, 3. Move fake sockets to separate files out of fake_session.h BUG=402993 Committed: https://crrev.com/98d8fe931814c79fd763148549fdb5ee2521e312 Cr-Commit-Position: refs/heads/master@{#296270}

Patch Set 1 : #

Total comments: 66

Patch Set 2 : #

Total comments: 8

Patch Set 3 : #

Created: 6 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+786 lines, -495 lines)			Patch
M	remoting/protocol/authenticator_test_base.h	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	remoting/protocol/authenticator_test_base.cc	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	remoting/protocol/buffered_socket_writer_unittest.cc	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	remoting/protocol/channel_multiplexer_unittest.cc	View	1	5 chunks	+18 lines, -15 lines	0 comments	Download
M	remoting/protocol/connection_to_client_unittest.cc	View	1	1 chunk	+4 lines, -3 lines	0 comments	Download
A	remoting/protocol/fake_datagram_socket.h	View	1 2	1 chunk	+132 lines, -0 lines	0 comments	Download
A	remoting/protocol/fake_datagram_socket.cc	View	1 2	1 chunk	+170 lines, -0 lines	0 comments	Download
M	remoting/protocol/fake_session.h	View		2 chunks	+5 lines, -152 lines	0 comments	Download
M	remoting/protocol/fake_session.cc	View		2 chunks	+3 lines, -310 lines	0 comments	Download
A	remoting/protocol/fake_stream_socket.h	View	1 2	1 chunk	+158 lines, -0 lines	0 comments	Download
A	remoting/protocol/fake_stream_socket.cc	View	1	1 chunk	+277 lines, -0 lines	0 comments	Download
M	remoting/protocol/message_reader_unittest.cc	View		3 chunks	+3 lines, -3 lines	0 comments	Download
M	remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc	View		2 chunks	+4 lines, -4 lines	0 comments	Download
M	remoting/remoting_test.gypi	View		1 chunk	+4 lines, -0 lines	0 comments	Download

Messages

Total messages: 13 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Wez

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_datagram_socket.cc File remoting/protocol/fake_datagram_socket.cc (right): https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_datagram_socket.cc#newcode36 remoting/protocol/fake_datagram_socket.cc:36: read_pending_ = false; nit: Suggest blank line after this, ...

6 years, 3 months ago (2014-09-20 00:25:10 UTC) #5

Sergey Ulanov

6 years, 3 months ago (2014-09-22 19:22:12 UTC) #6

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
File remoting/protocol/fake_datagram_socket.cc (right):

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.cc:36: read_pending_ = false;
On 2014/09/20 00:25:09, Wez wrote:
> nit: Suggest blank line after this, since next line is part of the "copy data
to
> buffer" logical block.

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.cc:40: read_buffer_ = NULL;
On 2014/09/20 00:25:09, Wez wrote:
> nit: Suggest blank line after this, since the next line is part of the "run
> callback" logical block.

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.cc:59: memcpy(buf->data(),
&(*input_packets_[input_pos_].begin()), result);
On 2014/09/20 00:25:09, Wez wrote:
> nit: You have to do this min+memcpy dance above as well; consider factoring it
> out into a static helper function taking a destination data+length, and a
source
> buffer.

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.cc:105:
FakeDatagramChannelFactory::~FakeDatagramChannelFactory() {}
On 2014/09/20 00:25:09, Wez wrote:
> EXPECT_TRUE(channels_.empty()) here?

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.cc:125: channel.reset(new
FakeDatagramSocket());
On 2014/09/20 00:25:09, Wez wrote:
> nit: I thought you were generally opting for { } even around single-line ifs?

Nah, I don't care about it as long as the code is consistent. And this is not
new code, I'm just moving it.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.cc:125: channel.reset(new
FakeDatagramSocket());
On 2014/09/20 00:25:09, Wez wrote:
> nit: This would be more readable if you initialized |channel| with the new
fake,
> and then reset() it iff |fail_| were true.

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.cc:130: channel->PairWith(peer->second);
On 2014/09/20 00:25:09, Wez wrote:
> Won't this crash if |fail_| is true, since |channel| will be NULL?

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.cc:149:
callback.Run(scoped_ptr<net::Socket>(channels_[name]));
On 2014/09/20 00:25:09, Wez wrote:
> Don't you need to clear the channel from |channels_| as well? Otherwise it'll
> still be in the map when the factory is deleted?

We need it in |channels_| so that the channel can be retrieved by the tests
later. Changed ChannelsMap to store weak pointers.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
File remoting/protocol/fake_datagram_socket.h (right):

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:44: void AppendInputPacket(const
std::string& data);
On 2014/09/20 00:25:10, Wez wrote:
> Would it be simpler to always create these things in pairs, like pipes, and
> thereby avoid the need for AppendInputPacket or written_packets()?

No. For some tests it's easier to have not connected seconds and just validate
what have been written in the second without creating a paired socket.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:44: void AppendInputPacket(const
std::string& data);
On 2014/09/20 00:25:09, Wez wrote:
> Consider replacing the term "input" with "to read", so this becomes
> AppendPacketToRead(), since the "input" packets are the ones we want to have
the
> caller read via Read()?

We had AppendInputPacket() before this CL. I think AppendPacketToRead() has the
same change of being confusing as AppendInputPacket(), so I don't think it needs
to be renamed.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:45: int input_pos() const { return
input_pos_; }
On 2014/09/20 00:25:09, Wez wrote:
> What does |input_pos| mean? Is it a position in packets, bytes, etc?

Added comment

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:46: void PairWith(FakeDatagramSocket*
peer_socket);
On 2014/09/20 00:25:09, Wez wrote:
> Clarify that caller retains ownership of |peer_socket|?

it's not scoped_ptr<> so it should be clear that ownership is not passed here.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:53: 
On 2014/09/20 00:25:09, Wez wrote:
> nit: Remove blank line here

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:60: bool read_pending_;
On 2014/09/20 00:25:10, Wez wrote:
> nit: has_pending_read_ or has_read_pending_ or is_read_pending_?

We use read_pending_ in other places as well, and it's also used in src/net.
Removed this variable - !read_callback_.is_null() can be used instead.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:80: void set_async_creation(bool
async_creation) {
On 2014/09/20 00:25:10, Wez wrote:
> nit: set_asynchronous, or set_is_asynchronous or set_asynchronous_create?

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:84: void set_fail(bool fail) { fail_ =
fail; }
On 2014/09/20 00:25:09, Wez wrote:
> nit: set_fail_create_channel or set_fail_create?

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:86: void
PairWith(FakeDatagramChannelFactory* peer_factory);
On 2014/09/20 00:25:10, Wez wrote:
> What does it mean to pair two factories?
It means that the sockets they create are paired as soon as they are created.
Added comment.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:88: FakeDatagramSocket*
GetChannel(const std::string& name);
On 2014/09/20 00:25:09, Wez wrote:
> What are the ownership semantics of GetChannel - looks like it's intended to
> return a reference to the socket?

Added comment 


> Also clarify how this is intended to be used? I assume it's there for tests to
> be able to retrieve the FakeDatagramSocket that was created for a particular
> caller, to feed data into?

Renamed to GetFakeChannel() to make the intent clearer. and added comment.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:105:
base::WeakPtr<FakeDatagramChannelFactory> peer_factory_;
On 2014/09/20 00:25:10, Wez wrote:
> nit: In the FakeDatagramSocket the peer weakptrfactory comes first - be
> consistent.

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
File remoting/protocol/fake_session.h (right):

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_session.h:34: FakeStreamChannelFactory&
fake_channel_factory() { return channel_factory_; }
On 2014/09/20 00:25:10, Wez wrote:
> Consider leaving GetFake[Stream]Channel() on this class, as shorthand for
> fake_channel_factory().GetChannel(().

Only ChannelMultiplexer tests are using this function and we don't need them
after the multiplexer replaced with SCTP

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
File remoting/protocol/fake_stream_socket.cc (right):

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.cc:37:
EXPECT_TRUE(task_runner_->BelongsToCurrentThread());
On 2014/09/20 00:25:10, Wez wrote:
> Should these be ASSERT_TRUE, since bad things will happen if they're not as
> expected?

ASSERT_TRUE() only works in functions that return void, but there are some
functions below that do return result, so it's better to use EXPECT_TRUE()
everywhere for conisistency.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.cc:44: CHECK(result > 0);
On 2014/09/20 00:25:10, Wez wrote:
> EXPECT_GT or ASSERT_GT?

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.cc:69: if (input_pos_ <
static_cast<int>(input_data_.size())) {
On 2014/09/20 00:25:10, Wez wrote:
> Looks like this fake provides no way to return EOF (i.e. completion result
> net::OK / zero)?

Yes. We've never needed it - that feature can be added later, if and when we
need it.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.cc:159:
EXPECT_TRUE(task_runner_->BelongsToCurrentThread());
On 2014/09/20 00:25:10, Wez wrote:
> Looks like several calls e.g. ::Disconnect(), ::SetSendBufferSize,
> ::GetPeerAddress, have no thread checks?

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.cc:232: scoped_ptr<FakeStreamSocket>
channel;
On 2014/09/20 00:25:10, Wez wrote:
> See comments on datagram equivalent.

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.cc:253:
callback.Run(scoped_ptr<net::StreamSocket>(channels_[name]));
On 2014/09/20 00:25:10, Wez wrote:
> Set comments on datagram equivalent.

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
File remoting/protocol/fake_stream_socket.h (right):

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.h:24: // FakeStreamSocket implement
net::StreamSocket interface. All data written to
On 2014/09/20 00:25:10, Wez wrote:
> Is there really nothing in net/ that performs this function...?
No.

> 
> From a maintenance perspective, this and FakeDatagramSocket should really live
> under net/, so they can be updated in alignment with net socket changes.

I don't think it makes much difference from maintance perspective - any change
in the Socket interface has to update all code in chromium that depends on it.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.h:26: // reads data from another buffer
that can be set with AppendInputData().
On 2014/09/20 00:25:10, Wez wrote:
> Similar comments re naming of "input data" apply as for the datagram socket
> fake.

This is not new code, so I'd prefer to keep AppendInputData().

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.h:39: const std::string& written_data()
const { return written_data_; }
On 2014/09/20 00:25:10, Wez wrote:
> I assume that this returns a single string w/ all data that has been written
to
> the fake via Write()?

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.h:48: bool read_pending() const { return
read_pending_; }
On 2014/09/20 00:25:10, Wez wrote:
> Document each of these, e.g. specify units of write_limit, specify whether
> next_write_error_ and next_read_error_ auto-reset, units of input_pos, etc.

Done.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.h:86: bool read_pending_;
On 2014/09/20 00:25:10, Wez wrote:
> is_read_pending_ or read_is_pending_

read_pending_ is used in many other places. removed it -
!read_callback_.is_null() can be used instead.

Wez

LGTM w/ nits. https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_datagram_socket.h File remoting/protocol/fake_datagram_socket.h (right): https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_datagram_socket.h#newcode46 remoting/protocol/fake_datagram_socket.h:46: void PairWith(FakeDatagramSocket* peer_socket); On 2014/09/22 19:22:11, ...

6 years, 2 months ago (2014-09-23 17:07:39 UTC) #8

Sergey Ulanov

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_datagram_socket.h File remoting/protocol/fake_datagram_socket.h (right): https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_datagram_socket.h#newcode46 remoting/protocol/fake_datagram_socket.h:46: void PairWith(FakeDatagramSocket* peer_socket); On 2014/09/23 17:07:38, Wez wrote: > ...

6 years, 2 months ago (2014-09-23 19:06:09 UTC) #9

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
File remoting/protocol/fake_datagram_socket.h (right):

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:46: void PairWith(FakeDatagramSocket*
peer_socket);
On 2014/09/23 17:07:38, Wez wrote:
> On 2014/09/22 19:22:11, Sergey Ulanov wrote:
> > On 2014/09/20 00:25:09, Wez wrote:
> > > Clarify that caller retains ownership of |peer_socket|?
> > 
> > it's not scoped_ptr<> so it should be clear that ownership is not passed
here.
> 
> 
> Except that there is still old code in the codebase that pre-dates
scoped_ptr<>,
> and code which interfaces with e.g. WebRTC, that doesn't have scoped_ptr<>.

I believe all code in src/remoting should be using scoped_ptr<> to pass
ownership, except the parts that depend on WebRTC. I tried to clean that up a
while ago. If there is still code that doesn't scoped_ptr<> and it could, we
need to fix it.

https://codereview.chromium.org/580243003/diff/40001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:86: void
PairWith(FakeDatagramChannelFactory* peer_factory);
On 2014/09/23 17:07:38, Wez wrote:
> On 2014/09/22 19:22:11, Sergey Ulanov wrote:
> > On 2014/09/20 00:25:10, Wez wrote:
> > > What does it mean to pair two factories?
> > It means that the sockets they create are paired as soon as they are
created.
> > Added comment.
> 
> Suggest tweaking comment to make clear that the created sockets are paired
with
> the socket of the same channel name in the peer factory.

Done.

https://codereview.chromium.org/580243003/diff/80001/remoting/protocol/fake_d...
File remoting/protocol/fake_datagram_socket.cc (right):

https://codereview.chromium.org/580243003/diff/80001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.cc:36: int result =
CopyReadData(read_buffer_.get(), read_buffer_size_);
On 2014/09/23 17:07:38, Wez wrote:
> nit: This could be const int. :)

It could but we don't use it in such cases in other places and IMO it would only
harm readability of this code.

https://codereview.chromium.org/580243003/diff/80001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.cc:130: const ChannelCreatedCallback&
callback) {
On 2014/09/23 17:07:38, Wez wrote:
> nit: DCHECK that there's not already a channel called |name| in this factory
> here?

Done.

https://codereview.chromium.org/580243003/diff/80001/remoting/protocol/fake_d...
File remoting/protocol/fake_datagram_socket.h (right):

https://codereview.chromium.org/580243003/diff/80001/remoting/protocol/fake_d...
remoting/protocol/fake_datagram_socket.h:99: // feed data into it. The caller
doesn't get ownership of the result.
On 2014/09/23 17:07:38, Wez wrote:
> nit: Also indicate that it's valid to call this for destroyed or non-existent
> channels, in which case NULL is returned?

Done.

https://codereview.chromium.org/580243003/diff/80001/remoting/protocol/fake_s...
File remoting/protocol/fake_stream_socket.h (right):

https://codereview.chromium.org/580243003/diff/80001/remoting/protocol/fake_s...
remoting/protocol/fake_stream_socket.h:42: // Set's maximum number of bytes
written by each Write() call.
On 2014/09/23 17:07:38, Wez wrote:
> nit: Sets

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/580243003/100001

6 years, 2 months ago (2014-09-23 20:31:17 UTC) #11

commit-bot: I haz the power

Committed patchset #3 (id:100001) as 856de83425e6db6caa72a9abbee0a83d07adc887

6 years, 2 months ago (2014-09-23 21:56:26 UTC) #12

commit-bot: I haz the power

6 years, 2 months ago (2014-09-23 21:56:59 UTC) #13

Message was sent while issue was closed.

Patchset 3 (id:??) landed as
https://crrev.com/98d8fe931814c79fd763148549fdb5ee2521e312
Cr-Commit-Position: refs/heads/master@{#296270}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages