Updated latency fixture and refactored connection helper

Updated latency fixture to match internal classes and refactored
connection helper.

1. Moved the Open new document logic to the SetUp function to enforce
a fresh document for tests.

2. Refactored connection helper to make it as minimum as possible, and
moved the ping response verify logic to the connected client fixture.

BUG=

Committed: https://crrev.com/4d6371eaba0106a6051282b86ef59ccd121e6bc5
Cr-Commit-Position: refs/heads/master@{#340588}

[liaoyuke, 2015-07-23 01:43:46]

[email protected] changed reviewers:
+ [email protected], [email protected], [email protected]

[liaoyuke, 2015-07-23 01:43:47]

Hey Joe, Sergey, Anand,

Please review a changelist from liaoyuke.

Thank you very much!

[Sergey Ulanov, 2015-07-23 19:36:55]

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_connected_client_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_fixture.cc:65:
test_chromoting_client_->RemoveRemoteConnectionObserver(this);
Before dereferencing a weak pointer you need to check that it's not null. I.e.
this line will crash if test_chromoting_client_ is already destroyed.
See my comment in .h

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_fixture.cc:74:
base::ResetAndReturn(&host_message_received_callback_).Run(message);
Do we need to reset the callback? Will we ever need to receive more than one
message?

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_fixture.cc:74:
base::ResetAndReturn(&host_message_received_callback_).Run(message);
Do you need to very type of the message, in case it belongs to a different
extension?

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_connected_client_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_fixture.h:80:
base::WeakPtr<TestChromotingClient> test_chromoting_client_;
I don't think you really need this to be a weak pointer. This object is owned by
connection_helper_ owned by this class. So it's safe to make this a raw pointer
(i.e. TestChromotingClient*). But a better solution is to add an accessor in the
ConnectionHelper to get test client.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/test_chromoti...
File remoting/test/test_chromoting_client.h (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/test_chromoti...
remoting/test/test_chromoting_client.h:76: base::WeakPtr<TestChromotingClient>
GetWeakPtr() {
I don't think you really need this - see my other comments.
In general it's better to avoid weak pointers when possible (and also
ref-counted objects).

[joedow, 2015-07-23 20:26:57]

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_connected_client_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_fixture.cc:74:
base::ResetAndReturn(&host_message_received_callback_).Run(message);
To add on to Sergey's comments, you could get several messages before you see
the one you want (e.g. you can get addwindow and removewindow messages for a
splash screen before the message for the main window arrives) so you don't want
to reset this here.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_latency_test_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_latency_test_fixture.cc:54: FAIL();
Have you run this through the try-bots yet?  I seem to recall Tony having a
problem on one build flavor where having code after FAIL() (on line 49 in your
case) would cause a build failure.  You should push this to the try-bots when
you've updated the code just to make sure it builds OK for all flavors.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_latency_test_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_latency_test_fixture.h:85: virtual bool
CreateNewDocument() = 0;
I probably wouldn't name this 'CreateNewDocument' although that is what your
derived class will use it for.  I am thinking it should be something like
'PrepareApplicationForTesting' or 'SetUpRunningApplication' (or similar).  The
idea is that a derived fixture would implement this method by performing certain
actions that prepares the connected app for testing.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_latency_test_fixture.h:123: //
AppRemotingConnectionHelper own its lifetime.
nit: reword this: "Used to maintain a reference to the TestVideoRenderer
instance while it exists."

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_latency_test_fixture.h:127: //
AppRemotingConnectionHelper own its lifetime.
nit: reword this: "Used to maintain a reference to the TestChromotingClient
instance while it exists."

[liaoyuke, 2015-07-24 02:01:04]

Patchset #3 (id:40001) has been deleted

[liaoyuke, 2015-07-24 02:07:28]

Hey Joe, Sergey, Anand,

I've addressed the feedback in my last patch set, meanwhile, to support creating
RGBValue in writing tests, I moved RGBValue struct out of TestVideoRenderer as a
standalone file, which results in large number of files in this CL, but most of
files only have a few lines' change.

Please take a look. Thank you very much!

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_connected_client_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_fixture.cc:65:
test_chromoting_client_->RemoveRemoteConnectionObserver(this);
On 2015/07/23 19:36:55, Sergey Ulanov wrote:
> Before dereferencing a weak pointer you need to check that it's not null. I.e.
> this line will crash if test_chromoting_client_ is already destroyed.
> See my comment in .h

Done.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_fixture.cc:74:
base::ResetAndReturn(&host_message_received_callback_).Run(message);
On 2015/07/23 19:36:55, Sergey Ulanov wrote:
> Do we need to reset the callback? Will we ever need to receive more than one
> message?

Done.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_fixture.cc:74:
base::ResetAndReturn(&host_message_received_callback_).Run(message);
On 2015/07/23 19:36:55, Sergey Ulanov wrote:
> Do you need to very type of the message, in case it belongs to a different
> extension?

Acknowledged.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_fixture.cc:74:
base::ResetAndReturn(&host_message_received_callback_).Run(message);
On 2015/07/23 20:26:56, joedow wrote:
> To add on to Sergey's comments, you could get several messages before you see
> the one you want (e.g. you can get addwindow and removewindow messages for a
> splash screen before the message for the main window arrives) so you don't
want
> to reset this here.

Acknowledged.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_connected_client_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_fixture.h:80:
base::WeakPtr<TestChromotingClient> test_chromoting_client_;
On 2015/07/23 19:36:55, Sergey Ulanov wrote:
> I don't think you really need this to be a weak pointer. This object is owned
by
> connection_helper_ owned by this class. So it's safe to make this a raw
pointer
> (i.e. TestChromotingClient*). But a better solution is to add an accessor in
the
> ConnectionHelper to get test client.

Done.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_latency_test_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_latency_test_fixture.cc:54: FAIL();
On 2015/07/23 20:26:56, joedow wrote:
> Have you run this through the try-bots yet?  I seem to recall Tony having a
> problem on one build flavor where having code after FAIL() (on line 49 in your
> case) would cause a build failure.  You should push this to the try-bots when
> you've updated the code just to make sure it builds OK for all flavors.

Yeah, I'll run try-bots on my next patch.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_latency_test_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_latency_test_fixture.h:85: virtual bool
CreateNewDocument() = 0;
On 2015/07/23 20:26:56, joedow wrote:
> I probably wouldn't name this 'CreateNewDocument' although that is what your
> derived class will use it for.  I am thinking it should be something like
> 'PrepareApplicationForTesting' or 'SetUpRunningApplication' (or similar).  The
> idea is that a derived fixture would implement this method by performing
certain
> actions that prepares the connected app for testing.  

Done.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_latency_test_fixture.h:123: //
AppRemotingConnectionHelper own its lifetime.
On 2015/07/23 20:26:56, joedow wrote:
> nit: reword this: "Used to maintain a reference to the TestVideoRenderer
> instance while it exists."

Done.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_latency_test_fixture.h:127: //
AppRemotingConnectionHelper own its lifetime.
On 2015/07/23 20:26:56, joedow wrote:
> nit: reword this: "Used to maintain a reference to the TestChromotingClient
> instance while it exists."

Acknowledged.

https://codereview.chromium.org/1253613002/diff/1/remoting/test/test_chromoti...
File remoting/test/test_chromoting_client.h (right):

https://codereview.chromium.org/1253613002/diff/1/remoting/test/test_chromoti...
remoting/test/test_chromoting_client.h:76: base::WeakPtr<TestChromotingClient>
GetWeakPtr() {
On 2015/07/23 19:36:55, Sergey Ulanov wrote:
> I don't think you really need this - see my other comments.
> In general it's better to avoid weak pointers when possible (and also
> ref-counted objects).

Done.

[joedow, 2015-07-24 13:50:58]

Almost there!  Please address the comments and fix the try-bot errors.

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
File remoting/test/app_remoting_connected_client_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
remoting/test/app_remoting_connected_client_fixture.cc:103: timer_->Stop();
Do you want to reset host_message_received_callback_ here since it has served
its purpose by this point?

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
File remoting/test/app_remoting_latency_test_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
remoting/test/app_remoting_latency_test_fixture.cc:52: LOG(ERROR) << "New
document could not be created.";
Please fix error message, we don't know if creating a new document is what the
derived fixture attempted to do.

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
File remoting/test/app_remoting_latency_test_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
remoting/test/app_remoting_latency_test_fixture.h:79: }
This should be defined in the cc file.  Trivial setter/getters are ok in the
header but other function implementations should go in the cc file.

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
remoting/test/app_remoting_latency_test_fixture.h:123: // exists
nit: add period to the end of the comment.

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/rgb_value.h
File remoting/test/rgb_value.h (right):

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/rgb_value...
remoting/test/rgb_value.h:13: // Black is the default color for tests.
Not sure what the value in this comment is for, I think it should be moved up a
line and state that the default c'tor will initialize the value to black. 
Whether it is used for tests or not is a usage detail independent of the
definition of the struct.

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

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/test_chro...
remoting/test/test_chromoting_client.cc:136:
connection_error_code_(protocol::OK) {}
Can you run clang-format, IIRC the trailing curly brace should be on the next
line

[liaoyuke, 2015-07-25 01:39:12]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2015-07-25 01:39:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1253613002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1253613002/80001

[liaoyuke, 2015-07-25 01:39:24]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2015-07-25 01:39:25]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2015-07-25 01:39:25]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[liaoyuke, 2015-07-25 01:43:49]

