Use a mocked compositor for unit tests

ui::TestCompositor::TestCompositor() constructor has been changed to take a a CompositorDelegate* as a parameter.  It can be NULL to get the same behaviour as before.  But some tests actually require ScheduleDraw() to cause a draw to happen, in order to test that their real delegate is doing its job.  Also, we move the compositor_factory from views::Widget and aura::Desktop to ui::Compositor.

BUG=101091
TEST=bots stay green

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107103

[danakj, 2011-10-13 16:04:49]

cheers.

[sky, 2011-10-13 19:51:59]

http://codereview.chromium.org/8240006/diff/4018/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8240006/diff/4018/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:236: '../views/widget/widget.h',
Ugh. I don't like pulling in widget here. How about we create a
CompositorFactory that can be swapped in.

http://codereview.chromium.org/8240006/diff/4018/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

http://codereview.chromium.org/8240006/diff/4018/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_unittest.cc:10: #include
"chrome/test/base/test_launcher_utils.h"
content shouldn't depend on chrome or views.

[Paweł Hajdan Jr., 2011-10-13 20:08:13]

http://codereview.chromium.org/8240006/diff/4018/chrome/test/base/in_process_...
File chrome/test/base/in_process_browser_test.cc (right):

http://codereview.chromium.org/8240006/diff/4018/chrome/test/base/in_process_...
chrome/test/base/in_process_browser_test.cc:146:
test_launcher_utils::OverrideGLImplementation(
Do you also want things to apply to content? If so, you should probably use
BrowserTestBase.

[danakj, 2011-10-14 15:15:13]

Thanks for the feedback.

1) Moved the compositor_factory_ from views::Widget and aura::Desktop to
ui::Compositor.
- Removes some code duplication.
- No views dependency from the RWH unit test.
- Doesn't add a dependency to aura either.

2) Moved the setup from in_process_browser_test.cc to browser_test_base.cc to
catch tests in content/ as well.

http://codereview.chromium.org/8240006/diff/4018/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8240006/diff/4018/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:236: '../views/widget/widget.h',
On 2011/10/13 19:52:00, sky wrote:
> Ugh. I don't like pulling in widget here. How about we create a
> CompositorFactory that can be swapped in.

moved the factory methods to ui::Compositor instead of views::Widget and
aura::Desktop.  So this is not needed now.

http://codereview.chromium.org/8240006/diff/4018/chrome/test/base/in_process_...
File chrome/test/base/in_process_browser_test.cc (right):

http://codereview.chromium.org/8240006/diff/4018/chrome/test/base/in_process_...
chrome/test/base/in_process_browser_test.cc:146:
test_launcher_utils::OverrideGLImplementation(
On 2011/10/13 20:08:13, Paweł Hajdan Jr. wrote:
> Do you also want things to apply to content? If so, you should probably use
> BrowserTestBase.

Yes I should, thanks !

http://codereview.chromium.org/8240006/diff/4018/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

http://codereview.chromium.org/8240006/diff/4018/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_unittest.cc:10: #include
"chrome/test/base/test_launcher_utils.h"
On 2011/10/13 19:52:00, sky wrote:
> content shouldn't depend on chrome or views.

Oh this isn't even needed now, removed.

http://codereview.chromium.org/8240006/diff/4018/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_unittest.cc:268:
views::Widget::set_compositor_factory_for_testing(&TestCreateCompositor);
Moved the factory to ui::Compositor.

http://codereview.chromium.org/8240006/diff/4018/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8240006/diff/4018/ui/aura/desktop.h#newcode80
ui/aura/desktop.h:80: }
Since this is in ui::Compositor now, it can just use those and we don't need two
copies of this code.  Removing this one.

[sky, 2011-10-14 17:07:16]

http://codereview.chromium.org/8240006/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

http://codereview.chromium.org/8240006/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_unittest.cc:245: #if
defined(VIEWS_COMPOSITOR)
I was wrong in where I put some of this code. It should live in the various
TestSuite subclasses. That way it covers all tests in an exe rather having to
execute it per test. This comment applies to all the various tests you've had to
plug this code into.

[jonathan.backer, 2011-10-14 17:53:17]

http://codereview.chromium.org/8240006/diff/15001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8240006/diff/15001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:239: '../ui/gfx/compositor/test_texture.h',
Shouldn't these be pulled in from a dep on compositor.gyp?

http://codereview.chromium.org/8240006/diff/15001/chrome/test/gpu/webgl_confo...
File chrome/test/gpu/webgl_conformance_tests.cc (left):

