Check for use_x11 before runnig desktop_capture_modules_tests on linux.

Check for use_x11 before runnig desktop_capture_modules_tests on linux.

The tests need "x11/shared_x_display.h" which is not included when use_x11 is false and we're on linux.

The problem is:

screen_capturer_integration_test.cc
 - requires ->
screen_drawer.h
 - requires ->
screen_drawer_linux.cc
 - requires ->
x11/shared_x_display.h
 which is not included when use_x11 is false.


BUG=None
NOTRY=True

Review-Url: https://codereview.webrtc.org/2684683003
Cr-Commit-Position: refs/heads/master@{#16529}
Committed: https://chromium.googlesource.com/external/webrtc/+/0d729b3039fe96b74e3efc80449c6a9cf28a04ba

[ehmaldonado_webrtc, 2017-02-08 17:29:43]

Description was changed from

==========
Check for use_x11 before runnig desktop_capture_modules_tests.


BUG=None
==========

to

==========
Check for use_x11 before runnig desktop_capture_modules_tests.

The tests need "x11/shared_x_display.h" which is not included when use_x11 is
false.

BUG=None
==========

[ehmaldonado_webrtc, 2017-02-08 17:29:43]

ehmaldonado@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[ehmaldonado_webrtc, 2017-02-08 17:30:11]

Please take a look :)
After this, all the tests compile downstream.

[kjellander_webrtc, 2017-02-09 07:11:35]

https://codereview.webrtc.org/2684683003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/BUILD.gn (right):

