Adding the base ChromotingInstance implementation and unittests. This class will be used by the ap…

Adding the base ChromotingInstance implementation and unittests.  This class will be used by the app remoting test driver in a subsequent checkin.

BUG=464808
Committed: https://crrev.com/20fd2b06a7e26f7ad265912f8c8d322c3cb5a306
Cr-Commit-Position: refs/heads/master@{#320161}

[joedow, 2015-03-07 01:03:00]

joedow@chromium.org changed reviewers:
+ sergeyu@chromium.org, wez@chromium.org

[joedow, 2015-03-07 01:03:01]

Adding the ChromotingInstance class and unittests for
use with the App Remoting Test Driver.  This class handles the creation of a
remote host connection and allows a test to register for events which it will
use for result verification.  Since this is a larger change, I will send out the
usage of the class and test cases after this has landed.

Sergey, can you take a look at the change I made to the ChromotingClient?  I
wanted to mock out the ConnectionToHost class which required that
ChromotingClient used a scoped_ptr instead of class directly.

Wez, can you review the rest?  :)

Thanks,
Joe

[Sergey Ulanov, 2015-03-09 06:44:41]

https://codereview.chromium.org/976233003/diff/40001/remoting/client/chromoti...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/client/chromoti...
remoting/client/chromoting_client.cc:36: connection_.reset(new
protocol::ConnectionToHost());
this should be in the initializer list above, but I think it's better to move it
to Start().

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
File remoting/test/chromoting_instance.cc (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:33: const char kRemotingCapabilities[] =
nit: empty line above this one

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:43: const char
kConnectionStateInitializing[]  = "INITIALIZING";
These consts are used only to convert enums to strings for logging. It's not
useful to define consts in cases such as this. Just inline the value sin the
ToFriendlyString() functions that would make code shorter and more readable.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:68: connection_error_code_(protocol::OK) {}
Move } to a separate line. I believe clang-format should do that for you
automatically.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:72: // themselves already, but we will
clear the list here just in case.
DCHECK that the observer list is empty? Actually ObserverList can do it
automatically. Declare connection_observers_ as
ObserverList<RemoteConnectionObserver, true> for that.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:87: // A valid message loop is required to
create a connection.
nit: I don't think this check is really useful. Any code that actually needs the
message loop may check it itself or it will just crash. Also you are calling
base::ThreadTaskRunnerHandle::Get() below - it already DCHECKs that there is a
message loop.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:93:
scoped_refptr<remoting::URLRequestContextGetter> request_context_getter;
here and everywhere else remove remoting::

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:133: network_settings));
nit: these arguments can fit on one line. Please use clang-format.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:157: std::string(),  // const std::string&
client_pairing_id
don't need to specify type in the comment.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:191:
DCHECK(!connection_observers_.HasObserver(observer));
Don't need this. ObserverList checks for it.

I would the DCHECK that observer != null. It's not very likely for it to be
useful, but feel free to keep it if you disagree.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:199:
DCHECK(connection_observers_.HasObserver(observer));
Don't need this DCHECK.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:212: DVLOG(1) <<
"ChromotingInstance::OnConnectionState() Called";
does this need to be DVLOG, given that this is test code? Use VLOG()?
can this be a single message: 
  VLOG(1) << "ChromotingInstance::OnConnectionState(" <<
              ConnectionStateToFriendlyString(state) << ", "
              ProtocolErrorToFriendlyString(error_code) << ") called";