Hey Joe, Sergey, Anand,

I've addressed feedback from Joe, and fixed all error message. Please take
another look.

Thank you very much!

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
File remoting/test/app_remoting_connected_client_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
remoting/test/app_remoting_connected_client_fixture.cc:103: timer_->Stop();
On 2015/07/24 13:50:58, joedow wrote:
> Do you want to reset host_message_received_callback_ here since it has served
> its purpose by this point?

Done.

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
File remoting/test/app_remoting_latency_test_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
remoting/test/app_remoting_latency_test_fixture.cc:52: LOG(ERROR) << "New
document could not be created.";
On 2015/07/24 13:50:58, joedow wrote:
> Please fix error message, we don't know if creating a new document is what the
> derived fixture attempted to do.

I don't think so, the error message should come from the unittests, but I'll
re-try and fix it.

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
File remoting/test/app_remoting_latency_test_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
remoting/test/app_remoting_latency_test_fixture.h:79: }
On 2015/07/24 13:50:58, joedow wrote:
> This should be defined in the cc file.  Trivial setter/getters are ok in the
> header but other function implementations should go in the cc file.

Done.

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/app_remot...
remoting/test/app_remoting_latency_test_fixture.h:123: // exists
On 2015/07/24 13:50:58, joedow wrote:
> nit: add period to the end of the comment.

