Implement a dummy host to do capturing and analysis only.

Implement a dummy host to do capturing and analysis only.

BUG=

Committed: https://crrev.com/dd0b05c54c79cf5e411d95b3c33a096e336b2aa8
Cr-Commit-Position: refs/heads/master@{#391855}

[Hzj_jie, 2016-04-28 00:42:31]

Patchset #2 (id:20001) has been deleted

[Hzj_jie, 2016-04-28 00:42:39]

Patchset #2 (id:40001) has been deleted

[Hzj_jie, 2016-04-28 00:42:50]

Patchset #1 (id:1) has been deleted

[Hzj_jie, 2016-04-28 02:22:15]

Description was changed from

==========
Implement a dummy host to do capturing and analysis only.

BUG=
==========

to

==========
Implement a dummy host to do capturing and analysis only.

BUG=
==========

[Hzj_jie, 2016-04-28 02:22:15]

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

[joedow, 2016-04-28 22:53:55]

https://codereview.chromium.org/1923573006/diff/60001/remoting/codec/video_en...
File remoting/codec/video_encoder.cc (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder.cc:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
no copyright on new files

https://codereview.chromium.org/1923573006/diff/60001/remoting/codec/video_en...
File remoting/codec/video_encoder.h (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder.h:12: #include "remoting/protocol/session_config.h"
I think you can forward declare SessionConfig since it is a const ref.

https://codereview.chromium.org/1923573006/diff/60001/remoting/host/it2me_des...
File remoting/host/it2me_desktop_environment.cc (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/host/it2me_des...
remoting/host/it2me_desktop_environment.cc:52:
!base::CommandLine::ForCurrentProcess()->HasSwitch("disable-it2me-ui");
Commented on elsewhere, but it seems odd to use a command line for this if it is
not going to be used on the command line.  I think a member is reasonable (you
might also want to rename it).

https://codereview.chromium.org/1923573006/diff/60001/remoting/protocol/fake_...
File remoting/protocol/fake_connection_to_client.cc (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/protocol/fake_...
remoting/protocol/fake_connection_to_client.cc:47: if (video_stub_ &&
video_encode_task_runner_) {
Should it be an error if you have one but not the other?  Maybe they should be
set together to set the 'mode' i.e. use the VideoFramePump or use the
FakeVideoStream.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/BUILD.gn
File remoting/test/BUILD.gn (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/BUILD.gn#...
remoting/test/BUILD.gn:143: executable("dummy_host") {
I don't think dummy_host is descriptive enough.  It should reflect what the tool
does and should give the reader an idea of what it would be used for.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
File remoting/test/dummy_host.cc (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:1: // Copyright (c) 2016 The Chromium Authors. All
rights reserved.
no copyright in the header for new files.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:101: using webrtc::DesktopCapturer;
In general we don't use using statements like this in chromium.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:195: class MessageLogger {
Why aren't you using the standard logging infra we already have?

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:242: cout << "DeliverHostMessage(" <<
message.ByteSize() << ")" << endl;
I don't think cout is right, we should use the same mechanism as the rest of the
codebase (LOG/VLOG/DLOG/etc).  Take a look at using InitLogging to see if that
gives you what you want.  I don't think it makes sense to implement a new
logging mechanism though.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:312: struct Container {
I wonder if this should be a class and go into it's own file.  Also change the
name so it is descriptive.

This feels like the class could implement the Stub functions above as well so it
is more self-contained.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:340: config(SessionConfig::ForTest()),  // Need a
pipe name for linux
This comment is confusing to me, is a pipe name specified elsewhere?  or is this
more of a TODO?

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:350:
EXPECT_CALL(*static_cast<MockSession*>(connection.session()), jid())
Did you pull in the GMOCK header?  I didn't see it above.  Also, if you don't
care about the return values (which I don't think you do) you can use something
like the following to silence the warnings:
testing::nice_mock<MockedClientSessionEventHandler> mock_object;

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:363: // Must run in network_task_runner
You should DCHECK this to prevent it.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:422:
CommandLine::ForCurrentProcess()->AppendSwitch("disable-it2me-ui");
This feels odd unless there is a use case where we want to use this value on the
command line.  Is it possible to add this as a param to the factory instead?

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:439: assert(remoting::LoadResources("en-US"));
Why aren't we using chromium asserts here?

[Hzj_jie, 2016-04-29 00:37:47]

Thank you Joe for the reviewing. When I was started this binary, I did not
realize it may become such huge. And yes, I should decouple main function with
other reusable stuff. I will address your comments tomorrow, but focus on making
it work today.

[Hzj_jie, 2016-05-03 19:07:06]

https://codereview.chromium.org/1923573006/diff/60001/remoting/codec/video_en...
File remoting/codec/video_encoder.cc (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder.cc:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
On 2016/04/28 22:53:54, joedow wrote:
> no copyright on new files

Done.

https://codereview.chromium.org/1923573006/diff/60001/remoting/codec/video_en...
File remoting/codec/video_encoder.h (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder.h:12: #include "remoting/protocol/session_config.h"
On 2016/04/28 22:53:54, joedow wrote:
> I think you can forward declare SessionConfig since it is a const ref.

Done.

https://codereview.chromium.org/1923573006/diff/60001/remoting/host/it2me_des...
File remoting/host/it2me_desktop_environment.cc (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/host/it2me_des...
remoting/host/it2me_desktop_environment.cc:52:
!base::CommandLine::ForCurrentProcess()->HasSwitch("disable-it2me-ui");
On 2016/04/28 22:53:54, joedow wrote:
> Commented on elsewhere, but it seems odd to use a command line for this if it
is
> not going to be used on the command line.  I think a member is reasonable (you
> might also want to rename it).

Done.

https://codereview.chromium.org/1923573006/diff/60001/remoting/protocol/fake_...
File remoting/protocol/fake_connection_to_client.cc (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/protocol/fake_...
remoting/protocol/fake_connection_to_client.cc:47: if (video_stub_ &&
video_encode_task_runner_) {
On 2016/04/28 22:53:54, joedow wrote:
> Should it be an error if you have one but not the other?  Maybe they should be
> set together to set the 'mode' i.e. use the VideoFramePump or use the
> FakeVideoStream.

It's a good point, indeed I have not thought about it. Is there any possibility
one of these two can combine with another parameters to impact some other
behavior?

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/BUILD.gn
File remoting/test/BUILD.gn (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/BUILD.gn#...
remoting/test/BUILD.gn:143: executable("dummy_host") {
On 2016/04/28 22:53:54, joedow wrote:
> I don't think dummy_host is descriptive enough.  It should reflect what the
tool
> does and should give the reader an idea of what it would be used for.

Emm.... Renamed to It2MeStandaloneHost

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
File remoting/test/dummy_host.cc (right):

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:1: // Copyright (c) 2016 The Chromium Authors. All
rights reserved.
On 2016/04/28 22:53:54, joedow wrote:
> no copyright in the header for new files.

Done.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:101: using webrtc::DesktopCapturer;
On 2016/04/28 22:53:55, joedow wrote:
> In general we don't use using statements like this in chromium.

Yes, I should remove most of them before sending out the CR, but unfortunately I
forgot. This has been done in the next iteration.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:195: class MessageLogger {
On 2016/04/28 22:53:55, joedow wrote:
> Why aren't you using the standard logging infra we already have?

Renamed to MessageCounter, sorry for the confusion, it's not a logger.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:242: cout << "DeliverHostMessage(" <<
message.ByteSize() << ")" << endl;
On 2016/04/28 22:53:54, joedow wrote:
> I don't think cout is right, we should use the same mechanism as the rest of
the
> codebase (LOG/VLOG/DLOG/etc).  Take a look at using InitLogging to see if that
> gives you what you want.  I don't think it makes sense to implement a new
> logging mechanism though.

These cout(s) have been removed, they were for a quick debugging purpose.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:312: struct Container {
On 2016/04/28 22:53:55, joedow wrote:
> I wonder if this should be a class and go into it's own file.  Also change the
> name so it is descriptive.
> 
> This feels like the class could implement the Stub functions above as well so
it
> is more self-contained.

Done.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:340: config(SessionConfig::ForTest()),  // Need a
pipe name for linux
On 2016/04/28 22:53:55, joedow wrote:
> This comment is confusing to me, is a pipe name specified elsewhere?  or is
this
> more of a TODO?

Updated. We cannot support audio capture in Linux now, since we need a pipe
name.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:350:
EXPECT_CALL(*static_cast<MockSession*>(connection.session()), jid())
On 2016/04/28 22:53:55, joedow wrote:
> Did you pull in the GMOCK header?  I didn't see it above.  Also, if you don't
> care about the return values (which I don't think you do) you can use
something
> like the following to silence the warnings:
> testing::nice_mock<MockedClientSessionEventHandler> mock_object;

Great, thank you for the hint.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:363: // Must run in network_task_runner
On 2016/04/28 22:53:55, joedow wrote:
> You should DCHECK this to prevent it.

No, ClientSession::OnConnectionAuthenticated and other similar functions have
the DCHECK embedded.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:422:
CommandLine::ForCurrentProcess()->AppendSwitch("disable-it2me-ui");
On 2016/04/28 22:53:55, joedow wrote:
> This feels odd unless there is a use case where we want to use this value on
the
> command line.  Is it possible to add this as a param to the factory instead?

Yes, removed.

https://codereview.chromium.org/1923573006/diff/60001/remoting/test/dummy_hos...
remoting/test/dummy_host.cc:439: assert(remoting::LoadResources("en-US"));
On 2016/04/28 22:53:55, joedow wrote:
> Why aren't we using chromium asserts here?

Done.

[joedow, 2016-05-03 22:28:30]

https://codereview.chromium.org/1923573006/diff/180001/remoting/codec/video_e...
File remoting/codec/video_encoder.cc (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/codec/video_e...
remoting/codec/video_encoder.cc:27: NOTREACHED();
Should this include a log to state which values was seen?

something like:
NOTREACHED() << "Unknown code type: " << video_config.codec;

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
File remoting/host/it2me_desktop_environment.cc (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.cc:9: #include "base/command_line.h"
Can you remove this header now?

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
File remoting/host/it2me_desktop_environment.h (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.h:63: void enable_user_interface(bool
enabled) {
s/enable_user_interface/set_enable_user_interface

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.h:68: bool enable_user_interface_;
nit: add newline between vars and MACRO

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.h:68: bool enable_user_interface_;
init to 'true' here?

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/BUILD.gn
File remoting/test/BUILD.gn (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/BUILD.gn...
remoting/test/BUILD.gn:157: static_library("it2me_standalone_host") {
I like this name much better than dummy_host, thanks!

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/BUILD.gn...
remoting/test/BUILD.gn:180: "it2me_standalone_host_runner.cc",
The convention I've seen for the executable cc file is to call it "..._main" so
I think this should be "it2me_standalone_host_main"

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/counter_...
File remoting/test/counter_stubs.h (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/counter_...
remoting/test/counter_stubs.h:4: 
Missing guard macro in the header file.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/counter_...
remoting/test/counter_stubs.h:35: 
This file should probably get broken down a bit more.  We try to avoid impls in
header files (such as NoBarrierAtomic below) as it should be included in the cc
file for the class that uses it.  If it needs to be included externally (to the
file) then it should get its own .h/.cc file.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/counter_...
remoting/test/counter_stubs.h:150: protocol::FakeConnectionToClient* connection_
= nullptr;
DISALLOW_COPY_AND_ASSIGN(...)?

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host.cc (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:27: : message_loop_(),
Do these need to be included?  I thought default init came for free

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:45: audio_stub_(),
You don't need to include these in the initializer list do you?  I thought the
default c'tor was already called for you by the time you hit the opening curly
braces below

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:82: void It2MeStandaloneHost::Run() {
I'd move this file above the private methods in this file.  I think it is nice
when the methods in the cc file are roughly in the same order as the header.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:85:
base::Bind(&It2MeStandaloneHost::ConnectProxy, this));
This method doesn't need to be static.  Instead of the work you are doing here
you could do:
PostTask(&It2MeStandaloneHost::Connect, base::Unretained(this));  You shouldn't
need to implement RefCounted as long as you can verify that your instance will
not be destroyed before the task_runner.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:89:
It2MeStandaloneHost::~It2MeStandaloneHost() {}
d'tor should go below the c'tor

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host.h (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:4: 
Add header macro guard.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:6: // without needing a client to connect
to.
I don't think this comment is needed here, you could move it above the class
definition though.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:21: class NiceMock;
I think you can remove this forward decl since you added the header.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:49: // Must run in the same thread as
Should you use a thread checker to enforce this comment?

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:68: std::unique_ptr<ClientSession>
session_;
DISALLOW_COPY_AND_ASSIGN(...)?

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host_runner.cc (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_runner.cc:53: OutputLogger("host",
host->host_stub());
I do wonder if this log could go into a logging class or perhaps the host
itself.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_runner.cc:61:
base::TimeDelta::FromSeconds(1));
Why not use a RepeatingTimer here?

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_runner.cc:85:
DCHECK(remoting::LoadResources("en-US"));
IIRC DCHECKS are removed completely on release builds, do you still want to load
resources on release?

[Hzj_jie, 2016-05-04 02:11:57]

https://codereview.chromium.org/1923573006/diff/180001/remoting/codec/video_e...
File remoting/codec/video_encoder.cc (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/codec/video_e...
remoting/codec/video_encoder.cc:27: NOTREACHED();
On 2016/05/03 22:28:29, joedow wrote:
> Should this include a log to state which values was seen?
> 
> something like:
> NOTREACHED() << "Unknown code type: " << video_config.codec;

I copied and pasted this logic from ice_connection_to_client.cc, but yes, I can
add log.

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
File remoting/host/it2me_desktop_environment.cc (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.cc:9: #include "base/command_line.h"
On 2016/05/03 22:28:29, joedow wrote:
> Can you remove this header now?

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
File remoting/host/it2me_desktop_environment.h (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.h:63: void enable_user_interface(bool
enabled) {
On 2016/05/03 22:28:29, joedow wrote:
> s/enable_user_interface/set_enable_user_interface

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.h:68: bool enable_user_interface_;
On 2016/05/03 22:28:29, joedow wrote:
> nit: add newline between vars and MACRO

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.h:68: bool enable_user_interface_;
On 2016/05/03 22:28:29, joedow wrote:
> init to 'true' here?

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/BUILD.gn
File remoting/test/BUILD.gn (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/BUILD.gn...
remoting/test/BUILD.gn:157: static_library("it2me_standalone_host") {
On 2016/05/03 22:28:29, joedow wrote:
> I like this name much better than dummy_host, thanks!

:)

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/BUILD.gn...
remoting/test/BUILD.gn:180: "it2me_standalone_host_runner.cc",
On 2016/05/03 22:28:29, joedow wrote:
> The convention I've seen for the executable cc file is to call it "..._main"
so
> I think this should be "it2me_standalone_host_main"

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/counter_...
File remoting/test/counter_stubs.h (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/counter_...
remoting/test/counter_stubs.h:4: 
On 2016/05/03 22:28:29, joedow wrote:
> Missing guard macro in the header file.

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/counter_...
remoting/test/counter_stubs.h:35: 
On 2016/05/03 22:28:29, joedow wrote:
> This file should probably get broken down a bit more.  We try to avoid impls
in
> header files (such as NoBarrierAtomic below) as it should be included in the
cc
> file for the class that uses it.  If it needs to be included externally (to
the
> file) then it should get its own .h/.cc file.

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/counter_...
remoting/test/counter_stubs.h:150: protocol::FakeConnectionToClient* connection_
= nullptr;
On 2016/05/03 22:28:29, joedow wrote:
> DISALLOW_COPY_AND_ASSIGN(...)?

I have disallowed copy and assign for MessageCounter, so copy and assign of
these classes are implicitly disallowed.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host.cc (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:27: : message_loop_(),
On 2016/05/03 22:28:30, joedow wrote:
> Do these need to be included?  I thought default init came for free

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:45: audio_stub_(),
On 2016/05/03 22:28:29, joedow wrote:
> You don't need to include these in the initializer list do you?  I thought the
> default c'tor was already called for you by the time you hit the opening curly
> braces below

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:82: void It2MeStandaloneHost::Run() {
On 2016/05/03 22:28:30, joedow wrote:
> I'd move this file above the private methods in this file.  I think it is nice
> when the methods in the cc file are roughly in the same order as the header.

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:85:
base::Bind(&It2MeStandaloneHost::ConnectProxy, this));
On 2016/05/03 22:28:30, joedow wrote:
> This method doesn't need to be static.  Instead of the work you are doing here
> you could do:
> PostTask(&It2MeStandaloneHost::Connect, base::Unretained(this));  You
shouldn't
> need to implement RefCounted as long as you can verify that your instance will
> not be destroyed before the task_runner.

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:89:
It2MeStandaloneHost::~It2MeStandaloneHost() {}
On 2016/05/03 22:28:30, joedow wrote:
> d'tor should go below the c'tor

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host.h (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:4: 
On 2016/05/03 22:28:30, joedow wrote:
> Add header macro guard.

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:6: // without needing a client to connect
to.
On 2016/05/03 22:28:30, joedow wrote:
> I don't think this comment is needed here, you could move it above the class
> definition though.

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:21: class NiceMock;
On 2016/05/03 22:28:30, joedow wrote:
> I think you can remove this forward decl since you added the header.

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:49: // Must run in the same thread as
On 2016/05/03 22:28:30, joedow wrote:
> Should you use a thread checker to enforce this comment?

Not really needed, ClientSession::OnConnectionAuthenticated already has a
DCHECK.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:68: std::unique_ptr<ClientSession>
session_;
On 2016/05/03 22:28:30, joedow wrote:
> DISALLOW_COPY_AND_ASSIGN(...)?

Same as other classes, copy and assign are implicitly disallowed.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host_runner.cc (right):

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_runner.cc:53: OutputLogger("host",
host->host_stub());
On 2016/05/03 22:28:30, joedow wrote:
> I do wonder if this log could go into a logging class or perhaps the host
> itself.

Done.

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_runner.cc:61:
base::TimeDelta::FromSeconds(1));
On 2016/05/03 22:28:30, joedow wrote:
> Why not use a RepeatingTimer here?

I do not know we have a RepeatingTimer before :)

https://codereview.chromium.org/1923573006/diff/180001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_runner.cc:85:
DCHECK(remoting::LoadResources("en-US"));
On 2016/05/03 22:28:30, joedow wrote:
> IIRC DCHECKS are removed completely on release builds, do you still want to
load
> resources on release?

Interesting, I believe I need this statement in both debug and release build.

[joedow, 2016-05-04 16:40:45]

https://codereview.chromium.org/1923573006/diff/220001/remoting/protocol/fake...
File remoting/protocol/fake_connection_to_client.cc (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/protocol/fake...
remoting/protocol/fake_connection_to_client.cc:47: if (video_stub_ &&
video_encode_task_runner_) {
What would it mean for a caller to set one of these and not the other?  Should
that be considered an error (to let the caller know the object isn't
initialized) or should the init methods be changed so they require both to be
set?  Or should we add a DCHECK to make sure they are bot set or both null?

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
File remoting/test/counter_stubs.cc (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:116: last_size_(),
Why are these members in the init list?  They should have their default c'tor
called automatically.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:155: int last_size_;
Init basic types inline, not in the initializer list.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:161: void OutputCounter(std::ostream& os,
DisplayStatistics?

Why isn't this part of the MessageCounter class? Each class knows it's type and
counter member.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:198: public:
Add a comment to note which methods belong to the Stub class you are deriving
from.  Same for the other class in this file.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:218: LogMessage(*audio_packet);
Since you are using unique_ptr, I think it wuold be clearer to use the get
method instead of dereference (audio_packet.get()) instead of *audio_packet. 
This practice is what I have seen in the other parts of our codebase as well.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:233: // Not a keepalive packet
comments should end with a period.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:258: return *client_stub_;
client_stub_.get()...alternatively, you could return a pointer from these
methods (this is pretty common in our codebase).

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
File remoting/test/counter_stubs.h (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:4: 
Please add macro guard.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:17: class CounterStubs {
Can you make this name more descriptive?  It seems specific to a host so maybe
HostStatisticsCounter?  HostConnectionStats?

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:19: CounterStubs(protocol::FakeConnectionToClient*
connection = nullptr);
single param c'tors should be defined as 'explicit'

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:20: virtual ~CounterStubs();
newline between c'tor/d'tor and getter methods

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:31: class CounterVideoStub;
newline between class forward decls and members.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:35: std::unique_ptr<CounterVideoStub> video_stub_;
DISALLOW_COPY_AND_ASSIGN(...)?

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host.h (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:38: void Run();
Maybe call this RunHost() or StartHost()?

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:40: void StartToOutputToStdout();
Rename to 'StartOutputTimer()' ?

is there ever a case where you want to start the output timer and not the host? 
Maybe this logic should be added to the Run() method...

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:44: //
It2MeDesktopEnvironmentFactory->caller_task_runner()
I think you should either use a deterministic method of ensuring the method is
run on the proper thread or remove the comment here.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:57: base::RepeatingTimer timer_;
DISALLOW_COPY_AND_ASSIGN(...)

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host_main.cc (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_main.cc:37: bool result =
remoting::LoadResources("en-US");
Do you only want to load resources on Linux?  I'm assuming this check should be
below the #endif statement.

[Hzj_jie, 2016-05-04 19:18:04]

https://codereview.chromium.org/1923573006/diff/220001/remoting/protocol/fake...
File remoting/protocol/fake_connection_to_client.cc (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/protocol/fake...
remoting/protocol/fake_connection_to_client.cc:47: if (video_stub_ &&
video_encode_task_runner_) {
On 2016/05/04 16:40:44, joedow wrote:
> What would it mean for a caller to set one of these and not the other?  Should
> that be considered an error (to let the caller know the object isn't
> initialized) or should the init methods be changed so they require both to be
> set?  Or should we add a DCHECK to make sure they are bot set or both null?

Oh, we have discussed this in an early iteration, since this is a fake object,
there may be some other combinations of the parameters for other scenarios.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
File remoting/test/counter_stubs.cc (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:116: last_size_(),
On 2016/05/04 16:40:44, joedow wrote:
> Why are these members in the init list?  They should have their default c'tor
> called automatically.

I prefer to keep count_ and size_ initialized in the list, since they may not
always be basic types.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:155: int last_size_;
On 2016/05/04 16:40:44, joedow wrote:
> Init basic types inline, not in the initializer list.

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:161: void OutputCounter(std::ostream& os,
On 2016/05/04 16:40:44, joedow wrote:
> DisplayStatistics?
> 
> Why isn't this part of the MessageCounter class? Each class knows it's type
and
> counter member.

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:198: public:
On 2016/05/04 16:40:44, joedow wrote:
> Add a comment to note which methods belong to the Stub class you are deriving
> from.  Same for the other class in this file.

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:218: LogMessage(*audio_packet);
On 2016/05/04 16:40:44, joedow wrote:
> Since you are using unique_ptr, I think it wuold be clearer to use the get
> method instead of dereference (audio_packet.get()) instead of *audio_packet. 
> This practice is what I have seen in the other parts of our codebase as well.

Then I will need to pass a pointer of MessageLite into LogMessage function, and
do another != nullptr check.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:233: // Not a keepalive packet
On 2016/05/04 16:40:44, joedow wrote:
> comments should end with a period.

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.cc:258: return *client_stub_;
On 2016/05/04 16:40:44, joedow wrote:
> client_stub_.get()...alternatively, you could return a pointer from these
> methods (this is pretty common in our codebase).

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
File remoting/test/counter_stubs.h (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:4: 
On 2016/05/04 16:40:44, joedow wrote:
> Please add macro guard.

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:17: class CounterStubs {
On 2016/05/04 16:40:44, joedow wrote:
> Can you make this name more descriptive?  It seems specific to a host so maybe
> HostStatisticsCounter?  HostConnectionStats?

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:19: CounterStubs(protocol::FakeConnectionToClient*
connection = nullptr);
On 2016/05/04 16:40:44, joedow wrote:
> single param c'tors should be defined as 'explicit'

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:20: virtual ~CounterStubs();
On 2016/05/04 16:40:44, joedow wrote:
> newline between c'tor/d'tor and getter methods

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:31: class CounterVideoStub;
On 2016/05/04 16:40:44, joedow wrote:
> newline between class forward decls and members.

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/counter_...
remoting/test/counter_stubs.h:35: std::unique_ptr<CounterVideoStub> video_stub_;
On 2016/05/04 16:40:45, joedow wrote:
> DISALLOW_COPY_AND_ASSIGN(...)?

Done.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host.h (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:38: void Run();
On 2016/05/04 16:40:45, joedow wrote:
> Maybe call this RunHost() or StartHost()?

Considering this class is called a Host, RunHost seems a little bit redundancy.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:40: void StartToOutputToStdout();
On 2016/05/04 16:40:45, joedow wrote:
> Rename to 'StartOutputTimer()' ?
> 
> is there ever a case where you want to start the output timer and not the
host? 
> Maybe this logic should be added to the Run() method...

I believe we may have a chance to start the host, but not output to console :)

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:44: //
It2MeDesktopEnvironmentFactory->caller_task_runner()
On 2016/05/04 16:40:45, joedow wrote:
> I think you should either use a deterministic method of ensuring the method is
> run on the proper thread or remove the comment here.

Seems I put this comment in a wrong position. I have moved it to Run function.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:57: base::RepeatingTimer timer_;
On 2016/05/04 16:40:45, joedow wrote:
> DISALLOW_COPY_AND_ASSIGN(...)

I believe this class does not require to be not copyable or assignable.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host_main.cc (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_main.cc:37: bool result =
remoting::LoadResources("en-US");
On 2016/05/04 16:40:45, joedow wrote:
> Do you only want to load resources on Linux?  I'm assuming this check should
be
> below the #endif statement.

I tried on Windows, the LoadResources failed. Looks weird, but it do happens.

[joedow, 2016-05-04 22:58:49]

lgtm after my comments are addressed.

https://codereview.chromium.org/1923573006/diff/220001/remoting/protocol/fake...
File remoting/protocol/fake_connection_to_client.cc (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/protocol/fake...
remoting/protocol/fake_connection_to_client.cc:47: if (video_stub_ &&
video_encode_task_runner_) {
On 2016/05/04 19:18:03, Hzj_jie wrote:
> On 2016/05/04 16:40:44, joedow wrote:
> > What would it mean for a caller to set one of these and not the other? 
Should
> > that be considered an error (to let the caller know the object isn't
> > initialized) or should the init methods be changed so they require both to
be
> > set?  Or should we add a DCHECK to make sure they are bot set or both null?
> 
> Oh, we have discussed this in an early iteration, since this is a fake object,
> there may be some other combinations of the parameters for other scenarios.

OK, I'd like to prevent a caller from partially initializing an instance of this
class but this is a minor nit.

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host_main.cc (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_main.cc:37: bool result =
remoting::LoadResources("en-US");
On 2016/05/04 19:18:04, Hzj_jie wrote:
> On 2016/05/04 16:40:45, joedow wrote:
> > Do you only want to load resources on Linux?  I'm assuming this check should
> be
> > below the #endif statement.
> 
> I tried on Windows, the LoadResources failed. Looks weird, but it do happens.

Based on discussion, can you move this?

https://codereview.chromium.org/1923573006/diff/240001/remoting/codec/video_e...
File remoting/codec/video_encoder.cc (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/codec/video_e...
remoting/codec/video_encoder.cc:27: NOTREACHED() << "Unknown code type: " <<
video_config.codec;
s/code/codec

https://codereview.chromium.org/1923573006/diff/240001/remoting/protocol/fake...
File remoting/protocol/fake_connection_to_client.h (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/protocol/fake...
remoting/protocol/fake_connection_to_client.h:75:
scoped_refptr<base::SingleThreadTaskRunner> runner) {
You should forward declare SingleThreadTaskRunner, I think this happens to work
as someone else is including the header for you now.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/fake_con...
File remoting/test/fake_connection_event_logger.cc (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/fake_con...
remoting/test/fake_connection_event_logger.cc:169: void
MessageCounter::DisplayStatistics(std::ostream& os, const char* name) {
Could this be passed in via the c'tor of the class or a setter?  it seems like a
property of the class more than something that could change (i.e. you wouldn't
want to call the Display method of the AudioCounterStub and pass "video" for the
name).

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/fake_con...
remoting/test/fake_connection_event_logger.cc:192: public:
I think these interface methods can all be private can't they (same for the
other 3 stub classes)?  From what I see the wrapper class only exposes the
interface pointer which would still work.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host.cc (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:27: void
OutputFakeConnectionEventLogger(const FakeConnectionEventLogger* logger) {
DCHECK(logger)?

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:27: void
OutputFakeConnectionEventLogger(const FakeConnectionEventLogger* logger) {
Also, why const FakeConnectionEventLogger* instead of
FakeConnectionEventLogger*?

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:39:
factory_(context_->file_task_runner(),
Perhaps use scoped_refptr as an alias for 'context_->file_task_runner()' and
then remove the comments?

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:44:
session_jid_("user@domain/rest-of-jid"),
Move constant ("user@domain/rest-of-jid") to anonymous namespace above?

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host.h (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:30: // An container for an it2me host by
using Counter*Stub(s) to do capturing
s/An/A

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:30: // An container for an it2me host by
using Counter*Stub(s) to do capturing
May also be good to update this since the Counter*Stubs names have changed.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:52: FakeConnectionEventLogger stubs_;
Update name from stubs_ to event_logger_?

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host_main.cc (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_main.cc:37: bool result =
remoting::LoadResources("en-US");
Move below and remove the result check since it isn't useful :)

[Hzj_jie, 2016-05-05 00:47:36]

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host_main.cc (right):

https://codereview.chromium.org/1923573006/diff/220001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host_main.cc:37: bool result =
remoting::LoadResources("en-US");
On 2016/05/04 22:58:49, joedow wrote:
> On 2016/05/04 19:18:04, Hzj_jie wrote:
> > On 2016/05/04 16:40:45, joedow wrote:
> > > Do you only want to load resources on Linux?  I'm assuming this check
should
> > be
> > > below the #endif statement.
> > 
> > I tried on Windows, the LoadResources failed. Looks weird, but it do
happens.
> 
> Based on discussion, can you move this?

Done.

https://codereview.chromium.org/1923573006/diff/240001/remoting/codec/video_e...
File remoting/codec/video_encoder.cc (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/codec/video_e...
remoting/codec/video_encoder.cc:27: NOTREACHED() << "Unknown code type: " <<
video_config.codec;
On 2016/05/04 22:58:49, joedow wrote:
> s/code/codec

Done.

https://codereview.chromium.org/1923573006/diff/240001/remoting/protocol/fake...
File remoting/protocol/fake_connection_to_client.h (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/protocol/fake...
remoting/protocol/fake_connection_to_client.h:75:
scoped_refptr<base::SingleThreadTaskRunner> runner) {
On 2016/05/04 22:58:49, joedow wrote:
> You should forward declare SingleThreadTaskRunner, I think this happens to
work
> as someone else is including the header for you now.

Done.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/fake_con...
File remoting/test/fake_connection_event_logger.cc (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/fake_con...
remoting/test/fake_connection_event_logger.cc:169: void
MessageCounter::DisplayStatistics(std::ostream& os, const char* name) {
On 2016/05/04 22:58:49, joedow wrote:
> Could this be passed in via the c'tor of the class or a setter?  it seems like
a
> property of the class more than something that could change (i.e. you wouldn't
> want to call the Display method of the AudioCounterStub and pass "video" for
the
> name).

Done.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/fake_con...
remoting/test/fake_connection_event_logger.cc:192: public:
On 2016/05/04 22:58:49, joedow wrote:
> I think these interface methods can all be private can't they (same for the
> other 3 stub classes)?  From what I see the wrapper class only exposes the
> interface pointer which would still work.

Done.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host.cc (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:27: void
OutputFakeConnectionEventLogger(const FakeConnectionEventLogger* logger) {
On 2016/05/04 22:58:49, joedow wrote:
> DCHECK(logger)?

Done.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:27: void
OutputFakeConnectionEventLogger(const FakeConnectionEventLogger* logger) {
On 2016/05/04 22:58:49, joedow wrote:
> Also, why const FakeConnectionEventLogger* instead of
> FakeConnectionEventLogger*?

Bind does not accept reference, but we do not need to modify logger instance.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:39:
factory_(context_->file_task_runner(),
On 2016/05/04 22:58:49, joedow wrote:
> Perhaps use scoped_refptr as an alias for 'context_->file_task_runner()' and
> then remove the comments?

Done.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.cc:44:
session_jid_("user@domain/rest-of-jid"),
On 2016/05/04 22:58:49, joedow wrote:
> Move constant ("user@domain/rest-of-jid") to anonymous namespace above?

Done.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
File remoting/test/it2me_standalone_host.h (right):

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:30: // An container for an it2me host by
using Counter*Stub(s) to do capturing
On 2016/05/04 22:58:49, joedow wrote:
> s/An/A

Done.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:30: // An container for an it2me host by
using Counter*Stub(s) to do capturing
On 2016/05/04 22:58:49, joedow wrote:
> May also be good to update this since the Counter*Stubs names have changed.

Done.

https://codereview.chromium.org/1923573006/diff/240001/remoting/test/it2me_st...
remoting/test/it2me_standalone_host.h:52: FakeConnectionEventLogger stubs_;
On 2016/05/04 22:58:49, joedow wrote:
> Update name from stubs_ to event_logger_?

Done.

[Hzj_jie, 2016-05-05 00:48:04]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-05-05 00:48:05]

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/1923573006/#ps260001
(title: "Resolve review commnets")

[commit-bot: I haz the power, 2016-05-05 00:48:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923573006/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923573006/260001

[commit-bot: I haz the power, 2016-05-05 00:59:24]

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

[commit-bot: I haz the power, 2016-05-05 00:59:25]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Hzj_jie, 2016-05-05 17:07:20]

zijiehe@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org

[Hzj_jie, 2016-05-05 17:10:33]

Hi, Brett and Dirk, this is Zijie He from Chromoting team, I am now implementing
a standalone binary to test Chromoting capturing performance, so I need to
execute gtk_init at the very beginning of the binary. Would you please LGTM
gtk2/BUILD.gn change?

Thank you.

[Dirk Pranke, 2016-05-05 17:18:08]

lgtm

[Hzj_jie, 2016-05-05 17:40:09]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-05-05 17:40:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923573006/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923573006/260001

[commit-bot: I haz the power, 2016-05-05 18:44:29]

Description was changed from

==========
Implement a dummy host to do capturing and analysis only.

BUG=
==========

to

==========
Implement a dummy host to do capturing and analysis only.

BUG=
==========

[commit-bot: I haz the power, 2016-05-05 18:44:30]

Committed patchset #11 (id:260001)

[commit-bot: I haz the power, 2016-05-05 18:45:52]

Description was changed from

==========
Implement a dummy host to do capturing and analysis only.

BUG=
==========

to

==========
Implement a dummy host to do capturing and analysis only.

BUG=

Committed: https://crrev.com/dd0b05c54c79cf5e411d95b3c33a096e336b2aa8
Cr-Commit-Position: refs/heads/master@{#391855}
==========

[commit-bot: I haz the power, 2016-05-05 18:45:53]

Patchset 11 (id:??) landed as
https://crrev.com/dd0b05c54c79cf5e411d95b3c33a096e336b2aa8
Cr-Commit-Position: refs/heads/master@{#391855}