Same for other log statements

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
File remoting/test/chromoting_instance.h (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.h:34: class ChromotingInstance :
Can this be called something else? We already have remoting::ChromotingInstance,
which is visble in remoting::test namespace..
It doesn't need to be "Instance". remoting::ChromotingInstance is called that
way because of Pepper, where pp::Instance is a plugin instance. This can be just
TestClient.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.h:35: public remoting::ClientUserInterface,
nit: don't need remoting::

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.h:123: scoped_ptr<remoting::ChromotingClient>
chromoting_client_;
Don't need to specify namespace

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
File remoting/test/chromoting_instance_unittest.cc (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance_unittest.cc:64:
scoped_ptr<base::MessageLoopForIO> message_loop_;
Don't need this to be a pointer, see my next comment.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance_unittest.cc:64:
scoped_ptr<base::MessageLoopForIO> message_loop_;
Usually message_loop_ is defined as a first field in tests, so it outlives other
objects.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance_unittest.cc:79: // Create a temporary message
loop if the current thread does not already
This code is never expected to be invoked with a MessageLoop created somewhere
else. DOn't need this check.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance_unittest.cc:101: &fake_connection_to_host_,
&FakeConnectionToHost::ConnectPtr));
Do you need MockConnectionToHost, given that there is FakeConnectionToHost?
GMOCK usage is discouraged. See
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis...
. This is the case when GMOCK usage makes code less readable. It's also
confusing to call FakeConnectionToHost here.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/remote_con...
File remoting/test/remote_connection_observer.h (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/test/remote_con...
remoting/test/remote_connection_observer.h:51: virtual void
DisconnectedFromRemoteHost() {}
Why do we need this given there is ConnectionStateChanged()

https://codereview.chromium.org/976233003/diff/40001/remoting/test/video_rend...
File remoting/test/video_renderer.h (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/test/video_rend...
remoting/test/video_renderer.h:17: class VideoRenderer :
call it TestVideoRenderer?

https://codereview.chromium.org/976233003/diff/40001/remoting/test/video_rend...
remoting/test/video_renderer.h:25: // remoting::VideoRenderer interface.
This should be in the public part of this class. We put interface overrides in
the private section only when the fact that the class implements that interface
is an implementation detail. In this case VideoStub can be in private section
because it could be implemented in a separate class and it wouldn't change how
clients of VideoRenderer would use it. In the same time this class must
implement VideoRenderer interface, so implementation of that interface is
public.

[joedow, 2015-03-09 21:09:26]

Thanks for the feedback Sergey!  I've updated my CL, PTAL.

Thanks!
Joe

https://codereview.chromium.org/976233003/diff/40001/remoting/client/chromoti...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/client/chromoti...
remoting/client/chromoting_client.cc:36: connection_.reset(new
protocol::ConnectionToHost());
On 2015/03/09 06:44:40, Sergey Ulanov wrote:
> this should be in the initializer list above, but I think it's better to move
it
> to Start().

The problem with moving this code to start is that the user of the class may
want to call SetProtocolConfigForTests() before calling Start() and then
connection_ will not be defined.  I've added it to the initialized list as I
think that is the safest solution and creating a ConnectionToHost that will be
overwritten by some tests seems like a reasonable trade-off.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
File remoting/test/chromoting_instance.cc (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:33: const char kRemotingCapabilities[] =
On 2015/03/09 06:44:40, Sergey Ulanov wrote:
> nit: empty line above this one

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:43: const char
kConnectionStateInitializing[]  = "INITIALIZING";
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> These consts are used only to convert enums to strings for logging. It's not
> useful to define consts in cases such as this. Just inline the value sin the
> ToFriendlyString() functions that would make code shorter and more readable. 

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:68: connection_error_code_(protocol::OK) {}
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> Move } to a separate line. I believe clang-format should do that for you
> automatically.

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:72: // themselves already, but we will
clear the list here just in case.
On 2015/03/09 06:44:40, Sergey Ulanov wrote:
> DCHECK that the observer list is empty? Actually ObserverList can do it
> automatically. Declare connection_observers_ as
> ObserverList<RemoteConnectionObserver, true> for that.

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:87: // A valid message loop is required to
create a connection.
On 2015/03/09 06:44:40, Sergey Ulanov wrote:
> nit: I don't think this check is really useful. Any code that actually needs
the
> message loop may check it itself or it will just crash. Also you are calling
> base::ThreadTaskRunnerHandle::Get() below - it already DCHECKs that there is a
> message loop. 

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:93:
scoped_refptr<remoting::URLRequestContextGetter> request_context_getter;
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> here and everywhere else remove remoting::

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:133: network_settings));
On 2015/03/09 06:44:40, Sergey Ulanov wrote:
> nit: these arguments can fit on one line. Please use clang-format.

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:157: std::string(),  // const std::string&
client_pairing_id
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> don't need to specify type in the comment.

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:191:
DCHECK(!connection_observers_.HasObserver(observer));
On 2015/03/09 06:44:40, Sergey Ulanov wrote:
> Don't need this. ObserverList checks for it.
> 
> I would the DCHECK that observer != null. It's not very likely for it to be
> useful, but feel free to keep it if you disagree.

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:199:
DCHECK(connection_observers_.HasObserver(observer));
On 2015/03/09 06:44:40, Sergey Ulanov wrote:
> Don't need this DCHECK.

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.cc:212: DVLOG(1) <<
"ChromotingInstance::OnConnectionState() Called";
On 2015/03/09 06:44:40, Sergey Ulanov wrote:
> does this need to be DVLOG, given that this is test code? Use VLOG()?
> can this be a single message: 
>   VLOG(1) << "ChromotingInstance::OnConnectionState(" <<
>               ConnectionStateToFriendlyString(state) << ", "
>               ProtocolErrorToFriendlyString(error_code) << ") called";
> 
> Same for other log statements

