Getting rid of CompositorFrameMetadata::satisfies_sequences

cc/layers/surface_layer.cc

Index: cc/layers/surface_layer.cc
diff --git a/cc/layers/surface_layer.cc b/cc/layers/surface_layer.cc
index 0f8bd3380be1aa873bf5ed197dcafabad45a9c51..38561e5ed00fdc085d04470e66097a165886b0aa 100644
--- a/cc/layers/surface_layer.cc
+++ b/cc/layers/surface_layer.cc
@@ -7,6 +7,7 @@
 #include <stdint.h>
 
 #include "base/macros.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/layers/surface_layer_impl.h"
 #include "cc/output/swap_promise.h"
@@ -16,28 +17,52 @@
 
 namespace cc {
 
+namespace {
+
+// Some thread checkers fail if satisfy callback is not run on the main thread.
+void PostSatisfyCallbackToMainThread(
+    scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+    SurfaceLayer::SatisfyCallback satisfy_callback,
+    SurfaceSequence sequence) {
+  main_task_runner->PostTask(FROM_HERE, base::Bind(satisfy_callback, sequence));
+}
+
+}  // namespace
+
 class SatisfySwapPromise : public SwapPromise {
  public:
-  SatisfySwapPromise(SurfaceSequence sequence,
-                     const SurfaceLayer::SatisfyCallback& satisfy_callback)
-      : sequence_(sequence), satisfy_callback_(satisfy_callback) {}
+  SatisfySwapPromise(
+      const SurfaceSequence& sequence,
+      const SurfaceLayer::SatisfyCallback& satisfy_callback,
+      scoped_refptr<base::SingleThreadTaskRunner> main_task_runner)
+      : sequence_(sequence),
+        satisfy_callback_(satisfy_callback),
+        main_task_runner_(std::move(main_task_runner)) {}
 
   ~SatisfySwapPromise() override {}
 
  private:
   void DidActivate() override {}
   void DidSwap(CompositorFrameMetadata* metadata) override {
-    metadata->satisfies_sequences.push_back(sequence_.sequence);
+    // This method is called before the swap is actually performed in order to
+    // potentially add some stuff to compositor frame's metadata.
+    // By posting a task, we ensure that satisfy callback is not called until
+    // the swap is over.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(

[Fady Samuel, 2016/12/14 05:03:06]

Not a huge fan of this, but if jbauman@ is OK with it then so am I.

[danakj, 2016/12/14 16:17:20]

Hm.. this will ofc wait for a fresh callstack, but it is pretty fragile as it
depends on behaviour of unrelated implementation details. It should have a test
that verifies that doing a post will run the task after the swap is done on that
thread.

Another option would be DidSwap => WillSwap (and DidNotSwap => WillNotSwap) and
then add a new DidSwap that is actually after. Then its less magical and spelled
out with APIs.

+        FROM_HERE, base::Bind(PostSatisfyCallbackToMainThread,
+                              main_task_runner_, satisfy_callback_, sequence_));
   }
 
   DidNotSwapAction DidNotSwap(DidNotSwapReason reason) override {
-    satisfy_callback_.Run(sequence_);
+    main_task_runner_->PostTask(FROM_HERE,
+                                base::Bind(satisfy_callback_, sequence_));
     return DidNotSwapAction::BREAK_PROMISE;
   }
   int64_t TraceId() const override { return 0; }
 
   SurfaceSequence sequence_;
   SurfaceLayer::SatisfyCallback satisfy_callback_;
+  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(SatisfySwapPromise);
 };
@@ -117,8 +142,9 @@ void SurfaceLayer::SatisfyDestroySequence() {
   if (!layer_tree_host())
     return;
   DCHECK(destroy_sequence_.is_valid());
-  std::unique_ptr<SatisfySwapPromise> satisfy(
-      new SatisfySwapPromise(destroy_sequence_, satisfy_callback_));
+  auto satisfy =
+      base::MakeUnique<SatisfySwapPromise>(destroy_sequence_, satisfy_callback_,
+                                           base::ThreadTaskRunnerHandle::Get());
   layer_tree_host()->GetSwapPromiseManager()->QueueSwapPromise(
       std::move(satisfy));
   destroy_sequence_ = SurfaceSequence();