Move test-only ContextFactory implementations out of production targets

Move test-only ContextFactory implementations out of production targets

This moves (Test|Default)ContextFactory and associated setup out of the
production compositor target into the test-only compositor_test_support library.
This removes the need for us to link in fake/mock classes into the prod build
and cleans up several pieces of confusing static state. Code that wants to use
one of the test ContextFactory implementations now has to call either
ui::InitializeContextFactoryForTests() or, if it's depending on content and
using aura, content::ImageTransportFactory::InitializeForUnitTests() and pass
in a ContextFactory implementation. Content-based tests get the latter for free
through content::BrowserTestBase.

R=piman,sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231867

[piman, 2013-10-26 00:38:35]

LGTM, thanks!

https://codereview.chromium.org/45963003/diff/80001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/45963003/diff/80001/ui/compositor/compositor....
ui/compositor/compositor.cc:351: if (g_context_factory)
nit: no need for the test any more.
(I wonder if we should even have to reset g_context_factory any more, the
embedder should do it already)

https://codereview.chromium.org/45963003/diff/80001/ui/compositor/compositor....
ui/compositor/compositor.cc:356: << "The ContextFactory should not outlive the
compositor thread.";
This DCHECK actually will never hit. Mind removing while you're here?

[sky, 2013-10-28 15:15:41]

LGTM

[jamesr, 2013-10-29 23:59:46]

The trybots have revealed a number of issues with tests.  I think I've addressed
them, but the changes probably deserve another look.  The basic issue is that
tests were passing due to a fairly random set of calls to modify static test and
production state.  I've separate the test-only static state from the production
static state and made the calls stricter, which required mucking with a number
of test harnesses.

The new contract is that if you want to use test contexts, you have to call
ui::InitializeContextFactoryForTests(bool) before anyone calls
ui::Compositor::Initialize().  You are only allowed to call
ui::InitializeContextFactoryForTests() so we can try to keep a handle on init
paths.  You have to call ui::TerminateContextFactoryForTests() after calling
ui::Compositor::Terminate().  In between the
ui::InitializeContextFactoryForTests() and Terminate...() calls, you can make as
many ui::Compositor::Initialize() / ui::Compositor::Terminate() calls as you
like.

To accomplish this required adding setup code in lots of places:

ash::test::AshTestHelper::SetUp() / TearDown() take care of most of the tests
that use ash
aura::test::AuraTestHelper::SetUp() / TearDown() take care of most of the tests
that use aura
main() in ui/aura/bench/bench_main.cc and ui/aura/demo/demo_main.cc take care of
the aura_bench and aura_demo binaries

then there are a number of other test harnesses elsewhere that needed handling
because they specifically initialized either aura or ash without going through
the test harnesses above:


DesktopNotificationsTest::SetUp () / TearDown() explicitly create/destroy
ash::Shell::Instances and so have initialization code
ViewEventTestBase is used by a number of tests and initializes an
ash::Test::TestShell / ash::ShellInstances, so it has initialization code
content/browser/aura/software_output_device_ozone_unittest explicitly does
compositor init/shutdown so it has setup code

I'm relying on the trybots to tell me if I've missed any targets, but searching
for ui::Compositor::Initialize() and the code that can an initialize aura::Env
tells me I've probably got everything.

PTAL, if interested.

[sky, 2013-10-30 00:06:55]

SLGTM

[jamesr, 2013-10-30 00:30:39]

https://codereview.chromium.org/45963003/diff/500001/ui/base/ime/input_method...
File ui/base/ime/input_method_linux_x11.h (left):

https://codereview.chromium.org/45963003/diff/500001/ui/base/ime/input_method...
ui/base/ime/input_method_linux_x11.h:51: TextInputClient* focused) OVERRIDE;
whoops, not intentional. i'll remove before landing

https://codereview.chromium.org/45963003/diff/500001/ui/base/ime/linux/linux_...
File ui/base/ime/linux/linux_input_method_context.h (left):

https://codereview.chromium.org/45963003/diff/500001/ui/base/ime/linux/linux_...
ui/base/ime/linux/linux_input_method_context.h:20: struct CompositionText;
ditto

[piman, 2013-10-30 04:47:10]

LGTM except for one thing (that looks unrelated to this patch).

https://codereview.chromium.org/45963003/diff/560001/ui/compositor/reflector.h
File ui/compositor/reflector.h (left):

https://codereview.chromium.org/45963003/diff/560001/ui/compositor/reflector....
ui/compositor/reflector.h:12: class COMPOSITOR_EXPORT Reflector
Really? It's used outside of compositor/ (in particular there's an
implementation in content/).

[jamesr, 2013-10-30 05:52:19]

This is actually required. Subclasses are exported, but the base class had
no symbols to export since it's all inline. Marking it exported causes link
errors on windows due to refs from compositor_test_support.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-10-30 05:54:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/45963003/560001

[commit-bot: I haz the power, 2013-10-30 08:59:33]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-30 16:08:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/45963003/560001

[commit-bot: I haz the power, 2013-10-30 17:33:15]

Change committed as 231867