WIP Handle AwContents needing multiple live functors.

android_webview/browser/test/fake_window.cc

Index: android_webview/browser/test/fake_window.cc
diff --git a/android_webview/browser/test/fake_window.cc b/android_webview/browser/test/fake_window.cc
index a13be3376b1fe87673cca7bcb4fdac93a4cee3a9..00f1f9f38eb3bb7bb7f3983f9278b101de6b219a 100644
--- a/android_webview/browser/test/fake_window.cc
+++ b/android_webview/browser/test/fake_window.cc
@@ -41,7 +41,6 @@ class FakeWindow::ScopedMakeCurrent {
 };
 
 FakeWindow::FakeWindow(BrowserViewRenderer* view,
-                       const DrawGLCallback& draw_gl,
                        WindowHooks* hooks,
                        gfx::Rect location)
     : view_(view),
@@ -49,8 +48,8 @@ FakeWindow::FakeWindow(BrowserViewRenderer* view,
       surface_size_(100, 100),
       location_(location),
       on_draw_hardware_pending_(false),
-      draw_gl_(draw_gl),
       context_current_(false),
+      functor_(nullptr),
       weak_ptr_factory_(this) {
   CheckCurrentlyOnUIThread();
   DCHECK(view_);
@@ -61,6 +60,7 @@ FakeWindow::FakeWindow(BrowserViewRenderer* view,
 
 FakeWindow::~FakeWindow() {
   CheckCurrentlyOnUIThread();
+  LOG(WARNING) << "XXX " << __PRETTY_FUNCTION__;
   if (render_thread_loop_) {
     base::WaitableEvent completion(true, false);
     render_thread_loop_->PostTask(
@@ -77,32 +77,34 @@ void FakeWindow::Detach() {
   view_->OnDetachedFromWindow();
 }
 
-void FakeWindow::RequestInvokeGL(bool wait_for_completion) {
+void FakeWindow::RequestInvokeGL(Functor* functor, bool wait_for_completion) {
   CheckCurrentlyOnUIThread();
   base::WaitableEvent completion(true, false);
   render_thread_loop_->PostTask(
       FROM_HERE,
-      base::Bind(&FakeWindow::ProcessFunctorOnRT, base::Unretained(this),
-                 wait_for_completion ? &completion : nullptr));
+      base::Bind(&FakeWindow::InvokeFunctorOnRT, base::Unretained(this),
+                 functor, wait_for_completion ? &completion : nullptr));
   if (wait_for_completion)
     completion.Wait();
 }
 
-void FakeWindow::ProcessFunctorOnRT(base::WaitableEvent* sync) {
+void FakeWindow::InvokeFunctorOnRT(Functor* functor,

[boliu, 2016/05/10 15:18:19]

why have both |functor_| and all these methods that take another functor? that
feels wrong

[Tobias Sargeant, 2016/05/10 16:01:17]

I guess functor_ should really be current_functor_; is only used to determine
where OnDrawHardware "draws" to.

Invokes and explicit draws know which functor they're associated to, which is
the reason for the functor parameters here.

[boliu, 2016/05/10 16:37:53]

But look like this method. You have |functor| arg, but using |functor_|. Which
one are you supposed to use? Just get rid of this whole thing. Either pass it
in, or have it be a member, don't mix them.

[Tobias Sargeant, 2016/05/13 13:23:56]

Ok. It's always passed in now, but as far as I can tell that means needing
another method on WindowHooks to get the current functor (which is what I did).

+                                   base::WaitableEvent* sync) {
   CheckCurrentlyOnRT();
-  AwDrawGLInfo process_info;
-  process_info.version = kAwDrawGLInfoVersion;
-  process_info.mode = AwDrawGLInfo::kModeProcess;
-
-  hooks_->WillProcessOnRT();
-  {
-    ScopedMakeCurrent make_current(this);
-    draw_gl_.Run(&process_info);
-  }
-  hooks_->DidProcessOnRT();
+  ScopedMakeCurrent make_current(this);
+  functor_->Invoke(sync, hooks_);
+}
 
-  if (sync)
-    sync->Signal();
+void FakeWindow::SetDrawFunctor(Functor* functor) {
+  functor_ = functor;
+  functor_->Attach(this);
+}
+
+void FakeWindow::RequestDrawGL(Functor* functor) {
+  CheckCurrentlyOnUIThread();
+  render_thread_loop_->PostTask(FROM_HERE,
+                                base::Bind(&FakeWindow::ProcessDrawOnRT,
+                                           base::Unretained(this), functor));
 }
 
 void FakeWindow::PostInvalidate() {
@@ -118,6 +120,7 @@ void FakeWindow::PostInvalidate() {
 void FakeWindow::OnDrawHardware() {
   CheckCurrentlyOnUIThread();
   DCHECK(on_draw_hardware_pending_);
+  DCHECK(functor_);
   on_draw_hardware_pending_ = false;
 
   view_->PrepareToDraw(gfx::Vector2d(), location_);
@@ -130,42 +133,25 @@ void FakeWindow::OnDrawHardware() {
     base::WaitableEvent completion(true, false);
     render_thread_loop_->PostTask(
         FROM_HERE, base::Bind(&FakeWindow::DrawFunctorOnRT,
-                              base::Unretained(this), &completion));
+                              base::Unretained(this), functor_, &completion));
     completion.Wait();
   }
 }
 
-void FakeWindow::DrawFunctorOnRT(base::WaitableEvent* sync) {
+void FakeWindow::ProcessSyncOnRT(Functor* functor, base::WaitableEvent* sync) {
   CheckCurrentlyOnRT();
-  // Ok to access UI functions until sync is signalled.
-  gfx::Rect location = location_;
-  {
-    AwDrawGLInfo process_info;
-    process_info.version = kAwDrawGLInfoVersion;
-    process_info.mode = AwDrawGLInfo::kModeSync;
-
-    hooks_->WillSyncOnRT();
-    draw_gl_.Run(&process_info);
-    hooks_->DidSyncOnRT();
-  }
-  sync->Signal();
-
-  AwDrawGLInfo draw_info;
-  draw_info.version = kAwDrawGLInfoVersion;
-  draw_info.mode = AwDrawGLInfo::kModeDraw;
-  draw_info.clip_left = location.x();
-  draw_info.clip_top = location.y();
-  draw_info.clip_right = location.x() + location.width();
-  draw_info.clip_bottom = location.y() + location.height();
+  functor->Sync(location_, sync, hooks_);
+}
 
-  if (!hooks_->WillDrawOnRT(&draw_info))
-    return;
+void FakeWindow::ProcessDrawOnRT(Functor* functor) {
+  CheckCurrentlyOnRT();
+  ScopedMakeCurrent make_current(this);
+  functor->Draw(hooks_);
+}
 
-  {
-    ScopedMakeCurrent make_current(this);
-    draw_gl_.Run(&draw_info);
-  }
-  hooks_->DidDrawOnRT();
+void FakeWindow::DrawFunctorOnRT(Functor* functor, base::WaitableEvent* sync) {
+  ProcessSyncOnRT(functor, sync);
+  ProcessDrawOnRT(functor);
 }
 
 void FakeWindow::CheckCurrentlyOnUIThread() {
@@ -215,4 +201,69 @@ void FakeWindow::CheckCurrentlyOnRT() {
   DCHECK(rt_checker_.CalledOnValidSequencedThread());
 }
 
+Functor::Functor(RenderThreadManagerFactory render_thread_manager_factory)
+    : render_thread_manager_(render_thread_manager_factory.Run(this)) {

[boliu, 2016/05/10 15:18:19]

Hmm, is this safe? Is the object constructed far enough to be used as RTMClient?

Also factory callback seems overkill. Just have like a separate Init object that
sets the things that depend on this. You already have Attach, so why not replace
that with Init(RTM, FakeWindow)?

[Tobias Sargeant, 2016/05/10 16:01:17]

This is effectively the pattern we use with AwGLFunctor (and elsewhere), so it
should be just as OK here as it is there.
We assume that RenderThreadManagerClient pointer is available when RTM is
constructed, and stays constant (client_ should be const in RTM, I guess) so I
don't think that we can Init with an existing RTM* without changing that.

[boliu, 2016/05/10 16:37:53]

It's a technical concern. The object may not be fully constructed. So what
happens if RTM wants to use RTMClient in the constructor? RTMClient isn't fully
constructed, so could have corrupt members and whatnot. Or maybe vtable is not
set up (totally made that up)
No that's fine? I mean like:
functor = new FUnctor()
rtm = new RTM(functor)
functor.init(rtm)

[Tobias Sargeant, 2016/05/13 13:23:56]

Took this approach. I can't see any reason why this was any more dangerous than
the way we construct AwGLFunctor (and AwContents) where we pass in this as a
*Client pointer to a member that's being constructed. I agree that that means
that you probably don't want to use the client pointer in the constructor of
RenderThreadManager or BrowserViewRenderer though.

+  callback_ = base::Bind(&RenderThreadManager::DrawGL,
+                         base::Unretained(render_thread_manager_.get()));
+}
+
+Functor::~Functor() {}
+
+CompositorFrameConsumer* Functor::GetCompositorFrameConsumer() {
+  return render_thread_manager_.get();
+}
+
+void Functor::Attach(FakeWindow* window) {
+  window_ = window;
+}
+
+void Functor::Sync(const gfx::Rect& location,
+                   base::WaitableEvent* sync,

[boliu, 2016/05/10 15:18:19]

keep the sync event logic in FakeWindow

[Tobias Sargeant, 2016/05/10 16:01:17]

Could do, but then callback_ has to be available to FakeWindow. It seemed more
consistent to have it all in Functor or all in FakeWindow, but doing the
committed_location_ setting argued against having Sync in FakeWindow.

[boliu, 2016/05/10 16:37:53]

Hmm, don't see why that's needed?

+                   WindowHooks* hooks) {
+  committed_location_ = location;
+  AwDrawGLInfo sync_info;
+  sync_info.version = kAwDrawGLInfoVersion;
+  sync_info.mode = AwDrawGLInfo::kModeSync;
+  hooks->WillSyncOnRT();
+  LOG(WARNING) << "XXX " << __PRETTY_FUNCTION__
+               << " render_thread_manager_ = " << render_thread_manager_.get();
+  callback_.Run(&sync_info);
+  hooks->DidSyncOnRT();
+  sync->Signal();
+}
+
+void Functor::Draw(WindowHooks* hooks) {
+  AwDrawGLInfo draw_info;
+  draw_info.version = kAwDrawGLInfoVersion;
+  draw_info.mode = AwDrawGLInfo::kModeDraw;
+  draw_info.clip_left = committed_location_.x();
+  draw_info.clip_top = committed_location_.y();
+  draw_info.clip_right = committed_location_.x() + committed_location_.width();
+  draw_info.clip_bottom =
+      committed_location_.y() + committed_location_.height();
+  if (!hooks->WillDrawOnRT(&draw_info))
+    return;
+  callback_.Run(&draw_info);
+  hooks->DidDrawOnRT();
+}
+
+void Functor::Invoke(base::WaitableEvent* sync, WindowHooks* hooks) {
+  AwDrawGLInfo invoke_info;
+  invoke_info.version = kAwDrawGLInfoVersion;
+  invoke_info.mode = AwDrawGLInfo::kModeProcess;
+  hooks->WillProcessOnRT();
+  callback_.Run(&invoke_info);
+  hooks->DidProcessOnRT();
+  if (sync)
+    sync->Signal();
+}
+
+bool Functor::RequestInvokeGL(bool wait_for_completion) {
+  DCHECK(window_);
+  window_->RequestInvokeGL(this, wait_for_completion);
+  return true;
+}
+
+void Functor::DetachFunctorFromView() {}
+
 }  // namespace android_webview