Added new driver for chromoting.

Refactored remoting targets to support app remoting and chromoting. Built a new driver to support chromoting.

BUG=

Committed: https://crrev.com/61931fe6c013ff289f33e3a32ada502deb2bf620
Cr-Commit-Position: refs/heads/master@{#333840}

[tonychun, 2015-06-08 17:03:57]

tonychun@google.com changed reviewers:
+ anandc@chromium.org, joedow@chromium.org, sergeyu@chromium.org

[tonychun, 2015-06-08 17:03:58]

Adding new driver for Chromoting.

This addition refactors remoting .gyp files to support and build a new driver
for Chromoting. 
- "remoting_test_driver_common.gyp" holds a common lib that both
chromoting_test.gyp and app_remoting_test.gyp use.
- "chromoting_test.gyp" builds the chromoting driver, which will eventually
measure performance aspects of Chromoting.
- "app_remoting_test.gyp" has been refactored to continue supporting App
Remoting.

This current build PASSES all try bots.

[joedow, 2015-06-08 17:20:05]

Added some coments.  Can you makje sure the change description has been updated?
 It looks like it is "Refactored switches for both drivers to take command line
parameters" which is no longer accurate.

Thanks,
Joe

https://codereview.chromium.org/1164143002/diff/80001/remoting/app_remoting_t...
File remoting/app_remoting_test.gyp (right):

https://codereview.chromium.org/1164143002/diff/80001/remoting/app_remoting_t...
remoting/app_remoting_test.gyp:18:
'<(DEPTH)/remoting/remoting_test_driver_common.gyp:remoting_test_driver_common',
Instead of using <(DEPTH) here, you probably want ".." so it matches the other
dependency.

https://codereview.chromium.org/1164143002/diff/80001/remoting/chromoting_tes...
File remoting/chromoting_test.gyp (right):

https://codereview.chromium.org/1164143002/diff/80001/remoting/chromoting_tes...
remoting/chromoting_test.gyp:15: # An external version of the test driver tool
which includes minimal tests
Was this copied from the app remoting test driver?  I'm not sure if this comment
applies for chromoting.

https://codereview.chromium.org/1164143002/diff/80001/remoting/remoting_all.gyp
File remoting/remoting_all.gyp (right):

https://codereview.chromium.org/1164143002/diff/80001/remoting/remoting_all.g...
remoting/remoting_all.gyp:15:
'../remoting/chromoting_test.gyp:chromoting_test_driver',
If you are going to order this alphabetically, can you bring up the two app
remoting targets from the bottom of this block as well?

https://codereview.chromium.org/1164143002/diff/80001/remoting/test/chromotin...
File remoting/test/chromoting_test_driver.cc (right):

https://codereview.chromium.org/1164143002/diff/80001/remoting/test/chromotin...
remoting/test/chromoting_test_driver.cc:4: #include <string>
add newline between comment and include statement.

https://codereview.chromium.org/1164143002/diff/80001/remoting/test/chromotin...
remoting/test/chromoting_test_driver.cc:27: // client should not interfere with
another test client.
Can you reword this comment to describe the constraint a bit more.  The
intention is to say that the tests may require access to the same client so you
run one at a time because of shared resource contention.

https://codereview.chromium.org/1164143002/diff/80001/remoting/test/chromotin...
remoting/test/chromoting_test_driver.cc:36: } else {
no need for an else clause here.

[tonychun, 2015-06-08 18:28:46]

Made changes to code for consistency and clarity.

- Changed
'<(DEPTH)/remoting/remoting_test_driver_common.gyp:remoting_test_driver_common'
to '../remoting/remoting_test_driver_common.gyp:remoting_test_driver_common' to
be consistent.
- Changed chromoting_test.gyp file comment to correctly describe the driver
- Alphabetized remoting_all.gyp targets
- Reformatted code in chromoting_test_driver.cc (added space, reworded comment
for running tests one at a time, and removed else case)

[joedow, 2015-06-08 18:35:21]

Couple more comments...

https://codereview.chromium.org/1164143002/diff/100001/remoting/app_remoting_...
File remoting/app_remoting_test.gyp (right):

https://codereview.chromium.org/1164143002/diff/100001/remoting/app_remoting_...
remoting/app_remoting_test.gyp:18:
'../remoting/remoting_test_driver_common.gyp:remoting_test_driver_common',
can you fix the ordering in this array (remoting_test_driver_common should
appear under proto, sorry for not seeing that last time).

https://codereview.chromium.org/1164143002/diff/100001/remoting/chromoting_te...
File remoting/chromoting_test.gyp (right):

https://codereview.chromium.org/1164143002/diff/100001/remoting/chromoting_te...
remoting/chromoting_test.gyp:15: # A chromoting version of the common test
driver tool
what is the common test driver tool?

https://codereview.chromium.org/1164143002/diff/100001/remoting/remoting_test...
File remoting/remoting_test_driver_common.gyp (left):

https://codereview.chromium.org/1164143002/diff/100001/remoting/remoting_test...
remoting/remoting_test_driver_common.gyp:13: 
Any reason this newline was removed?

https://codereview.chromium.org/1164143002/diff/100001/remoting/test/chromoti...
File remoting/test/chromoting_test_driver.cc (right):

https://codereview.chromium.org/1164143002/diff/100001/remoting/test/chromoti...
remoting/test/chromoting_test_driver.cc:28: // client should not interfere with
another test client.
Can you make this comment more clear about why you don't want to run tests in
parallel?

[Sergey Ulanov, 2015-06-08 18:57:37]

https://codereview.chromium.org/1164143002/diff/100001/remoting/chromoting_te...
File remoting/chromoting_test.gyp (right):

https://codereview.chromium.org/1164143002/diff/100001/remoting/chromoting_te...
remoting/chromoting_test.gyp:1: # Copyright 2015 The Chromium Authors. All
rights reserved.
Do we really need to add this extra .gyp file? Why not just add the new target
in remoting_test.gypi?

I also think app_remoting_test.gyp should be replaced with .gypi or just moved
inside remoting_test.gypi.

In general there is a cost to each extra .gyp file - they slow down gyp
processing. So it's better to avoid adding new .gyp files when possible. 
Also it requires some code duplication (e.g. include statement), and not
consistent with other targets we have in remoting/ .

https://codereview.chromium.org/1164143002/diff/100001/remoting/chromoting_te...
remoting/chromoting_test.gyp:10: 'target_defaults': {
Why do you need this?

https://codereview.chromium.org/1164143002/diff/100001/remoting/remoting_test...
File remoting/remoting_test_driver_common.gyp (right):

https://codereview.chromium.org/1164143002/diff/100001/remoting/remoting_test...
remoting/remoting_test_driver_common.gyp:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
I don't think you need a separate .gyp file for this. Just the target in
remoting_test.gypi

https://codereview.chromium.org/1164143002/diff/100001/remoting/test/chromoti...
File remoting/test/chromoting_test_driver.cc (right):

https://codereview.chromium.org/1164143002/diff/100001/remoting/test/chromoti...
remoting/test/chromoting_test_driver.cc:40: return 0;
This doesn't actually run any tests. Is this intentional?

[tonychun, 2015-06-08 20:35:59]

Made alphabetical changes to dependencies
Clarified comment on chromoting version of the common driver
Removed target_defaults in chromoting_test.gyp

https://codereview.chromium.org/1164143002/diff/100001/remoting/app_remoting_...
File remoting/app_remoting_test.gyp (right):

https://codereview.chromium.org/1164143002/diff/100001/remoting/app_remoting_...
remoting/app_remoting_test.gyp:18:
'../remoting/remoting_test_driver_common.gyp:remoting_test_driver_common',
On 2015/06/08 18:35:20, joedow wrote:
> can you fix the ordering in this array (remoting_test_driver_common should
> appear under proto, sorry for not seeing that last time).

Done.

https://codereview.chromium.org/1164143002/diff/100001/remoting/chromoting_te...
File remoting/chromoting_test.gyp (right):

https://codereview.chromium.org/1164143002/diff/100001/remoting/chromoting_te...
remoting/chromoting_test.gyp:1: # Copyright 2015 The Chromium Authors. All
rights reserved.
On 2015/06/08 18:57:37, Sergey Ulanov wrote:
> Do we really need to add this extra .gyp file? Why not just add the new target
> in remoting_test.gypi?
> 
> I also think app_remoting_test.gyp should be replaced with .gypi or just moved
> inside remoting_test.gypi.
> 
> In general there is a cost to each extra .gyp file - they slow down gyp
> processing. So it's better to avoid adding new .gyp files when possible. 
> Also it requires some code duplication (e.g. include statement), and not
> consistent with other targets we have in remoting/ .

I completely agree. This change would optimize gyp processing time.

However, I would like to take care of this issue in a separate CL because the
objective of this CL is to get a driver up and running with how I refactored the
.gyp files.

I will have the next CL focused on making that change.

https://codereview.chromium.org/1164143002/diff/100001/remoting/chromoting_te...
remoting/chromoting_test.gyp:10: 'target_defaults': {
On 2015/06/08 18:57:37, Sergey Ulanov wrote:
> Why do you need this?

I thought, by default, every .gyp file requires a target_defaults key. It seems
like this is not the case, so I can remove it.

https://codereview.chromium.org/1164143002/diff/100001/remoting/chromoting_te...
remoting/chromoting_test.gyp:15: # A chromoting version of the common test
driver tool
On 2015/06/08 18:35:20, joedow wrote:
> what is the common test driver tool?

It is the common lib in remoting_all.gyp. I will restate the comment to make
this more clear.

https://codereview.chromium.org/1164143002/diff/100001/remoting/remoting_test...
File remoting/remoting_test_driver_common.gyp (right):

https://codereview.chromium.org/1164143002/diff/100001/remoting/remoting_test...
remoting/remoting_test_driver_common.gyp:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/06/08 18:57:37, Sergey Ulanov wrote:
> I don't think you need a separate .gyp file for this. Just the target in
> remoting_test.gypi

Yes I agree. I will take care of this in the next CL.

https://codereview.chromium.org/1164143002/diff/100001/remoting/test/chromoti...
File remoting/test/chromoting_test_driver.cc (right):

https://codereview.chromium.org/1164143002/diff/100001/remoting/test/chromoti...
remoting/test/chromoting_test_driver.cc:28: // client should not interfere with
another test client.
On 2015/06/08 18:35:20, joedow wrote:
> Can you make this comment more clear about why you don't want to run tests in
> parallel?

This is an old comment. I should have changed it.

https://codereview.chromium.org/1164143002/diff/100001/remoting/test/chromoti...
remoting/test/chromoting_test_driver.cc:40: return 0;
On 2015/06/08 18:57:37, Sergey Ulanov wrote:
> This doesn't actually run any tests. Is this intentional?

Yes. My first step was to just have a driver up and running. I don't want to put
too much onto one CL.

[tonychun, 2015-06-09 01:13:42]

Put all driver targets into remoting_test.gypi to optimize gyp processing time.
Corrected routing issues to targets due to refactoring remoting code.

[tonychun, 2015-06-09 01:13:43]

Put all driver targets into remoting_test.gypi to optimize gyp processing time.
Corrected routing issues to targets due to refactoring remoting code.

[joedow, 2015-06-09 02:45:49]

lgtm

https://codereview.chromium.org/1164143002/diff/140001/remoting/remoting_test...
File remoting/remoting_test.gypi (right):

https://codereview.chromium.org/1164143002/diff/140001/remoting/remoting_test...
remoting/remoting_test.gypi:123: # A chromoting version of
remoting_test_driver_common
nit: you can probably just remove this comment, it doesn't really provide much
value.

[joedow, 2015-06-09 02:45:52]

lgtm

lgtm

[Sergey Ulanov, 2015-06-09 20:31:04]

lgtm

[tonychun, 2015-06-09 22:42:13]

The CQ bit was checked by tonychun@google.com

[commit-bot: I haz the power, 2015-06-09 22:44:39]

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

[commit-bot: I haz the power, 2015-06-09 23:21:02]

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

[commit-bot: I haz the power, 2015-06-09 23:21:03]

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...)

[tonychun, 2015-06-09 23:45:34]

tonychun@google.com changed reviewers:
+ cjhopman@chromium.org
- anandc@chromium.org, joedow@chromium.org, sergeyu@chromium.org

[tonychun, 2015-06-09 23:45:34]

Made changes to ../build/gn_migration.gypi to give the correct path to the
target.

[cjhopman, 2015-06-09 23:55:09]

lgtm

[tonychun, 2015-06-09 23:55:29]

The CQ bit was checked by tonychun@google.com

[commit-bot: I haz the power, 2015-06-09 23:57:22]

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

[commit-bot: I haz the power, 2015-06-10 00:52:52]

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

[commit-bot: I haz the power, 2015-06-10 00:52:53]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tonychun, 2015-06-10 02:50:53]

Windows build failure is fixed. Made change to gn_migration.gypi file.

I missed one reference to the removed app_remoting_test.gyp file in a Windows
dependencies list. The build should work correctly now.

[anandc, 2015-06-10 05:06:52]

The CQ bit was checked by anandc@chromium.org

[anandc, 2015-06-10 05:06:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, cjhopman@chromium.org, joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/1164143002/#ps160001
(title: "Fix for windows")

[commit-bot: I haz the power, 2015-06-10 05:07:02]

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

[commit-bot: I haz the power, 2015-06-10 06:19:08]

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

[commit-bot: I haz the power, 2015-06-10 06:19:10]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tonychun, 2015-06-10 20:33:03]

The CQ bit was checked by tonychun@google.com

[tonychun, 2015-06-10 20:33:03]

The CQ bit was unchecked by tonychun@google.com

[tonychun, 2015-06-10 20:33:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, cjhopman@chromium.org, joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/1164143002/#ps180001
(title: "Windows return at FAIL() fix")

[tonychun, 2015-06-10 20:33:08]

The CQ bit was unchecked by tonychun@google.com

[tonychun, 2015-06-10 22:51:06]

The CQ bit was checked by tonychun@google.com

[commit-bot: I haz the power, 2015-06-10 22:51:39]

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

[commit-bot: I haz the power, 2015-06-10 23:28:48]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-10 23:29:33]

Patchset 10 (id:??) landed as
https://crrev.com/61931fe6c013ff289f33e3a32ada502deb2bf620
Cr-Commit-Position: refs/heads/master@{#333840}