I've been using DVLOG in my other test objects.  I'd prefer to keep these DVLOG
for the time being and then update all of the test code in a single change to
keep logging consistent.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
File remoting/test/chromoting_instance.h (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.h:34: class ChromotingInstance :
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> Can this be called something else? We already have
remoting::ChromotingInstance,
> which is visble in remoting::test namespace..
> It doesn't need to be "Instance". remoting::ChromotingInstance is called that
> way because of Pepper, where pp::Instance is a plugin instance. This can be
just
> TestClient.

I received the same feedback from Wez so I will think of a better name for this
class.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.h:35: public remoting::ClientUserInterface,
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> nit: don't need remoting::

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance.h:123: scoped_ptr<remoting::ChromotingClient>
chromoting_client_;
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> Don't need to specify namespace

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
File remoting/test/chromoting_instance_unittest.cc (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance_unittest.cc:64:
scoped_ptr<base::MessageLoopForIO> message_loop_;
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> Don't need this to be a pointer, see my next comment.

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance_unittest.cc:64:
scoped_ptr<base::MessageLoopForIO> message_loop_;
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> Usually message_loop_ is defined as a first field in tests, so it outlives
other
> objects.

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance_unittest.cc:79: // Create a temporary message
loop if the current thread does not already
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> This code is never expected to be invoked with a MessageLoop created somewhere
> else. DOn't need this check.

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...
remoting/test/chromoting_instance_unittest.cc:101: &fake_connection_to_host_,
&FakeConnectionToHost::ConnectPtr));
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> Do you need MockConnectionToHost, given that there is FakeConnectionToHost?
> GMOCK usage is discouraged. See
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis...
> . This is the case when GMOCK usage makes code less readable. It's also
> confusing to call FakeConnectionToHost here.

Thanks for this info, I see GMOCK used often in the chromoting code base and the
GMock docs don't mention any of this...I'll remove the mocks since they aren't
super useful and just complicate the test code.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/remote_con...
File remoting/test/remote_connection_observer.h (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/test/remote_con...
remoting/test/remote_connection_observer.h:51: virtual void
DisconnectedFromRemoteHost() {}
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> Why do we need this given there is ConnectionStateChanged()

I had intended to use the last two methods (connected/disconnected) in this
interface for observers who didn't want to handle any state or error values, the
logic to determine if the connection was established or disconnected could just
use the last two.

I'll remove them for this CL and see if we need that simplification once I have
more code which uses this interface.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/video_rend...
File remoting/test/video_renderer.h (right):

https://codereview.chromium.org/976233003/diff/40001/remoting/test/video_rend...
remoting/test/video_renderer.h:17: class VideoRenderer :
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> call it TestVideoRenderer?

Done.

https://codereview.chromium.org/976233003/diff/40001/remoting/test/video_rend...
remoting/test/video_renderer.h:25: // remoting::VideoRenderer interface.
On 2015/03/09 06:44:41, Sergey Ulanov wrote:
> This should be in the public part of this class. We put interface overrides in
> the private section only when the fact that the class implements that
interface
> is an implementation detail. In this case VideoStub can be in private section
> because it could be implemented in a separate class and it wouldn't change how
> clients of VideoRenderer would use it. In the same time this class must
> implement VideoRenderer interface, so implementation of that interface is
> public.

Done.

[chromium-reviews, 2015-03-10 17:49:21]

Ping :)