http://codereview.chromium.org/8240006/diff/15001/chrome/test/gpu/webgl_confo...
chrome/test/gpu/webgl_conformance_tests.cc:22: #else
you sure about this?

[danakj, 2011-10-14 19:51:38]

http://codereview.chromium.org/8240006/diff/15001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8240006/diff/15001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:239: '../ui/gfx/compositor/test_texture.h',
On 2011/10/14 17:53:17, jonathan.backer wrote:
> Shouldn't these be pulled in from a dep on compositor.gyp?

Should it? It doesn't seem to be typical from what I saw.

http://codereview.chromium.org/8240006/diff/15001/chrome/test/gpu/webgl_confo...
File chrome/test/gpu/webgl_conformance_tests.cc (left):

http://codereview.chromium.org/8240006/diff/15001/chrome/test/gpu/webgl_confo...
chrome/test/gpu/webgl_conformance_tests.cc:22: #else
On 2011/10/14 17:53:17, jonathan.backer wrote:
> you sure about this?

discussed offline: these used to be used to turn on EGL on windows here for the
tests.  it was removed in a previous commit (revision 92155).

http://codereview.chromium.org/8240006/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

http://codereview.chromium.org/8240006/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_unittest.cc:245: #if
defined(VIEWS_COMPOSITOR)
On 2011/10/14 17:07:16, sky wrote:
> I was wrong in where I put some of this code. It should live in the various
> TestSuite subclasses. That way it covers all tests in an exe rather having to
> execute it per test. This comment applies to all the various tests you've had
to
> plug this code into.

done. does it look right/did i get the relevant ones?

[sky, 2011-10-14 20:52:41]

LGTM

http://codereview.chromium.org/8240006/diff/17002/chrome/test/base/chrome_tes...
File chrome/test/base/chrome_test_suite.cc (right):

http://codereview.chromium.org/8240006/diff/17002/chrome/test/base/chrome_tes...
chrome/test/base/chrome_test_suite.cc:175: #if defined(OS_LINUX)
#elif

