cc::OutputSurfaceClient::InitializeForGL

cc/trees/thread_proxy.cc

Index: cc/trees/thread_proxy.cc
diff --git a/cc/trees/thread_proxy.cc b/cc/trees/thread_proxy.cc
index f61e7feb73e826b4b2ae6067c29a6ffd0b6a41fa..a33bb9acd74e9ead4236aa044e57ded9a9e2b7f2 100644
--- a/cc/trees/thread_proxy.cc
+++ b/cc/trees/thread_proxy.cc
@@ -1164,13 +1164,26 @@ void ThreadProxy::InitializeOutputSurfaceOnImplThread(
 
   *success = layer_tree_host_impl_->InitializeRenderer(output_surface.Pass());
 
+  DidTryInitializeRendererOnImplThread(*success, offscreen_context_provider);
+
+  if (*success)
+    *capabilities = layer_tree_host_impl_->GetRendererCapabilities();
+
+  completion->Signal();
+}
+
+void ThreadProxy::DidTryInitializeRendererOnImplThread(
+    bool success,
+    scoped_refptr<ContextProvider> offscreen_context_provider) {
+  DCHECK(IsImplThread());
+
   if (offscreen_context_provider.get())
     offscreen_context_provider->BindToCurrentThread();
 
-  if (*success) {
-    *capabilities = layer_tree_host_impl_->GetRendererCapabilities();
+  if (success) {
     scheduler_on_impl_thread_->SetSwapBuffersCompleteSupported(
-        capabilities->using_swap_complete_callback);
+        layer_tree_host_impl_->GetRendererCapabilities()
+            .using_swap_complete_callback);
 
     OutputSurface* output_surface_ptr = layer_tree_host_impl_->output_surface();

[danakj, 2013/06/06 14:49:31]

This stuff is meant to happen when the output surface changes, but it did not
change here. Should these stay in the InitializeOutputSurfaceOnImplThread()
method?

[boliu, 2013/06/06 17:39:25]

Also expanding the question to SetSwapBuffersCompleteSupported above, even
though that could actually change with a new renderer.

I'm on the fence for this one.

On one hand, caps can change and there is no way to plumb the new values to main
thread in time, so there is a weak and brittle contract with webview that these
caps cannot change. (This is even happens to be true for
using_swap_complete_callback since the in-process context does not support it)
In this case, we should move these back into InitializeOutputSurfaceOnImplThread
and actually DCHECK here that nothing has changed.

On the other hand, there is this overall effort (feeling?) that for impl-side
painting, nothing on main thread should need to know about caps. Nulling
PrioritizedResourceManager was one step towards that but it's not complete. In
this case, it's fine to change these in case something does change.

Practically, it really doesn't matter for webview at this point as we just want
to get hardware up without having apply a bunch of uncommitted patches. Maybe
we'll tweak things like max_frames_pending later when we are ready to do perf
optimizations.

So...maybe just keep these here for "not diverging from init path"? Thoughts?

[danakj, 2013/06/06 17:53:39]

I really like the idea of DCHECK () << "You cant do this and this is why". With
a TODO to re-examine this when "settings.impl_side_painting" is on everywhere.
(Use the setting name exactly so we find it with grep when we go to delete this
setting one day).

     DCHECK(output_surface_ptr);
@@ -1191,8 +1204,6 @@ void ThreadProxy::InitializeOutputSurfaceOnImplThread(
   } else if (offscreen_context_provider.get()) {
     offscreen_context_provider->VerifyContexts();
   }
-
-  completion->Signal();
 }
 
 void ThreadProxy::FinishGLOnImplThread(CompletionEvent* completion) {