On Mon, Mar 9, 2015 at 2:09 PM, <joedow@chromium.org> wrote:

> Thanks for the feedback Sergey!  I've updated my CL, PTAL.
>
> Thanks!
> Joe
>
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/client/chromoting_client.cc
> File remoting/client/chromoting_client.cc (right):
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/client/chromoting_client.cc#newcode36
> remoting/client/chromoting_client.cc:36: connection_.reset(new
> protocol::ConnectionToHost());
> On 2015/03/09 06:44:40, Sergey Ulanov wrote:
>
>> this should be in the initializer list above, but I think it's better
>>
> to move it
>
>> to Start().
>>
>
> The problem with moving this code to start is that the user of the class
> may want to call SetProtocolConfigForTests() before calling Start() and
> then connection_ will not be defined.  I've added it to the initialized
> list as I think that is the safest solution and creating a
> ConnectionToHost that will be overwritten by some tests seems like a
> reasonable trade-off.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc
> File remoting/test/chromoting_instance.cc (right):
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode33
> remoting/test/chromoting_instance.cc:33: const char
> kRemotingCapabilities[] =
> On 2015/03/09 06:44:40, Sergey Ulanov wrote:
>
>> nit: empty line above this one
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode43
> remoting/test/chromoting_instance.cc:43: const char
> kConnectionStateInitializing[]  = "INITIALIZING";
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> These consts are used only to convert enums to strings for logging.
>>
> It's not
>
>> useful to define consts in cases such as this. Just inline the value
>>
> sin the
>
>> ToFriendlyString() functions that would make code shorter and more
>>
> readable.
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode68
> remoting/test/chromoting_instance.cc:68:
> connection_error_code_(protocol::OK) {}
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> Move } to a separate line. I believe clang-format should do that for
>>
> you
>
>> automatically.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode72
> remoting/test/chromoting_instance.cc:72: // themselves already, but we
> will clear the list here just in case.
> On 2015/03/09 06:44:40, Sergey Ulanov wrote:
>
>> DCHECK that the observer list is empty? Actually ObserverList can do
>>
> it
>
>> automatically. Declare connection_observers_ as
>> ObserverList<RemoteConnectionObserver, true> for that.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode87
> remoting/test/chromoting_instance.cc:87: // A valid message loop is
> required to create a connection.
> On 2015/03/09 06:44:40, Sergey Ulanov wrote:
>
>> nit: I don't think this check is really useful. Any code that actually
>>
> needs the
>
>> message loop may check it itself or it will just crash. Also you are
>>
> calling
>
>> base::ThreadTaskRunnerHandle::Get() below - it already DCHECKs that
>>
> there is a
>
>> message loop.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode93
> remoting/test/chromoting_instance.cc:93:
> scoped_refptr<remoting::URLRequestContextGetter> request_context_getter;
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> here and everywhere else remove remoting::
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode133
> remoting/test/chromoting_instance.cc:133: network_settings));
> On 2015/03/09 06:44:40, Sergey Ulanov wrote:
>
>> nit: these arguments can fit on one line. Please use clang-format.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode157
> remoting/test/chromoting_instance.cc:157: std::string(),  // const
> std::string& client_pairing_id
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> don't need to specify type in the comment.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode191
> remoting/test/chromoting_instance.cc:191:
> DCHECK(!connection_observers_.HasObserver(observer));
> On 2015/03/09 06:44:40, Sergey Ulanov wrote:
>
>> Don't need this. ObserverList checks for it.
>>
>
>  I would the DCHECK that observer != null. It's not very likely for it
>>
> to be
>
>> useful, but feel free to keep it if you disagree.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode199
> remoting/test/chromoting_instance.cc:199:
> DCHECK(connection_observers_.HasObserver(observer));
> On 2015/03/09 06:44:40, Sergey Ulanov wrote:
>
>> Don't need this DCHECK.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.cc#newcode212
> remoting/test/chromoting_instance.cc:212: DVLOG(1) <<
> "ChromotingInstance::OnConnectionState() Called";
> On 2015/03/09 06:44:40, Sergey Ulanov wrote:
>
>> does this need to be DVLOG, given that this is test code? Use VLOG()?
>> can this be a single message:
>>    VLOG(1) << "ChromotingInstance::OnConnectionState(" <<
>>                ConnectionStateToFriendlyString(state) << ", "
>>                ProtocolErrorToFriendlyString(error_code) << ") called";
>>
>
>  Same for other log statements
>>
>
> I've been using DVLOG in my other test objects.  I'd prefer to keep
> these DVLOG for the time being and then update all of the test code in a
> single change to keep logging consistent.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.h
> File remoting/test/chromoting_instance.h (right):
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.h#newcode34
> remoting/test/chromoting_instance.h:34: class ChromotingInstance :
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> Can this be called something else? We already have
>>
> remoting::ChromotingInstance,
>
>> which is visble in remoting::test namespace..
>> It doesn't need to be "Instance". remoting::ChromotingInstance is
>>
> called that
>
>> way because of Pepper, where pp::Instance is a plugin instance. This
>>
> can be just
>
>> TestClient.
>>
>
> I received the same feedback from Wez so I will think of a better name
> for this class.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.h#newcode35
> remoting/test/chromoting_instance.h:35: public
> remoting::ClientUserInterface,
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> nit: don't need remoting::
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance.h#newcode123
> remoting/test/chromoting_instance.h:123:
> scoped_ptr<remoting::ChromotingClient> chromoting_client_;
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> Don't need to specify namespace
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance_unittest.cc
> File remoting/test/chromoting_instance_unittest.cc (right):
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance_unittest.cc#newcode64
> remoting/test/chromoting_instance_unittest.cc:64:
> scoped_ptr<base::MessageLoopForIO> message_loop_;
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> Don't need this to be a pointer, see my next comment.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance_unittest.cc#newcode64
> remoting/test/chromoting_instance_unittest.cc:64:
> scoped_ptr<base::MessageLoopForIO> message_loop_;
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> Usually message_loop_ is defined as a first field in tests, so it
>>
> outlives other
>
>> objects.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance_unittest.cc#newcode79
> remoting/test/chromoting_instance_unittest.cc:79: // Create a temporary
> message loop if the current thread does not already
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> This code is never expected to be invoked with a MessageLoop created
>>
> somewhere
>
>> else. DOn't need this check.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/chromoting_instance_unittest.cc#newcode101
> remoting/test/chromoting_instance_unittest.cc:101:
> &fake_connection_to_host_, &FakeConnectionToHost::ConnectPtr));
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> Do you need MockConnectionToHost, given that there is
>>
> FakeConnectionToHost?
>
>> GMOCK usage is discouraged. See
>>
>
> https://groups.google.com/a/chromium.org/d/topic/chromium-
> dev/-KH_IP0rIWQ/discussion
>
>> . This is the case when GMOCK usage makes code less readable. It's
>>
> also
>
>> confusing to call FakeConnectionToHost here.
>>
>
> Thanks for this info, I see GMOCK used often in the chromoting code base
> and the GMock docs don't mention any of this...I'll remove the mocks
> since they aren't super useful and just complicate the test code.
>
> https://codereview.chromium.org/976233003/diff/40001/remoting/test/remote_
> connection_observer.h
> File remoting/test/remote_connection_observer.h (right):
>
> https://codereview.chromium.org/976233003/diff/40001/remoting/test/remote_
> connection_observer.h#newcode51
> remoting/test/remote_connection_observer.h:51: virtual void
> DisconnectedFromRemoteHost() {}
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> Why do we need this given there is ConnectionStateChanged()
>>
>
> I had intended to use the last two methods (connected/disconnected) in
> this interface for observers who didn't want to handle any state or
> error values, the logic to determine if the connection was established
> or disconnected could just use the last two.
>
> I'll remove them for this CL and see if we need that simplification once
> I have more code which uses this interface.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/video_renderer.h
> File remoting/test/video_renderer.h (right):
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/video_renderer.h#newcode17
> remoting/test/video_renderer.h:17: class VideoRenderer :
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> call it TestVideoRenderer?
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/diff/40001/
> remoting/test/video_renderer.h#newcode25
> remoting/test/video_renderer.h:25: // remoting::VideoRenderer interface.
> On 2015/03/09 06:44:41, Sergey Ulanov wrote:
>
>> This should be in the public part of this class. We put interface
>>
> overrides in
>
>> the private section only when the fact that the class implements that
>>
> interface
>
>> is an implementation detail. In this case VideoStub can be in private
>>
> section
>
>> because it could be implemented in a separate class and it wouldn't
>>
> change how
>
>> clients of VideoRenderer would use it. In the same time this class
>>
> must
>
>> implement VideoRenderer interface, so implementation of that interface
>>
> is
>
>> public.
>>
>
> Done.
>
> https://codereview.chromium.org/976233003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sergey Ulanov, 2015-03-11 00:09:52]

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/fake_c...
File remoting/protocol/fake_connection_to_host.cc (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/fake_c...
remoting/protocol/fake_connection_to_host.cc:34: void
FakeConnectionToHost::OnSessionStateChange(
I'm not sure why you need this. FakeConnectionToHost shouldn't be actually
protocol::Session or protocol::SessionManager, so I'm not sure how this is going
to be called.

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/fake_c...
File remoting/protocol/fake_connection_to_host.h (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/fake_c...
remoting/protocol/fake_connection_to_host.h:16: class FakeConnectionToHost :
public remoting::protocol::ConnectionToHost {
don't need remoting::

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/fake_c...
remoting/protocol/fake_connection_to_host.h:21: // ConnectionToHost interface.
s/interface/overrides/.
ConnectionToHost is not an interface at the moment.

BTW, it might be good idea to make ConnectionToHost an actual interface and move
the current implementation to ConnectionToHostImpl.

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/protoc...
File remoting/protocol/protocol_mock_objects.h (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/protoc...
remoting/protocol/protocol_mock_objects.h:73: class MockConnectionToHost :
public ConnectionToHost {
This class is no longer used in this CL, can be removed.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/remote_con...
File remoting/test/remote_connection_observer.h (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/test/remote_con...
remoting/test/remote_connection_observer.h:24: virtual void
ConnectionStateChanged(protocol::ConnectionToHost::State state,
please include headers that define protocol::ConnectionToHost::State and other
types used in this interface.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
File remoting/test/test_chromoting_client.cc (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:34: const char kRemotingCapabilities[] =
suggest renaming this to kLocalCapabilities.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:39: "casting";
Do you really need all of this. I don't think this class implements any of these
capabilities, so you probably don't need any of them here.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:83: scoped_ptr<AudioPlayer>()));
this can be just nullptr.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:120: fetch_secret_callback_ =
I don't think you need this. See my comment below.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:132: fetch_secret_callback_,
token_fetcher.Pass(), auth_methods));
If i remember correctly fetch_secret_callback is needed only for pin-based
authentication, but you have only ThirdParty auth enabled, so you can just pass
a null callback here.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:147: // destroyed however we will verify
and update the connection state if not.
Maybe better to (D)CHECK here that ChromotingClient did update state?

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:178: DVLOG(2) << "--State: " <<
ConnectionStateToFriendlyString(state);
Please see the second part of this comment:
https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:283: const char*
TestChromotingClient::ConnectionStateToFriendlyString(
This doesn't need to be a method. Can be a static function. Same for
ProtocolErrorToFriendlyString().

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
File remoting/test/test_chromoting_client.h (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.h:62: static void FetchThirdPartyToken(
this doesn't need to be a public static method. It can be just a static function
in test_chromoting_client.cc

Same for FetchSecretFromString

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.h:118: protocol::FetchSecretCallback
fetch_secret_callback_;
This doesn't need to be class member. StartConnection() passes it to the
authenticator. You don't need to store it here.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
File remoting/test/test_chromoting_client_unittest.cc (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client_unittest.cc:26: class
TestChromotingClientTest : public ::testing::Test,
I'm not sure how useful it is to unittest test code. IMO this test may create
more maintenance work without much return. Do we really need it?

[joedow, 2015-03-11 19:06:40]

Updated based on Sergey's second round of feedback, PTAL!

Thanks,
Joe

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/fake_c...
File remoting/protocol/fake_connection_to_host.cc (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/fake_c...
remoting/protocol/fake_connection_to_host.cc:34: void
FakeConnectionToHost::OnSessionStateChange(
On 2015/03/11 00:09:51, Sergey Ulanov wrote:
> I'm not sure why you need this. FakeConnectionToHost shouldn't be actually
> protocol::Session or protocol::SessionManager, so I'm not sure how this is
going
> to be called.

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/fake_c...
File remoting/protocol/fake_connection_to_host.h (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/fake_c...
remoting/protocol/fake_connection_to_host.h:16: class FakeConnectionToHost :
public remoting::protocol::ConnectionToHost {
On 2015/03/11 00:09:51, Sergey Ulanov wrote:
> don't need remoting::

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/fake_c...
remoting/protocol/fake_connection_to_host.h:21: // ConnectionToHost interface.
On 2015/03/11 00:09:51, Sergey Ulanov wrote:
> s/interface/overrides/.
> ConnectionToHost is not an interface at the moment.
> 
> BTW, it might be good idea to make ConnectionToHost an actual interface and
move
> the current implementation to ConnectionToHostImpl.

Left this as interface since now ConnectionToHost is an interface.

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/protoc...
File remoting/protocol/protocol_mock_objects.h (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/protocol/protoc...
remoting/protocol/protocol_mock_objects.h:73: class MockConnectionToHost :
public ConnectionToHost {
On 2015/03/11 00:09:51, Sergey Ulanov wrote:
> This class is no longer used in this CL, can be removed.

Done.  I had left it in the CL in case someone else needed it, but I'm fine
removing it.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/remote_con...
File remoting/test/remote_connection_observer.h (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/test/remote_con...
remoting/test/remote_connection_observer.h:24: virtual void
ConnectionStateChanged(protocol::ConnectionToHost::State state,
On 2015/03/11 00:09:51, Sergey Ulanov wrote:
> please include headers that define protocol::ConnectionToHost::State and other
> types used in this interface.

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
File remoting/test/test_chromoting_client.cc (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:34: const char kRemotingCapabilities[] =
On 2015/03/11 00:09:51, Sergey Ulanov wrote:
> suggest renaming this to kLocalCapabilities.

Acknowledged.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:39: "casting";
On 2015/03/11 00:09:52, Sergey Ulanov wrote:
> Do you really need all of this. I don't think this class implements any of
these
> capabilities, so you probably don't need any of them here.

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:83: scoped_ptr<AudioPlayer>()));
On 2015/03/11 00:09:51, Sergey Ulanov wrote:
> this can be just nullptr.

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:120: fetch_secret_callback_ =
On 2015/03/11 00:09:52, Sergey Ulanov wrote:
> I don't think you need this. See my comment below.

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:132: fetch_secret_callback_,
token_fetcher.Pass(), auth_methods));
On 2015/03/11 00:09:52, Sergey Ulanov wrote:
> If i remember correctly fetch_secret_callback is needed only for pin-based
> authentication, but you have only ThirdParty auth enabled, so you can just
pass
> a null callback here.

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:147: // destroyed however we will verify
and update the connection state if not.
On 2015/03/11 00:09:52, Sergey Ulanov wrote:
> Maybe better to (D)CHECK here that ChromotingClient did update state?

Looks like the state that is set depends on whether an active connection was
established, I'd like to keep the state consistent here so I've updated the
comments.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:178: DVLOG(2) << "--State: " <<
ConnectionStateToFriendlyString(state);
On 2015/03/11 00:09:51, Sergey Ulanov wrote:
> Please see the second part of this comment:
>
https://codereview.chromium.org/976233003/diff/40001/remoting/test/chromoting...

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.cc:283: const char*
TestChromotingClient::ConnectionStateToFriendlyString(
On 2015/03/11 00:09:51, Sergey Ulanov wrote:
> This doesn't need to be a method. Can be a static function. Same for
> ProtocolErrorToFriendlyString().

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
File remoting/test/test_chromoting_client.h (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.h:62: static void FetchThirdPartyToken(
On 2015/03/11 00:09:52, Sergey Ulanov wrote:
> this doesn't need to be a public static method. It can be just a static
function
> in test_chromoting_client.cc
> 
> Same for FetchSecretFromString

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client.h:118: protocol::FetchSecretCallback
fetch_secret_callback_;
On 2015/03/11 00:09:52, Sergey Ulanov wrote:
> This doesn't need to be class member. StartConnection() passes it to the
> authenticator. You don't need to store it here.

Done.

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
File remoting/test/test_chromoting_client_unittest.cc (right):

https://codereview.chromium.org/976233003/diff/60001/remoting/test/test_chrom...
remoting/test/test_chromoting_client_unittest.cc:26: class
TestChromotingClientTest : public ::testing::Test,
On 2015/03/11 00:09:52, Sergey Ulanov wrote:
> I'm not sure how useful it is to unittest test code. IMO this test may create
> more maintenance work without much return. Do we really need it?

Acknowledged.

[joedow, 2015-03-11 20:09:35]

The connection_to_host refactor caused a release only build break which I have
fixed and it looks good based on the try bot results.
PTAL :)

Thanks,
Joe

[Sergey Ulanov, 2015-03-11 20:22:17]

lgtm

https://codereview.chromium.org/976233003/diff/120001/remoting/protocol/conne...
File remoting/protocol/connection_to_host.h (right):

https://codereview.chromium.org/976233003/diff/120001/remoting/protocol/conne...
remoting/protocol/connection_to_host.h:13: #include
"remoting/proto/internal.pb.h"
I don't think we need this include anymore.

https://codereview.chromium.org/976233003/diff/120001/remoting/protocol/conne...
remoting/protocol/connection_to_host.h:14: #include
"remoting/protocol/clipboard_filter.h"
And this one

https://codereview.chromium.org/976233003/diff/120001/remoting/protocol/conne...
remoting/protocol/connection_to_host.h:16: #include
"remoting/protocol/session_config.h"
Forward-declare CandidateSessionConfig and this include won't be necessary.

https://codereview.chromium.org/976233003/diff/120001/remoting/protocol/conne...
File remoting/protocol/connection_to_host_impl.h (right):

https://codereview.chromium.org/976233003/diff/120001/remoting/protocol/conne...
remoting/protocol/connection_to_host_impl.h:47: class VideoStub;
Many of these classes are already forward-declared in connection_to_host.h . you
don't need to declare them again here.

https://codereview.chromium.org/976233003/diff/120001/remoting/protocol/conne...
remoting/protocol/connection_to_host_impl.h:76: //
SignalStrategy::StatusObserver interface.
please move all interfaces other than ConnectionToHost to the private section.

[joedow, 2015-03-11 20:42:54]

The CQ bit was checked by joedow@chromium.org

[joedow, 2015-03-11 20:42:55]

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

[commit-bot: I haz the power, 2015-03-11 20:43:13]

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

[commit-bot: I haz the power, 2015-03-11 22:08:12]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-11 22:09:10]

Patchset 8 (id:??) landed as
https://crrev.com/20fd2b06a7e26f7ad265912f8c8d322c3cb5a306
Cr-Commit-Position: refs/heads/master@{#320161}