Done.

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/rgb_value.h
File remoting/test/rgb_value.h (right):

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/rgb_value...
remoting/test/rgb_value.h:13: // Black is the default color for tests.
On 2015/07/24 13:50:58, joedow wrote:
> Not sure what the value in this comment is for, I think it should be moved up
a
> line and state that the default c'tor will initialize the value to black. 
> Whether it is used for tests or not is a usage detail independent of the
> definition of the struct.

Done.

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

https://codereview.chromium.org/1253613002/diff/60001/remoting/test/test_chro...
remoting/test/test_chromoting_client.cc:136:
connection_error_code_(protocol::OK) {}
On 2015/07/24 13:50:58, joedow wrote:
> Can you run clang-format, IIRC the trailing curly brace should be on the next
> line

Yes, I didn't mean to make the change, this is an auto correction from
clang-format.

[joedow, 2015-07-25 02:13:34]

https://codereview.chromium.org/1253613002/diff/80001/remoting/test/app_remot...
File remoting/test/app_remoting_latency_test_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/80001/remoting/test/app_remot...
remoting/test/app_remoting_latency_test_fixture.cc:52: LOG(ERROR) << "New
document could not be created.";
Please fix the error message here, the base class does not know what the
subclass is doing to prepare the application for testing.  It is fine to log the
error here, you need to make the message make sense regardless of what the
subclass that implements the method needs to do.

