Cleanup: Remove CopyOutputRequest relay request for safer encapsulation.

As part of a design exploration, it was noted that code in layer.cc is
manually creating a copy of one or more CopyOutputRequests and then
copying over their properties. This is brittle, since adding new
properties to CopyOutputRequest could silently break functionality.

This change solves the problem by adding a "result task runner" that can
be set on a CopyOutputRequest. Using this will ensure results are
delivered via an alternate task runner where that is required. At the
same time, lots of LOC are able to be deleted because the requests are
no longer being duplicated.

Change-Id: Ie26eb2de8f92e796d437e4e1c2b04a964e2ffef7
Reviewed-on: https://chromium-review.googlesource.com/567602Reviewed-by: Vladimir Levin <vmpstr@chromium.org>
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486030}

cc/layers/layer.cc

@@ -1114,21 +1114,6 @@ void Layer::SetStickyPositionConstraint(
   SetNeedsCommit();
 }
 
-static void RunCopyCallbackOnMainThread(
-    std::unique_ptr<CopyOutputRequest> request,
-    std::unique_ptr<CopyOutputResult> result) {
-  request->SendResult(std::move(result));
-}
-
-static void PostCopyCallbackToMainThread(
-    scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner,
-    std::unique_ptr<CopyOutputRequest> request,
-    std::unique_ptr<CopyOutputResult> result) {
-  main_thread_task_runner->PostTask(
-      FROM_HERE, base::BindOnce(&RunCopyCallbackOnMainThread,
-                                base::Passed(&request), base::Passed(&result)));
-}
-
 bool Layer::IsSnapped() {
   return scrollable();
 }
@@ -1218,21 +1203,17 @@ void Layer::PushPropertiesTo(LayerImpl* layer) {
 
 void Layer::TakeCopyRequests(
     std::vector<std::unique_ptr<CopyOutputRequest>>* requests) {
-  for (auto& it : inputs_.copy_requests) {
-    scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner =
-        layer_tree_host()->GetTaskRunnerProvider()->MainThreadTaskRunner();
-    std::unique_ptr<CopyOutputRequest> original_request = std::move(it);
-    const CopyOutputRequest& original_request_ref = *original_request;
-    std::unique_ptr<CopyOutputRequest> main_thread_request =
-        CopyOutputRequest::CreateRelayRequest(
-            original_request_ref,
-            base::Bind(&PostCopyCallbackToMainThread, main_thread_task_runner,
-                       base::Passed(&original_request)));
-    if (main_thread_request->has_area()) {
-      main_thread_request->set_area(gfx::IntersectRects(
-          main_thread_request->area(), gfx::Rect(bounds())));
+  for (std::unique_ptr<CopyOutputRequest>& request : inputs_.copy_requests) {
+    // Ensure the result callback is not invoked on the compositing thread.
+    if (!request->has_result_task_runner()) {
+      request->set_result_task_runner(
+          layer_tree_host()->GetTaskRunnerProvider()->MainThreadTaskRunner());
+    }
+    if (request->has_area()) {
+      request->set_area(
+          gfx::IntersectRects(request->area(), gfx::Rect(bounds())));
     }
-    requests->push_back(std::move(main_thread_request));
+    requests->push_back(std::move(request));
   }
 
   inputs_.copy_requests.clear();

cc/output/copy_output_request.cc

@@ -14,17 +14,6 @@
 
 namespace cc {
 
-// static
-std::unique_ptr<CopyOutputRequest> CopyOutputRequest::CreateRelayRequest(
-    const CopyOutputRequest& original_request,
-    const CopyOutputRequestCallback& result_callback) {
-  std::unique_ptr<CopyOutputRequest> relay = CreateRequest(result_callback);
-  relay->force_bitmap_result_ = original_request.force_bitmap_result_;
-  relay->area_ = original_request.area_;
-  relay->texture_mailbox_ = original_request.texture_mailbox_;
-  return relay;
-}
-
 CopyOutputRequest::CopyOutputRequest() : force_bitmap_result_(false) {}
 
 CopyOutputRequest::CopyOutputRequest(
@@ -42,9 +31,16 @@ CopyOutputRequest::~CopyOutputRequest() {
 }
 
 void CopyOutputRequest::SendResult(std::unique_ptr<CopyOutputResult> result) {
-  bool success = !result->IsEmpty();
-  base::ResetAndReturn(&result_callback_).Run(std::move(result));
-  TRACE_EVENT_ASYNC_END1("cc", "CopyOutputRequest", this, "success", success);
+  TRACE_EVENT_ASYNC_END1("cc", "CopyOutputRequest", this, "success",
+                         !result->IsEmpty());
+  if (result_task_runner_) {
+    result_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(base::ResetAndReturn(&result_callback_),
+                                  std::move(result)));
+    result_task_runner_ = nullptr;
+  } else {
+    base::ResetAndReturn(&result_callback_).Run(std::move(result));
+  }
 }
 
 void CopyOutputRequest::SendEmptyResult() {

cc/output/copy_output_request.h

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/memory/ptr_util.h"
 #include "base/optional.h"
+#include "base/task_runner.h"
 #include "base/unguessable_token.h"
 #include "cc/cc_export.h"
 #include "cc/resources/single_release_callback.h"
@@ -43,14 +44,19 @@ class CC_EXPORT CopyOutputRequest {
       const CopyOutputRequestCallback& result_callback) {
     return base::WrapUnique(new CopyOutputRequest(true, result_callback));
   }
-  static std::unique_ptr<CopyOutputRequest> CreateRelayRequest(
-      const CopyOutputRequest& original_request,
-      const CopyOutputRequestCallback& result_callback);
 
   ~CopyOutputRequest();
 
   bool IsEmpty() const { return result_callback_.is_null(); }
 
+  // Requests that the result callback be run as a task posted to the given
+  // |task_runner|. If this is not set, the result callback could be run from
+  // any context.
+  void set_result_task_runner(scoped_refptr<base::TaskRunner> task_runner) {
+    result_task_runner_ = std::move(task_runner);
+  }
+  bool has_result_task_runner() const { return !!result_task_runner_; }
+
   // Optionally specify the source of this copy request. If set when this copy
   // request is submitted to a layer, a prior uncommitted copy request from the
   // same source will be aborted.
@@ -93,6 +99,7 @@ class CC_EXPORT CopyOutputRequest {
   CopyOutputRequest(bool force_bitmap_result,
                     const CopyOutputRequestCallback& result_callback);
 
+  scoped_refptr<base::TaskRunner> result_task_runner_;
   base::Optional<base::UnguessableToken> source_;
   bool force_bitmap_result_;
   base::Optional<gfx::Rect> area_;