https://codereview.webrtc.org/2684683003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/BUILD.gn:40: if (use_x11 &&
rtc_desktop_capture_supported) {
use_x11 is only ever true on Linux
(https://cs.chromium.org/chromium/src/build/config/ui.gni?rcl=9539388cc0c6c98a...)
so this would make the test no longer be built for Windows and Mac, which is not
what we want, right?

[ehmaldonado_webrtc, 2017-02-09 13:20:27]

Patchset #1 (id:1) has been deleted

[ehmaldonado_webrtc, 2017-02-09 13:24:33]

On 2017/02/09 07:11:35, kjellander_webrtc wrote:
>
https://codereview.webrtc.org/2684683003/diff/20001/webrtc/modules/desktop_ca...
> File webrtc/modules/desktop_capture/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2684683003/diff/20001/webrtc/modules/desktop_ca...
> webrtc/modules/desktop_capture/BUILD.gn:40: if (use_x11 &&
> rtc_desktop_capture_supported) {
> use_x11 is only ever true on Linux
>
(https://cs.chromium.org/chromium/src/build/config/ui.gni?rcl=9539388cc0c6c98a...)
> so this would make the test no longer be built for Windows and Mac, which is
not
> what we want, right?

Right. PTAL.

[kjellander_webrtc, 2017-02-09 13:59:50]

kjellander@webrtc.org changed reviewers:
+ sergeyu@chromium.org

[kjellander_webrtc, 2017-02-09 13:59:51]

Sergey, we're trying to get these tests to play nice when X11 is disabled,  but
I'm not sure which is the best way. What do you suggest?

https://codereview.webrtc.org/2684683003/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/BUILD.gn (right):

https://codereview.webrtc.org/2684683003/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/BUILD.gn:40: if ((!is_linux || use_x11) &&
rtc_desktop_capture_supported) {
This is somewhat hard to read.
What about to change
https://cs.chromium.org/chromium/src/third_party/webrtc/webrtc.gni?rcl=a48313...
like:
rtc_desktop_capture_supported = is_win || is_mac || (is_linux && use_x11)
if the problem is that these tests are failing when X11 is not enabled?

[ehmaldonado_webrtc, 2017-02-09 14:01:30]

>
https://codereview.webrtc.org/2684683003/diff/40001/webrtc/modules/desktop_ca...
> webrtc/modules/desktop_capture/BUILD.gn:40: if ((!is_linux || use_x11) &&
> rtc_desktop_capture_supported) {
> This is somewhat hard to read.
> What about to change
>
https://cs.chromium.org/chromium/src/third_party/webrtc/webrtc.gni?rcl=a48313...
> like:
> rtc_desktop_capture_supported = is_win || is_mac || (is_linux && use_x11)
> if the problem is that these tests are failing when X11 is not enabled?

Yeah, the problem is that it'd also disable the tests in
desktop_capture_unittests, and I haven't had any problems with those so far.

[ehmaldonado_webrtc, 2017-02-09 14:02:50]

Description was changed from

==========
Check for use_x11 before runnig desktop_capture_modules_tests.

The tests need "x11/shared_x_display.h" which is not included when use_x11 is
false.

BUG=None
==========

to

==========
Check for use_x11 before runnig desktop_capture_modules_tests on linux.

The tests need "x11/shared_x_display.h" which is not included when use_x11 is
false and we're on linux.

The problem is:

screen_capturer_integration_test.cc
 - requires ->
screen_drawer.h
 - requires ->
screen_drawer_linux.cc
 - requires ->
x11/shared_x_display.h
 which is not included when use_x11 is false.


BUG=None
==========

[Sergey Ulanov, 2017-02-09 18:52:22]

https://codereview.webrtc.org/2684683003/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/BUILD.gn (right):

https://codereview.webrtc.org/2684683003/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/BUILD.gn:40: if ((!is_linux || use_x11) &&
rtc_desktop_capture_supported) {
On 2017/02/09 13:59:50, kjellander_webrtc wrote:
> This is somewhat hard to read.
> What about to change
>
https://cs.chromium.org/chromium/src/third_party/webrtc/webrtc.gni?rcl=a48313...
> like:
> rtc_desktop_capture_supported = is_win || is_mac || (is_linux && use_x11)
> if the problem is that these tests are failing when X11 is not enabled?

There is an issue with Chrome OS and Android. There are no screen capturers for
ChromeOS or Android in WebRTC repo, but screen capturing is still supported in
chrome, it's just implemented in chromium instead of in webrtc. These screen
capturers may still depend on on some classes currently in desktop_capturer
module (e.g. DesktopRegion), so we don't want to disable all unittests
completely. Currently rtc_desktop_capture_supported disables all tests, see
desktop_capture_unittests. 

I think we should use rtc_desktop_capture_supported to indicate that screen
capturer is implemented in WebRTC. Then it makes sense setting it to "is_win ||
is_mac || (is_linux && use_x11)", but desktop_capture_unittests should be
updated to handle it properly. Right now it's already hacked for android - see
is_android condition there.
I suggest updating desktop_capture_unittests target to always include all deps
and all tests except screen_capturer*unittest.cc. The screen_capturer* tests
should be enabled only when rtc_desktop_capture_supported=true.

[ehmaldonado_webrtc, 2017-02-09 20:52:39]

Patchset #3 (id:60001) has been deleted

[ehmaldonado_webrtc, 2017-02-09 20:56:20]

The CQ bit was checked by ehmaldonado@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 20:56:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[ehmaldonado_webrtc, 2017-02-09 20:56:50]

On 2017/02/09 18:52:22, Sergey Ulanov wrote:
>
https://codereview.webrtc.org/2684683003/diff/40001/webrtc/modules/desktop_ca...
> File webrtc/modules/desktop_capture/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2684683003/diff/40001/webrtc/modules/desktop_ca...
> webrtc/modules/desktop_capture/BUILD.gn:40: if ((!is_linux || use_x11) &&
> rtc_desktop_capture_supported) {
> On 2017/02/09 13:59:50, kjellander_webrtc wrote:
> > This is somewhat hard to read.
> > What about to change
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/webrtc.gni?rcl=a48313...
> > like:
> > rtc_desktop_capture_supported = is_win || is_mac || (is_linux && use_x11)
> > if the problem is that these tests are failing when X11 is not enabled?
> 
> There is an issue with Chrome OS and Android. There are no screen capturers
for
> ChromeOS or Android in WebRTC repo, but screen capturing is still supported in
> chrome, it's just implemented in chromium instead of in webrtc. These screen
> capturers may still depend on on some classes currently in desktop_capturer
> module (e.g. DesktopRegion), so we don't want to disable all unittests
> completely. Currently rtc_desktop_capture_supported disables all tests, see
> desktop_capture_unittests. 
> 
> I think we should use rtc_desktop_capture_supported to indicate that screen
> capturer is implemented in WebRTC. Then it makes sense setting it to "is_win
||
> is_mac || (is_linux && use_x11)", but desktop_capture_unittests should be
> updated to handle it properly. Right now it's already hacked for android - see
> is_android condition there.
> I suggest updating desktop_capture_unittests target to always include all deps
> and all tests except screen_capturer*unittest.cc. The screen_capturer* tests
> should be enabled only when rtc_desktop_capture_supported=true.

PTAL

[commit-bot: I haz the power, 2017-02-09 20:58:19]

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

[commit-bot: I haz the power, 2017-02-09 20:58:19]

Dry run: Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/17536)

[Sergey Ulanov, 2017-02-09 21:15:39]

lgtm

[ehmaldonado_webrtc, 2017-02-10 09:36:02]

Description was changed from

==========
Check for use_x11 before runnig desktop_capture_modules_tests on linux.

The tests need "x11/shared_x_display.h" which is not included when use_x11 is
false and we're on linux.

The problem is:

screen_capturer_integration_test.cc
 - requires ->
screen_drawer.h
 - requires ->
screen_drawer_linux.cc
 - requires ->
x11/shared_x_display.h
 which is not included when use_x11 is false.


BUG=None
==========

to

==========
Check for use_x11 before runnig desktop_capture_modules_tests on linux.

The tests need "x11/shared_x_display.h" which is not included when use_x11 is
false and we're on linux.

The problem is:

screen_capturer_integration_test.cc
 - requires ->
screen_drawer.h
 - requires ->
screen_drawer_linux.cc
 - requires ->
x11/shared_x_display.h
 which is not included when use_x11 is false.


BUG=None
NOTRY=True
==========

[ehmaldonado_webrtc, 2017-02-10 09:36:10]

The CQ bit was checked by ehmaldonado@webrtc.org

[commit-bot: I haz the power, 2017-02-10 09:36:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-10 09:38:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486719370085510,
"parent_rev": "38e9324e4e1da12ad1e8dd8451958c3b8b3f6fc4", "commit_rev":
"0d729b3039fe96b74e3efc80449c6a9cf28a04ba"}

[commit-bot: I haz the power, 2017-02-10 09:38:26]

Description was changed from

==========
Check for use_x11 before runnig desktop_capture_modules_tests on linux.

The tests need "x11/shared_x_display.h" which is not included when use_x11 is
false and we're on linux.

The problem is:

screen_capturer_integration_test.cc
 - requires ->
screen_drawer.h
 - requires ->
screen_drawer_linux.cc
 - requires ->
x11/shared_x_display.h
 which is not included when use_x11 is false.


BUG=None
NOTRY=True
==========

to

==========
Check for use_x11 before runnig desktop_capture_modules_tests on linux.

The tests need "x11/shared_x_display.h" which is not included when use_x11 is
false and we're on linux.

The problem is:

screen_capturer_integration_test.cc
 - requires ->
screen_drawer.h
 - requires ->
screen_drawer_linux.cc
 - requires ->
x11/shared_x_display.h
 which is not included when use_x11 is false.


BUG=None
NOTRY=True

Review-Url: https://codereview.webrtc.org/2684683003
Cr-Commit-Position: refs/heads/master@{#16529}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0d729b3039fe96b74e3efc804...
==========

[commit-bot: I haz the power, 2017-02-10 09:38:27]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/0d729b3039fe96b74e3efc804...