https://codereview.chromium.org/1253613002/diff/80001/remoting/test/app_remot...
File remoting/test/app_remoting_latency_test_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/80001/remoting/test/app_remot...
remoting/test/app_remoting_latency_test_fixture.h:83: // Create a fresh document
for test.
Can you fix the comment here, you don't know in the base class what the derived
class needs to do to set up for a test case.  I'd make this generic and say
"Used to ensure the application under test is ready for testing." or similar.

[liaoyuke, 2015-07-27 17:22:50]

Hey Joe, Sergey, Anand,

Please take a look to see if you have any additional comments.

Thank you very much!

https://codereview.chromium.org/1253613002/diff/80001/remoting/test/app_remot...
File remoting/test/app_remoting_latency_test_fixture.cc (right):

https://codereview.chromium.org/1253613002/diff/80001/remoting/test/app_remot...
remoting/test/app_remoting_latency_test_fixture.cc:52: LOG(ERROR) << "New
document could not be created.";
On 2015/07/25 02:13:34, joedow wrote:
> Please fix the error message here, the base class does not know what the
> subclass is doing to prepare the application for testing.  It is fine to log
the
> error here, you need to make the message make sense regardless of what the
> subclass that implements the method needs to do.

Sorry, misunderstood the error message, I thought you were talking about the try
bot errors. Done!

https://codereview.chromium.org/1253613002/diff/80001/remoting/test/app_remot...
File remoting/test/app_remoting_latency_test_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/80001/remoting/test/app_remot...
remoting/test/app_remoting_latency_test_fixture.h:83: // Create a fresh document
for test.
On 2015/07/25 02:13:34, joedow wrote:
> Can you fix the comment here, you don't know in the base class what the
derived
> class needs to do to set up for a test case.  I'd make this generic and say
> "Used to ensure the application under test is ready for testing." or similar.

Done.

[joedow, 2015-07-27 17:30:12]

https://codereview.chromium.org/1253613002/diff/100001/remoting/test/app_remo...
File remoting/test/app_remoting_connection_helper.h (right):

https://codereview.chromium.org/1253613002/diff/100001/remoting/test/app_remo...
remoting/test/app_remoting_connection_helper.h:52: TestChromotingClient*
GetTestChromotingClient() { return client_.get(); }
This should use the unix_hacker_style() naming convention:
https://www.chromium.org/developers/coding-style

Simple accessors should generally be the only inline functions. These should be
named unix_hacker_style().

This is consistent with the host_stub and input_stub methods above.

[liaoyuke, 2015-07-27 17:40:45]

Hey Joe, Sergey, Anand,

Please take another look.

Thank you very much!

https://codereview.chromium.org/1253613002/diff/100001/remoting/test/app_remo...
File remoting/test/app_remoting_connection_helper.h (right):

https://codereview.chromium.org/1253613002/diff/100001/remoting/test/app_remo...
remoting/test/app_remoting_connection_helper.h:52: TestChromotingClient*
GetTestChromotingClient() { return client_.get(); }
On 2015/07/27 17:30:12, joedow wrote:
> This should use the unix_hacker_style() naming convention:
> https://www.chromium.org/developers/coding-style
> 
> Simple accessors should generally be the only inline functions. These should
be
> named unix_hacker_style().
> 
> This is consistent with the host_stub and input_stub methods above.

Done.

[joedow, 2015-07-27 17:44:01]

lgtm

