aura: Clean up compositor initialization/destruction.

ui/compositor/compositor.cc

Index: ui/compositor/compositor.cc
diff --git a/ui/compositor/compositor.cc b/ui/compositor/compositor.cc
index c804e8d060c8842d00bcd26564895c053a16e7b2..309453b42d993d9b846f891b4300fb850f0776b1 100644
--- a/ui/compositor/compositor.cc
+++ b/ui/compositor/compositor.cc
@@ -50,10 +50,9 @@ enum SwapType {
   READPIXELS_SWAP,
 };
 
+bool g_compositor_initialized = false;
 base::Thread* g_compositor_thread = NULL;
 
-bool g_test_compositor_enabled = false;
-
 ui::ContextFactory* g_implicit_factory = NULL;
 ui::ContextFactory* g_context_factory = NULL;
 
@@ -77,39 +76,13 @@ class PendingSwap {
   DISALLOW_COPY_AND_ASSIGN(PendingSwap);
 };
 
-void SetupImplicitFactory() {
-  // We leak the implicit factory so that we don't race with the tear down of
-  // the gl_bindings.
-  DCHECK(!g_context_factory);
-  DCHECK(!g_implicit_factory);
-  if (g_test_compositor_enabled) {
-    g_implicit_factory = new ui::TestContextFactory;
-  } else {
-    DVLOG(1) << "Using DefaultContextFactory";
-    scoped_ptr<ui::DefaultContextFactory> instance(
-        new ui::DefaultContextFactory());
-    if (instance->Initialize())
-      g_implicit_factory = instance.release();
-  }
-  g_context_factory = g_implicit_factory;
-}
-
-void ResetImplicitFactory() {
-  if (!g_implicit_factory || g_context_factory != g_implicit_factory)
-    return;
-  delete g_implicit_factory;
-  g_implicit_factory = NULL;
-  g_context_factory = NULL;
-}
-
 }  // namespace
 
 namespace ui {
 
 // static
 ContextFactory* ContextFactory::GetInstance() {
-  if (!g_context_factory)
-    SetupImplicitFactory();
+  DCHECK(g_context_factory);
   return g_context_factory;
 }
 
@@ -180,6 +153,8 @@ DefaultContextFactory::OffscreenContextProviderForCompositorThread() {
 void DefaultContextFactory::RemoveCompositor(Compositor* compositor) {
 }
 
+bool DefaultContextFactory::UsesTestContexts() { return false; }
+
 scoped_ptr<WebKit::WebGraphicsContext3D>
 DefaultContextFactory::CreateContextCommon(Compositor* compositor,
                                            bool offscreen) {
@@ -248,6 +223,8 @@ TestContextFactory::OffscreenContextProviderForCompositorThread() {
 void TestContextFactory::RemoveCompositor(Compositor* compositor) {
 }
 
+bool TestContextFactory::UsesTestContexts() { return true; }
+
 Texture::Texture(bool flipped, const gfx::Size& size, float device_scale_factor)
     : size_(size),
       flipped_(flipped),
@@ -410,14 +387,18 @@ Compositor::Compositor(CompositorDelegate* delegate,
       next_draw_is_resize_(false),
       disable_schedule_composite_(false),
       compositor_lock_(NULL) {
+  DCHECK(g_compositor_initialized)
+      << "Compositor::Initialize must be called before creating a Compositor";
+
   root_web_layer_ = cc::Layer::Create();
   root_web_layer_->SetAnchorPoint(gfx::PointF(0.f, 0.f));
 
   CommandLine* command_line = CommandLine::ForCurrentProcess();
 
   cc::LayerTreeSettings settings;
-  settings.refresh_rate =
-      g_test_compositor_enabled ? kTestRefreshRate : kDefaultRefreshRate;
+  settings.refresh_rate = ContextFactory::GetInstance()->UsesTestContexts()
+                          ? kTestRefreshRate
+                          : kDefaultRefreshRate;
   settings.partial_swap_enabled =
       !command_line->HasSwitch(cc::switches::kUIDisablePartialSwap);
   settings.per_tile_painting_enabled =
@@ -452,6 +433,8 @@ Compositor::Compositor(CompositorDelegate* delegate,
 }
 
 Compositor::~Compositor() {
+  DCHECK(g_compositor_initialized);
+
   CancelCompositorLock();
   DCHECK(!compositor_lock_);
 
@@ -468,6 +451,42 @@ Compositor::~Compositor() {
 }
 
 // static
+void Compositor::InitializeContextFactoryForTests(bool allow_test_contexts) {
+  DCHECK(!g_context_factory) << "ContextFactory already initialized";
+  DCHECK(!g_implicit_factory) << "ContextFactory for tests already initialized";
+
+  bool use_test_contexts = true;
+
+  // Always use test contexts unless the disable command line flag is used.
+  CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kDisableTestCompositor))

[danakj, 2013/07/31 01:32:51]

I don't know if we really need to honor command line or IsRunningOnChromeOS here
anymore. Were those only for ImageTransportFactory::Initialize?

I'd kinda expect that in unit tests they should know what kind of contexts they
want, and we could just use the value passed into this method?

[piman, 2013/07/31 07:01:48]

The "test compositor" is an optimization for the bots, so that they run faster.
--disable-test-compositor is so that you can see what's happening when you need
to debug things, and that's definitely useful for UI people.
I forgot what led to the IsRunningOnChromeOS thing below, but on Chrome OS we
have a real GPU or at least a fast software implementation on VMs, so the
optimization doesn't bring us much. I suspect also it breaks some
ChromeOS-specific tests (e.g. login UI) because it disables accelerated
compositing in the renderer.

+    use_test_contexts = false;
+
+#if defined(OS_CHROMEOS)
+  // If the test is running on the chromeos envrionment (such as
+  // device or vm bots), always use real contexts.
+  if (base::chromeos::IsRunningOnChromeOS())
+    use_test_contexts = false;
+#endif
+
+  if (!allow_test_contexts)
+    use_test_contexts = false;
+
+  // We leak the implicit factory so that we don't race with the tear down of
+  // the gl_bindings.

[piman, 2013/07/31 07:01:48]

nit: I guess we don't leak it any more...

+  if (use_test_contexts) {
+    g_implicit_factory = new ui::TestContextFactory;
+  } else {
+    DVLOG(1) << "Using DefaultContextFactory";
+    scoped_ptr<ui::DefaultContextFactory> instance(
+        new ui::DefaultContextFactory());
+    if (instance->Initialize())
+      g_implicit_factory = instance.release();
+  }
+  g_context_factory = g_implicit_factory;
+}
+
+// static
 void Compositor::Initialize() {
 #if defined(OS_CHROMEOS)
   bool use_thread = !CommandLine::ForCurrentProcess()->HasSwitch(
@@ -483,6 +502,9 @@ void Compositor::Initialize() {
     g_compositor_thread = new base::Thread("Browser Compositor");
     g_compositor_thread->Start();
   }
+
+  DCHECK(!g_compositor_initialized) << "Compositor initialized twice.";
+  g_compositor_initialized = true;
 }
 
 // static
@@ -500,11 +522,24 @@ scoped_refptr<base::MessageLoopProxy> Compositor::GetCompositorMessageLoop() {
 
 // static
 void Compositor::Terminate() {
+  if (g_context_factory) {
+    if (g_implicit_factory) {
+      delete g_implicit_factory;
+      g_implicit_factory = NULL;
+    }
+    g_context_factory = NULL;
+  }
+
   if (g_compositor_thread) {
+    DCHECK(!g_context_factory)
+        << "The ContextFactory should not outlive the compositor thread.";
     g_compositor_thread->Stop();
     delete g_compositor_thread;
     g_compositor_thread = NULL;
   }
+
+  DCHECK(g_compositor_initialized) << "Compositor::Initialize() didn't happen";
+  g_compositor_initialized = false;
 }
 
 void Compositor::ScheduleDraw() {
@@ -751,27 +786,4 @@ void Compositor::NotifyEnd() {
                     OnCompositingEnded(this));
 }
 
-COMPOSITOR_EXPORT void SetupTestCompositor() {
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kDisableTestCompositor)) {
-    g_test_compositor_enabled = true;
-  }
-#if defined(OS_CHROMEOS)
-  // If the test is running on the chromeos envrionment (such as
-  // device or vm bots), use the real compositor.
-  if (base::chromeos::IsRunningOnChromeOS())
-    g_test_compositor_enabled = false;
-#endif
-  ResetImplicitFactory();
-}
-
-COMPOSITOR_EXPORT void DisableTestCompositor() {
-  ResetImplicitFactory();
-  g_test_compositor_enabled = false;
-}
-
-COMPOSITOR_EXPORT bool IsTestCompositorEnabled() {
-  return g_test_compositor_enabled;
-}
-
 }  // namespace ui