Modify unit tests for NativeDesktopMediaList to cover aura window capture.

Modify unit tests for NativeDesktopMediaList to cover aura window capture.

This unit tests CL is associated wiht CL
https://codereview.chromium.org/1808273002/.

Changes in NativeDesktopMediaList. h and NativeDesktopMediaList.cc honor the after landing comments in CL
https://codereview.chromium.org/1808273002/

BUG=581790, 289779, 590915
Committed: https://crrev.com/e5db3ea5297b94573a1147d9d21fb990179f5991
Cr-Commit-Position: refs/heads/master@{#386129}

[GeorgeZ, 2016-03-24 19:56:47]

Patchset #1 (id:1) has been deleted

[GeorgeZ, 2016-03-25 22:39:31]

gyzhou@chromium.org changed reviewers:
+ sergeyu@chromium.org

[GeorgeZ, 2016-03-25 22:39:31]

Sergey,

Could you take a look of this CL?

The unit tests skip associating native ID with aura ID and capturing aura
windows which are hard to mimic in the unit tests and I guess they are not key
parts of the work flow.

Thanks,

[GeorgeZ, 2016-03-29 23:52:59]

Ping Sergey.

[Sergey Ulanov, 2016-03-30 18:50:53]

I don't think this is the right approach to test this functionality. The tests
pass fake Aura windows to the prod code. This increases complexity, but the code
that's actually responsible for Aura windows is not being tested. I think it's
better to make the tests create actual Aura windows. That way you'd avoid any
test-only logic in NativeDesktopMediaList and increase test coverage.

[GeorgeZ, 2016-03-30 19:53:12]

On 2016/03/30 18:50:53, Sergey Ulanov wrote:
> I don't think this is the right approach to test this functionality. The tests
> pass fake Aura windows to the prod code. This increases complexity, but the
code
> that's actually responsible for Aura windows is not being tested. I think it's
> better to make the tests create actual Aura windows. That way you'd avoid any
> test-only logic in NativeDesktopMediaList and increase test coverage.

I will do investigation for not touching prod code for unit tests as your
suggested.
There are two major challenges here
1, views::DesktopWindowTreeHostWin::GetContentWindowForHWND() and  
 views::DesktopWindowTreeHostX11::GetContentWindowForXID() are os 
dependent. I need figure out how the fake system in unit tests
working with them.

2, I created fake browsers as aura windows for TabDesktopMediaList's unit tests.

However, I am not very sure whether ui::GrabWindowSnapshotAndScaleAsync() will
work with those fake browsers.

Since new approach as you suggested will be very different than the one in this
CL. 
I planned to close this CL now and create a new CL for new approach later.

[GeorgeZ, 2016-03-31 04:18:06]

On 2016/03/30 19:53:12, GeorgeZ wrote:
> On 2016/03/30 18:50:53, Sergey Ulanov wrote:
> > I don't think this is the right approach to test this functionality. The
tests
> > pass fake Aura windows to the prod code. This increases complexity, but the
> code
> > that's actually responsible for Aura windows is not being tested. I think
it's
> > better to make the tests create actual Aura windows. That way you'd avoid
any
> > test-only logic in NativeDesktopMediaList and increase test coverage.
> 
> I will do investigation for not touching prod code for unit tests as your
> suggested.
> There are two major challenges here
> 1, views::DesktopWindowTreeHostWin::GetContentWindowForHWND() and  
>  views::DesktopWindowTreeHostX11::GetContentWindowForXID() are os 
> dependent. I need figure out how the fake system in unit tests
> working with them.
> 
> 2, I created fake browsers as aura windows for TabDesktopMediaList's unit
tests.
> 
> However, I am not very sure whether ui::GrabWindowSnapshotAndScaleAsync() will
> work with those fake browsers.
> 
> Since new approach as you suggested will be very different than the one in
this
> CL. 
> I planned to close this CL now and create a new CL for new approach later.

for views::DesktopWindowTreeHostWin::GetContentWindowForHWND(),
it is defined in DesktopWindowTreeHostWin class and there is no unit tests
associated
with this class. The function is called in several classes. Those class either
have no
unit test or their unit test doesn't cover the function.
On the other hand, the class defined GetContentWindowForXID() has unit test
associated
with it.

[GeorgeZ, 2016-04-06 16:38:36]

Patchset #1 (id:20001) has been deleted

[GeorgeZ, 2016-04-06 16:38:45]

Patchset #1 (id:40001) has been deleted

[GeorgeZ, 2016-04-06 16:39:01]

Patchset #1 (id:60001) has been deleted

[GeorgeZ, 2016-04-06 21:43:05]

Patchset #2 (id:100001) has been deleted

[GeorgeZ, 2016-04-06 21:43:17]

Patchset #1 (id:80001) has been deleted

[GeorgeZ, 2016-04-06 22:49:27]

Patchset #1 (id:120001) has been deleted

[GeorgeZ, 2016-04-06 23:09:51]

Description was changed from

==========
Enhance Unit tests for NativeDesktopMediaList to cover aura thumbnails.

This unit tests CL is associated wiht CL
https://codereview.chromium.org/1808273002/.

BUG=581790, 289779, 590915
==========

to

==========
Modify unit tests for NativeDesktopMediaList to cover aura window capture.

This unit tests CL is associated wiht CL
https://codereview.chromium.org/1808273002/.

Changes in NativeDesktopMediaList. h and NativeDesktopMediaList.cc honor the
after landing comments in CL
https://codereview.chromium.org/1808273002/

BUG=581790, 289779, 590915
==========

[GeorgeZ, 2016-04-06 23:11:20]

Sergey,

The unit tests has been rewritten to follow your suggestion. Could you take a
look of this CL?

Thanks

[Sergey Ulanov, 2016-04-07 00:20:14]

Awesome stuff! Thanks for getting these new tests implemented!

Currently the tests always use FakeScreenCapturer and FakeWindowCapturer. We
still need them for non-aura platforms, but with aura it should be easy to test
with real window capturers. Can you please add a test that does it? It will just
need to verify that window gets added/removed when you create a views widget.

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:162: class
DesktopMediaListTest : public views::ViewsTestBase {
This should be NativeDesktopMediaListTest. Can you please rename it?

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:175:
webrtc::ScreenCapturer* screen_capturer = NULL;
nullptr

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:182: window_capturer_
= NULL;
nullptr

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:193: views::Widget*
CreateDesktopWidget() {
use std::unique_ptr<> for the result.

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:194: views::Widget*
widget = new views::Widget;
std::unique_ptr<>

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:216: window.id =
reinterpret_cast<DesktopMediaID::Id>(widget);
I think this should compile on non-windows platforms as well, so you don't need
the ifdef above.

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:232: bool
has_view_dialog = false) {
Style guide doesn't allow optional parameters.

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:445:
DCHECK_GE(kDefaultWindowCount, 2);
This can be replaced with a COMPILE_ASSERT()

[GeorgeZ, 2016-04-07 17:11:25]

On 2016/04/07 00:20:14, Sergey Ulanov wrote:
> Awesome stuff! Thanks for getting these new tests implemented!
> 
> Currently the tests always use FakeScreenCapturer and FakeWindowCapturer. We
> still need them for non-aura platforms, but with aura it should be easy to
test
> with real window capturers. Can you please add a test that does it? It will
just
> need to verify that window gets added/removed when you create a views widget.
> 

Sergey,

I am not sure I understand your suggestion correctly. I added a unit test for
RemoveAuraWindow. 
There was a unit test for AddAuraWindow. Aura windows added when I create a view
widget is 
covered by AddWindowsAndVerify() and AddAurawindow() and removed is covered by
RemoveAuraWindow.

Current work flow begins with webrtc window capturer providing a window list.
Then each window 
in the list is checked whether it is a aura window. Not all aura windows can be
in the list 
(such as minimized ones). Therefore, a window capturer to provide a window list
is always needed. 

I uploaded a patch based on you comment. 

Thanks

[GeorgeZ, 2016-04-07 17:36:43]

Sergey,

Forgot to publish draft.

Thanks,

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:162: class
DesktopMediaListTest : public views::ViewsTestBase {
On 2016/04/07 00:20:14, Sergey Ulanov wrote:
> This should be NativeDesktopMediaListTest. Can you please rename it?

Done.

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:175:
webrtc::ScreenCapturer* screen_capturer = NULL;
On 2016/04/07 00:20:14, Sergey Ulanov wrote:
> nullptr

Done.

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:182: window_capturer_
= NULL;
On 2016/04/07 00:20:14, Sergey Ulanov wrote:
> nullptr

Done.

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:193: views::Widget*
CreateDesktopWidget() {
On 2016/04/07 00:20:14, Sergey Ulanov wrote:
> use std::unique_ptr<> for the result.

Done.

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:194: views::Widget*
widget = new views::Widget;
On 2016/04/07 00:20:14, Sergey Ulanov wrote:
> std::unique_ptr<>

done.

What's the benefit of std::unique_ptr over scoped_ptr? Just std::unique_ptr is
in std library?

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:216: window.id =
reinterpret_cast<DesktopMediaID::Id>(widget);
On 2016/04/07 00:20:14, Sergey Ulanov wrote:
> I think this should compile on non-windows platforms as well, so you don't
need
> the ifdef above.

window.id = reinterpret_cast<DesktopMediaID::Id>(widget); for non-windows
platform leads to compile error

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:232: bool
has_view_dialog = false) {
On 2016/04/07 00:20:14, Sergey Ulanov wrote:
> Style guide doesn't allow optional parameters.

Done.

https://codereview.chromium.org/1828303003/diff/140001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:445:
DCHECK_GE(kDefaultWindowCount, 2);
On 2016/04/07 00:20:14, Sergey Ulanov wrote:
> This can be replaced with a COMPILE_ASSERT()

COMPILE_ASSERT() from
"chrome/installer/mini_installer/mini_string.h" is not working with windows OS.
All other definition of it seems in third-party. Which one do you think I should
use?

[GeorgeZ, 2016-04-07 17:36:59]

Patchset #2 (id:160001) has been deleted

[Sergey Ulanov, 2016-04-07 18:17:31]

The new tests use FakeWindowCapturer. This means you need to call
SetWindowList() to make sure that NativeDesktopMediaList knows about new window.
I suggest adding another test that uses real WindowCapturer, i.e. instead of
creating FakeWindowCapturer the test will call WindowCapturer::Create() to
create WindowsCapturer for the NativeDesktopMediaList. Since the tests create
real Views window the WindowCapturer should be able to discover it
automatically.

https://codereview.chromium.org/1828303003/diff/180001/chrome/browser/media/n...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/1828303003/diff/180001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:248:
desktop_widgets_[index].reset();
don't need this. Since it's a vector of scoped_ptr<> values the object will be
deleted automatically when you call erase() below.

https://codereview.chromium.org/1828303003/diff/180001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:255: }
check here that i < window_list_.size()

[GeorgeZ, 2016-04-07 22:17:46]

On 2016/04/07 18:17:31, Sergey Ulanov wrote:
> The new tests use FakeWindowCapturer. This means you need to call
> SetWindowList() to make sure that NativeDesktopMediaList knows about new
window.
> I suggest adding another test that uses real WindowCapturer, i.e. instead of
> creating FakeWindowCapturer the test will call WindowCapturer::Create() to
> create WindowsCapturer for the NativeDesktopMediaList. Since the tests create
> real Views window the WindowCapturer should be able to discover it
> automatically.
> 

Sergey, 

I tried to do as you suggested and there may have some issues. The real 
WindowCapturer will list not only my test aura windows but also other native 
windows and even other aura windows in the try-bot machine. The list also may 
change overtime. I only can know this list information after refresh() is done 
and by call GetSourceCount() and GetSource(). I don't know what are expected
for observer during the refresh() which is the major different from fake
capturer. 
I also don't know when I should call QuitMessageLoop() to terminate
starUpdating()
and refresh(). Do you have suggestions to address those issues?

Since I cannot know the window list before refresh() and have no control of try
bot, only thing I can verify is that the window list contains the test aura
 windows I created after refresh(). I guess this limit the value of real
capturer
Test.

Thanks,

[GeorgeZ, 2016-04-08 02:06:39]

On 2016/04/07 22:17:46, GeorgeZ wrote:
> On 2016/04/07 18:17:31, Sergey Ulanov wrote:
> > The new tests use FakeWindowCapturer. This means you need to call
> > SetWindowList() to make sure that NativeDesktopMediaList knows about new
> window.
> > I suggest adding another test that uses real WindowCapturer, i.e. instead of
> > creating FakeWindowCapturer the test will call WindowCapturer::Create() to
> > create WindowsCapturer for the NativeDesktopMediaList. Since the tests
create
> > real Views window the WindowCapturer should be able to discover it
> > automatically.
> > 
> 
> Sergey, 
> 
> I tried to do as you suggested and there may have some issues. The real 
> WindowCapturer will list not only my test aura windows but also other native 
> windows and even other aura windows in the try-bot machine. The list also may 
> change overtime. I only can know this list information after refresh() is done

> and by call GetSourceCount() and GetSource(). I don't know what are expected
> for observer during the refresh() which is the major different from fake
> capturer. 
> I also don't know when I should call QuitMessageLoop() to terminate
> starUpdating()
> and refresh(). Do you have suggestions to address those issues?
> 
> Since I cannot know the window list before refresh() and have no control of
try
> bot, only thing I can verify is that the window list contains the test aura
>  windows I created after refresh(). I guess this limit the value of real
> capturer
> Test.
> 
> Thanks,

Sergey,

I just did some tests in linux OS. Real desktopcapturer only can 
list/discover visible windows in Screen and cannot list/discover 
test aura widgets/windows. It is reasonable, since test aura widget
doesn't actually render anything in the screen.

Thanks,

[GeorgeZ, 2016-04-08 13:15:23]

Sergey,

A patch is uploaded based on your comments.

Thanks,

https://codereview.chromium.org/1828303003/diff/180001/chrome/browser/media/n...
File chrome/browser/media/native_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/1828303003/diff/180001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:248:
desktop_widgets_[index].reset();
On 2016/04/07 18:17:31, Sergey Ulanov wrote:
> don't need this. Since it's a vector of scoped_ptr<> values the object will be
> deleted automatically when you call erase() below.

Done.

https://codereview.chromium.org/1828303003/diff/180001/chrome/browser/media/n...
chrome/browser/media/native_desktop_media_list_unittest.cc:255: }
On 2016/04/07 18:17:30, Sergey Ulanov wrote:
> check here that i < window_list_.size()

Done.

[Sergey Ulanov, 2016-04-08 17:38:07]

On 2016/04/08 02:06:39, GeorgeZ wrote:
> Sergey,
> 
> I just did some tests in linux OS. Real desktopcapturer only can 
> list/discover visible windows in Screen and cannot list/discover 
> test aura widgets/windows. It is reasonable, since test aura widget
> doesn't actually render anything in the screen.
> 
> Thanks,

I see. I thought it would create real window. In that case LGTM.

[GeorgeZ, 2016-04-08 18:11:20]

The CQ bit was checked by gyzhou@chromium.org

[commit-bot: I haz the power, 2016-04-08 18:11:54]

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

[commit-bot: I haz the power, 2016-04-08 18:34:00]

Description was changed from

==========
Modify unit tests for NativeDesktopMediaList to cover aura window capture.

This unit tests CL is associated wiht CL
https://codereview.chromium.org/1808273002/.

Changes in NativeDesktopMediaList. h and NativeDesktopMediaList.cc honor the
after landing comments in CL
https://codereview.chromium.org/1808273002/

BUG=581790, 289779, 590915
==========

to

==========
Modify unit tests for NativeDesktopMediaList to cover aura window capture.

This unit tests CL is associated wiht CL
https://codereview.chromium.org/1808273002/.

Changes in NativeDesktopMediaList. h and NativeDesktopMediaList.cc honor the
after landing comments in CL
https://codereview.chromium.org/1808273002/

BUG=581790, 289779, 590915
==========

[commit-bot: I haz the power, 2016-04-08 18:34:01]

Committed patchset #3 (id:200001)

[commit-bot: I haz the power, 2016-04-08 18:35:14]

Description was changed from

==========
Modify unit tests for NativeDesktopMediaList to cover aura window capture.

This unit tests CL is associated wiht CL
https://codereview.chromium.org/1808273002/.

Changes in NativeDesktopMediaList. h and NativeDesktopMediaList.cc honor the
after landing comments in CL
https://codereview.chromium.org/1808273002/

BUG=581790, 289779, 590915
==========

to

==========
Modify unit tests for NativeDesktopMediaList to cover aura window capture.

This unit tests CL is associated wiht CL
https://codereview.chromium.org/1808273002/.

Changes in NativeDesktopMediaList. h and NativeDesktopMediaList.cc honor the
after landing comments in CL
https://codereview.chromium.org/1808273002/

BUG=581790, 289779, 590915

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

[commit-bot: I haz the power, 2016-04-08 18:35:15]

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