[Sergey Ulanov, 2015-07-27 18:40:25]

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
File remoting/test/app_remoting_connection_helper.h (right):

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_connection_helper.h:50: // Returns the test
chromoting client.
nit: this comment doesn't really add anything. Remove it?

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_connection_helper.h:51: // NOTE: Caller should not
release the object.
s/release/delete/ or just remove the comment. This function returns a raw
pointer (and not scoped_ptr<>) which means that it doesn't give ownership of the
result, so even without this comment it's clear that the caller shouldn't delete
the result.

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
File remoting/test/app_remoting_latency_test_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_latency_test_fixture.h:8: #include <string>
nit: std::string is not used anywhere in this file

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_latency_test_fixture.h:72: void
SetHostMessageReceivedCallback(
Please either rename this to set_host_message_received_callback() or don't
inline it.

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_latency_test_fixture.h:73:
HostMessageReceivedCallback host_message_received_callback) {
nit: pass callback parameters as const ref

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_latency_test_fixture.h:84: virtual bool
PrepareApplicationForTesting() = 0;
It's better to make this method and GetApplicationDetails() protected, to make
it clear that they are overridden in child classes and not expected to be called
from outside.

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/rgb_value.h
File remoting/test/rgb_value.h (right):

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/rgb_valu...
remoting/test/rgb_value.h:18: int red;
what's the range for these values? Should these be uint8_t instead of int?

[liaoyuke, 2015-07-27 19:02:57]

Hey Joe, Sergey, Anand,

Please take another look to see if you have any additional comments.

Thank you very much!

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
File remoting/test/app_remoting_connection_helper.h (right):

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_connection_helper.h:50: // Returns the test
chromoting client.
On 2015/07/27 18:40:24, Sergey Ulanov wrote:
> nit: this comment doesn't really add anything. Remove it?

Done.

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_connection_helper.h:51: // NOTE: Caller should not
release the object.
On 2015/07/27 18:40:24, Sergey Ulanov wrote:
> s/release/delete/ or just remove the comment. This function returns a raw
> pointer (and not scoped_ptr<>) which means that it doesn't give ownership of
the
> result, so even without this comment it's clear that the caller shouldn't
delete
> the result.

Done.

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
File remoting/test/app_remoting_latency_test_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_latency_test_fixture.h:8: #include <string>
On 2015/07/27 18:40:24, Sergey Ulanov wrote:
> nit: std::string is not used anywhere in this file

Done.

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_latency_test_fixture.h:72: void
SetHostMessageReceivedCallback(
On 2015/07/27 18:40:24, Sergey Ulanov wrote:
> Please either rename this to set_host_message_received_callback() or don't
> inline it.

Done.

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_latency_test_fixture.h:73:
HostMessageReceivedCallback host_message_received_callback) {
On 2015/07/27 18:40:24, Sergey Ulanov wrote:
> nit: pass callback parameters as const ref

Done.

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_latency_test_fixture.h:84: virtual bool
PrepareApplicationForTesting() = 0;
On 2015/07/27 18:40:24, Sergey Ulanov wrote:
> It's better to make this method and GetApplicationDetails() protected, to make
> it clear that they are overridden in child classes and not expected to be
called
> from outside.

Yes, I think they are already protected.

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/rgb_value.h
File remoting/test/rgb_value.h (right):

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/rgb_valu...
remoting/test/rgb_value.h:18: int red;
On 2015/07/27 18:40:24, Sergey Ulanov wrote:
> what's the range for these values? Should these be uint8_t instead of int?

Done.

[Sergey Ulanov, 2015-07-27 20:08:05]

lgtm

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
File remoting/test/app_remoting_latency_test_fixture.h (right):

https://codereview.chromium.org/1253613002/diff/120001/remoting/test/app_remo...
remoting/test/app_remoting_latency_test_fixture.h:84: virtual bool
PrepareApplicationForTesting() = 0;
On 2015/07/27 19:02:57, liaoyuke wrote:
> On 2015/07/27 18:40:24, Sergey Ulanov wrote:
> > It's better to make this method and GetApplicationDetails() protected, to
make
> > it clear that they are overridden in child classes and not expected to be
> called
> > from outside.
> 
> Yes, I think they are already protected.

Sorry I missed that

[liaoyuke, 2015-07-27 21:12:52]

The CQ bit was checked by [email protected]

[liaoyuke, 2015-07-27 21:12:53]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected],
[email protected]
Link to the patchset: https://codereview.chromium.org/1253613002/#ps160001
(title: ""Changed cstdint to stdint.h to compile on mac"")

[commit-bot: I haz the power, 2015-07-27 21:14:34]

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

[commit-bot: I haz the power, 2015-07-27 22:42:53]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-07-27 22:43:51]

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