Switch PepperView to use asynchronous completion for Flush().

PepperView was requesting "optional" synchronous/asynchronous completion from Flush(), so as to be able to handle errors, which was not possible with the CompletionCallbackClosureAdapter, since the closure would not receive the result.

This CL replaces CompletionCallbackClosureAdapter with a PpCompletionCallback() function that accepts a base::Callback<void(int)> and returns a pp::CompletionCallback suitable for passing directly to Flush().  PepperView is also updated to use the new helper, and to expect Flush() to always complete asynchronously.

TEST=Chromoting client renders correctly.


Review URL: http://codereview.chromium.org/10010033

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@131304 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 25 insertions, 51 deletions

diff --git a/remoting/client/plugin/pepper_util.cc b/remoting/client/plugin/pepper_util.cc
index 013bba4..525d7d3 100644
--- a/remoting/client/plugin/pepper_util.cc
+++ b/remoting/client/plugin/pepper_util.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -10,10 +10,16 @@
 
 namespace remoting {
 
-void CompletionCallbackClosureAdapter(void* user_data, int32_t not_used) {
-  base::Closure* closure = reinterpret_cast<base::Closure*>(user_data);
-  closure->Run();
-  delete closure;
+static void CallbackAdapter(void* user_data, int32_t result) {
+  scoped_ptr<base::Callback<void(int)> > callback(
+      reinterpret_cast<base::Callback<void(int)>*>(user_data));
+  callback->Run(result);
+}
+
+pp::CompletionCallback PpCompletionCallback(
+    base::Callback<void(int)> callback) {
+  return pp::CompletionCallback(&CallbackAdapter,
+                                new base::Callback<void(int)>(callback));
 }
 
 }  // namespace remoting
diff --git a/remoting/client/plugin/pepper_util.h b/remoting/client/plugin/pepper_util.h
index 90dbeb3..80ab630 100644
--- a/remoting/client/plugin/pepper_util.h
+++ b/remoting/client/plugin/pepper_util.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -12,12 +12,10 @@
 
 namespace remoting {
 
-// Function for adapting a Chromium base::Closure to a PP_CompletionCallback
-// friendly function.  The base::Closure object should be a dynamically
-// allocated copy of the result from base::Bind(). It should be passed as
-// |user_data|.  This function will invoke base::Closure::Run() on
-// |user_data| when called, and then delete |user_data|.
-void CompletionCallbackClosureAdapter(void* user_data, int32_t not_used);
+// Adapts a base::Callback to a pp::CompletionCallback, which may be passed to
+// exactly one Pepper API. If the adapted callback is not used then a copy of
+// |callback| will be leaked, including references & passed values bound to it.
+pp::CompletionCallback PpCompletionCallback(base::Callback<void(int)> callback);
 
 }  // namespace remoting
 
diff --git a/remoting/client/plugin/pepper_view.cc b/remoting/client/plugin/pepper_view.cc
index 908ad56..6b67eac 100644
--- a/remoting/client/plugin/pepper_view.cc
+++ b/remoting/client/plugin/pepper_view.cc
@@ -318,45 +318,16 @@ void PepperView::FlushBuffer(const SkIRect& clip_area,
   }
 
   // Flush the updated areas to the screen.
-  scoped_ptr<base::Closure> task(
-      new base::Closure(
-          base::Bind(&PepperView::OnFlushDone, AsWeakPtr(), start_time,
-                     buffer)));
-
-  // Flag needs to be set here in order to get a proper error code for Flush().
-  // Otherwise Flush() will always return PP_OK_COMPLETIONPENDING and the error
-  // would be hidden.
-  //
-  // Note that we can also handle this by providing an actual callback which
-  // takes the result code. Right now everything goes to the task that doesn't
-  // result value.
-  pp::CompletionCallback pp_callback(&CompletionCallbackClosureAdapter,
-                                     task.get(),
-                                     PP_COMPLETIONCALLBACK_FLAG_OPTIONAL);
-  int error = graphics2d_.Flush(pp_callback);
-
-  // If Flush() returns asynchronously then release the task.
-  flush_pending_ = (error == PP_OK_COMPLETIONPENDING);
-  if (flush_pending_) {
-    ignore_result(task.release());
-  } else {
-    instance_->GetStats()->video_paint_ms()->Record(
-        (base::Time::Now() - start_time).InMilliseconds());
-
-    ReturnBuffer(buffer);
-
-    // Resume painting for the buffer that was previoulsy postponed because of
-    // pending flush.
-    if (merge_buffer_ != NULL) {
-      buffer = merge_buffer_;
-      merge_buffer_ = NULL;
-      FlushBuffer(merge_clip_area_, buffer, merge_region_);
-    }
-  }
+  int error = graphics2d_.Flush(
+      PpCompletionCallback(base::Bind(
+          &PepperView::OnFlushDone, AsWeakPtr(), start_time, buffer)));
+  CHECK(error == PP_OK_COMPLETIONPENDING);
+  flush_pending_ = true;
 }
 
 void PepperView::OnFlushDone(base::Time paint_start,
-                             pp::ImageData* buffer) {
+                             pp::ImageData* buffer,
+                             int result) {
   DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
   DCHECK(flush_pending_);
 
@@ -366,8 +337,7 @@ void PepperView::OnFlushDone(base::Time paint_start,
   flush_pending_ = false;
   ReturnBuffer(buffer);
 
-  // Resume painting for the buffer that was previoulsy postponed because of
-  // pending flush.
+  // If there is a buffer queued for rendering then render it now.
   if (merge_buffer_ != NULL) {
     buffer = merge_buffer_;
     merge_buffer_ = NULL;
diff --git a/remoting/client/plugin/pepper_view.h b/remoting/client/plugin/pepper_view.h
index 5a0af54..30f0df0 100644
--- a/remoting/client/plugin/pepper_view.h
+++ b/remoting/client/plugin/pepper_view.h
@@ -81,7 +81,7 @@ class PepperView : public ChromotingView,
                    const SkRegion& region);
 
   // This is a completion callback for FlushGraphics().
-  void OnFlushDone(base::Time paint_start, pp::ImageData* buffer);
+  void OnFlushDone(base::Time paint_start, pp::ImageData* buffer, int result);
 
   // Reference to the creating plugin instance. Needed for interacting with
   // pepper.  Marking explicitly as const since it must be initialized at