http://codereview.chromium.org/8240006/diff/17002/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/8240006/diff/17002/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor.h:146: static ui::Compositor*
(*compositor_factory())(
newline between 145 and 146.

[danakj, 2011-10-17 14:52:13]

http://codereview.chromium.org/8240006/diff/17002/chrome/test/base/chrome_tes...
File chrome/test/base/chrome_test_suite.cc (right):

http://codereview.chromium.org/8240006/diff/17002/chrome/test/base/chrome_tes...
chrome/test/base/chrome_test_suite.cc:175: #if defined(OS_LINUX)
On 2011/10/14 20:52:41, sky wrote:
> #elif

Done.

http://codereview.chromium.org/8240006/diff/17002/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/8240006/diff/17002/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor.h:146: static ui::Compositor*
(*compositor_factory())(
On 2011/10/14 20:52:41, sky wrote:
> newline between 145 and 146.

Done.

[Paweł Hajdan Jr., 2011-10-18 10:04:32]

I think this new approach still fails to make the switches apply for content/
right?

[danakj, 2011-10-18 13:16:57]

On 2011/10/18 10:04:32, Paweł Hajdan Jr. wrote:
> I think this new approach still fails to make the switches apply for content/
> right?

Right, I can add it to content_test_suite.cc also.

[Paweł Hajdan Jr., 2011-10-18 13:21:34]

On 2011/10/18 13:16:57, danakj wrote:
> On 2011/10/18 10:04:32, Paweł Hajdan Jr. wrote:
> > I think this new approach still fails to make the switches apply for
content/
> > right?
> 
> Right, I can add it to content_test_suite.cc also.

Let's find some way to reduce the duplication. Could you extract the switch code
to something that can be used in all places the CL uses it? Like
ApplyTestGLSettings or something like that.

[danakj, 2011-10-18 13:47:21]

On 2011/10/18 13:21:34, Paweł Hajdan Jr. wrote:
> On 2011/10/18 13:16:57, danakj wrote:
> > On 2011/10/18 10:04:32, Paweł Hajdan Jr. wrote:
> > > I think this new approach still fails to make the switches apply for
> content/
> > > right?
> > 
> > Right, I can add it to content_test_suite.cc also.
> 
> Let's find some way to reduce the duplication. Could you extract the switch
code
> to something that can be used in all places the CL uses it? Like
> ApplyTestGLSettings or something like that.

I'm not sure where higher up it would belong.  It only applies to some tests -
ones which create native widgets.  Do you have some place in mind?

[Paweł Hajdan Jr., 2011-10-18 13:51:54]

On 2011/10/18 13:47:21, danakj wrote:
> On 2011/10/18 13:21:34, Paweł Hajdan Jr. wrote:
> > On 2011/10/18 13:16:57, danakj wrote:
> > > On 2011/10/18 10:04:32, Paweł Hajdan Jr. wrote:
> > > > I think this new approach still fails to make the switches apply for
> > content/
> > > > right?
> > > 
> > > Right, I can add it to content_test_suite.cc also.
> > 
> > Let's find some way to reduce the duplication. Could you extract the switch
> code
> > to something that can be used in all places the CL uses it? Like
> > ApplyTestGLSettings or something like that.
> 
> I'm not sure where higher up it would belong.  It only applies to some tests -
> ones which create native widgets.  Do you have some place in mind?

It seems that kUseGL and the other thing is declared in src/ui, right? I suggest
src/ui/test then or something like that. Just avoid duplicating this logic in
all tests because you'll inevitable forget some, or in the future someone will
forget to update some of them.

[danakj, 2011-10-18 14:49:47]

On 2011/10/18 13:51:54, Paweł Hajdan Jr. wrote:
> On 2011/10/18 13:47:21, danakj wrote:
> > On 2011/10/18 13:21:34, Paweł Hajdan Jr. wrote:
> > > On 2011/10/18 13:16:57, danakj wrote:
> > > > On 2011/10/18 10:04:32, Paweł Hajdan Jr. wrote:
> > > > > I think this new approach still fails to make the switches apply for
> > > content/
> > > > > right?
> > > > 
> > > > Right, I can add it to content_test_suite.cc also.
> > > 
> > > Let's find some way to reduce the duplication. Could you extract the
switch
> > code
> > > to something that can be used in all places the CL uses it? Like
> > > ApplyTestGLSettings or something like that.
> > 
> > I'm not sure where higher up it would belong.  It only applies to some tests
-
> > ones which create native widgets.  Do you have some place in mind?
> 
> It seems that kUseGL and the other thing is declared in src/ui, right? I
suggest
> src/ui/test then or something like that. Just avoid duplicating this logic in
> all tests because you'll inevitable forget some, or in the future someone will
> forget to update some of them.

Is this what you're looking for?

Added:
- ui/gfx/test/gfx_test_utils.h
- ui/gfx/test/gfx_test_utils.cc

Edited:
- chrome/chrome_tests.gypi
- chrome/test/base/chrome_test_suite.cc
- content/content_tests.gypi
- content/test/content_test_suite.cc
- ui/aura/test/test_suite.cc
- ui/gfx/compositor/compositor.gyp
- views/run_all_unittests.cc

[Paweł Hajdan Jr., 2011-10-19 08:49:37]

http://codereview.chromium.org/8240006/diff/32027/ui/gfx/test/gfx_test_utils.cc
File ui/gfx/test/gfx_test_utils.cc (right):

http://codereview.chromium.org/8240006/diff/32027/ui/gfx/test/gfx_test_utils....
ui/gfx/test/gfx_test_utils.cc:22:
EXPECT_TRUE(!CommandLine::ForCurrentProcess()->HasSwitch(switches::kUseGL));
Please convert it so that the function returns bool and uses WARN_UNUSED_RESULT.
This makes it easier to propagate failure info up.

http://codereview.chromium.org/8240006/diff/32027/ui/gfx/test/gfx_test_utils.h
File ui/gfx/test/gfx_test_utils.h (right):

http://codereview.chromium.org/8240006/diff/32027/ui/gfx/test/gfx_test_utils....
ui/gfx/test/gfx_test_utils.h:5: #ifndef UI_GFX_GFX_TEST_UTILS_H_
I'd prefer just ui/test.

http://codereview.chromium.org/8240006/diff/32027/ui/gfx/test/gfx_test_utils....
ui/gfx/test/gfx_test_utils.h:12: void SetupTestCompositor();
Comment here please.

http://codereview.chromium.org/8240006/diff/32027/ui/gfx/test/gfx_test_utils....
ui/gfx/test/gfx_test_utils.h:16: 
nit: Remove empty line.

[danakj, 2011-10-19 13:55:48]

http://codereview.chromium.org/8240006/diff/32027/ui/gfx/test/gfx_test_utils.cc
File ui/gfx/test/gfx_test_utils.cc (right):

http://codereview.chromium.org/8240006/diff/32027/ui/gfx/test/gfx_test_utils....
ui/gfx/test/gfx_test_utils.cc:22:
EXPECT_TRUE(!CommandLine::ForCurrentProcess()->HasSwitch(switches::kUseGL));
On 2011/10/19 08:49:37, Paweł Hajdan Jr. wrote:
> Please convert it so that the function returns bool and uses
WARN_UNUSED_RESULT.
> This makes it easier to propagate failure info up.

So if you have GL set to something already the test is not going to work on
Linux.  I believe the test should just fail at this point, because this is a
finding a conflict in the code.  And I don't see you handling this with some
sort of fallback path, do you?

[jonathan.backer, 2011-10-20 22:19:59]

Taking over from Dana.

I trimmed this down and tried to move unnecessary/contentious parts to separate
CLs (not all up for review --- no need to TAL; I'll put them up for review once
I know they're safe/necessary).

http://codereview.chromium.org/8364010/
http://codereview.chromium.org/8366009/
http://codereview.chromium.org/8364012/

[piman, 2011-10-25 18:06:51]

Couple of comments while trying to look at
http://code.google.com/p/chromium/issues/detail?id=101477

http://codereview.chromium.org/8240006/diff/53001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8240006/diff/53001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:66:
'../ui/gfx/compositor/compositor.gyp:compositor_test_support',
I think this is problematic: compositor_test_support pulls webkit_support which
conflicts with renderer (they both define webkit "support" functions).

http://codereview.chromium.org/8240006/diff/53001/ui/gfx/compositor/composito...
File ui/gfx/compositor/compositor.gyp (right):

http://codereview.chromium.org/8240006/diff/53001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor.gyp:119: 'compositor.cc',
Why are we compiling these twice? Also, won't we have a potential conflict if we
link both compositor and compositor_test_support? Wouldn't it make more sense to
depend on compositor?

[jonathan.backer, 2011-10-25 20:10:09]

http://codereview.chromium.org/8240006/diff/53001/ui/gfx/compositor/composito...
File ui/gfx/compositor/compositor.gyp (right):

http://codereview.chromium.org/8240006/diff/53001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor.gyp:119: 'compositor.cc',
The idea is to avoid pulling in the actual implementations. AFAIK, most of the
places where this is used, we're using a mock compositor. There are one or two
exceptions (aura_test_shell?) where it is used to initialize WK and use a real
compositor. These use cases depend on both compositor and
compositor_test_support.

I'd like to keep the separate, because it may be possible to lose the WK
dependency for test if we can get a test Layer implementation.

[piman, 2011-10-25 20:31:12]

On Tue, Oct 25, 2011 at 1:10 PM, <backer@chromium.org> wrote:

>
> http://codereview.chromium.**org/8240006/diff/53001/ui/gfx/**
>
compositor/compositor.gyp<http://codereview.chromium.org/8240006/diff/53001/ui/gfx/compositor/compositor.gyp>
> File ui/gfx/compositor/compositor.**gyp (right):
>
> http://codereview.chromium.**org/8240006/diff/53001/ui/gfx/**
>
compositor/compositor.gyp#**newcode119<http://codereview.chromium.org/8240006/diff/53001/ui/gfx/compositor/compositor.gyp#newcode119>
> ui/gfx/compositor/compositor.**gyp:119: 'compositor.cc',
> The idea is to avoid pulling in the actual implementations. AFAIK, most
> of the places where this is used, we're using a mock compositor. There
> are one or two exceptions (aura_test_shell?) where it is used to
> initialize WK and use a real compositor. These use cases depend on both
> compositor and compositor_test_support.
>
> I'd like to keep the separate, because it may be possible to lose the WK
> dependency for test if we can get a test Layer implementation.
>

When I created that target, the whole point was to be able to pull the WK
dependency in a way that was convenient (i.e. not add a bunch of ifdefs
everywhere).
Also, as I mentioned above, you can't depend on this and renderer at the
same time and expect it to work (webkit_support is mutually exclusive with
it).
Also, I'm really surprised you can link both compositor and this, since it
duplicates objects (especially in the shared build), that are compiled with
different flags (some with COMPOSITOR_IMPLEMENTATION and some without). That
violate the "one definition rule", and I think that's wrong.

I'm not sure I understand which problem we're trying to solve by "avoid
pulling in the actual implementations".
Even if that's something that's desirable, we should do it by having a
target that has compositor.cc and layer.cc and another target that has the
implementations.


>
>
http://codereview.chromium.**org/8240006/<http://codereview.chromium.org/8240...
>