cc: Fix logic for detecting when raster tasks were throttled

cc: Fix logic for detecting when raster tasks were throttled

The PixelBufferRasterWorkerPool checks whether raster tasks were
throttled to avoid notifying the client that all pending tasks were
completed too early. The current logic for this fails if tasks from the
previous graph were still active when we schedule a new graph. In these
situations we end up thinking some tasks were throttled and will not
schedule the final sentinel task to indicate work completion. This can
lead to delayed tree activation.

The fix is to only consider tasks from the new work queue when seeing if
any got throttled.

BUG=331534
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243962

[reveman, 2014-01-09 18:01:21]

LGTM, thanks!

[commit-bot: I haz the power, 2014-01-09 18:09:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/131763003/1

[commit-bot: I haz the power, 2014-01-09 18:10:01]

Failed to apply patch for cc/resources/raster_worker_pool_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/resources/raster_worker_pool_unittest.cc
  Hunk #1 succeeded at 70 (offset 14 lines).
  Hunk #2 FAILED at 207.
  Hunk #3 succeeded at 412 (offset 92 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
cc/resources/raster_worker_pool_unittest.cc.rej

Patch:       cc/resources/raster_worker_pool_unittest.cc
Index: cc/resources/raster_worker_pool_unittest.cc
diff --git a/cc/resources/raster_worker_pool_unittest.cc
b/cc/resources/raster_worker_pool_unittest.cc
index
716edf6c5263ef1380f60e6a0617c22bfe93c46d..2ba76e436027263a21e21d1f412855a73aba1c42
100644
--- a/cc/resources/raster_worker_pool_unittest.cc
+++ b/cc/resources/raster_worker_pool_unittest.cc
@@ -56,6 +56,39 @@ class TestRasterWorkerPoolTaskImpl : public
internal::RasterWorkerPoolTask {
   DISALLOW_COPY_AND_ASSIGN(TestRasterWorkerPoolTaskImpl);
 };
 
+class BlockingRasterWorkerPoolTaskImpl : public TestRasterWorkerPoolTaskImpl {
+ public:
+  BlockingRasterWorkerPoolTaskImpl(
+      const Resource* resource,
+      const Reply& reply,
+      base::Lock* lock,
+      TaskVector* dependencies)
+      : TestRasterWorkerPoolTaskImpl(resource, reply, dependencies),
+        lock_(lock) {}
+
+  // Overridden from TestRasterWorkerPoolTaskImpl:
+  virtual bool RunOnWorkerThread(unsigned thread_index,
+                                 void* buffer,
+                                 gfx::Size size,
+                                 int stride) OVERRIDE {
+    base::AutoLock lock(*lock_);
+    return TestRasterWorkerPoolTaskImpl::RunOnWorkerThread(thread_index,
+                                                           buffer,
+                                                           size,
+                                                           stride);
+  }
+  virtual void CompleteOnOriginThread() OVERRIDE {}
+
+ protected:
+  virtual ~BlockingRasterWorkerPoolTaskImpl() {}
+
+ private:
+  base::Lock* lock_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlockingRasterWorkerPoolTaskImpl);
+};
+
+
 class RasterWorkerPoolTest : public testing::Test,
                              public RasterWorkerPoolClient  {
  public:
@@ -174,6 +207,26 @@ class RasterWorkerPoolTest : public testing::Test,
             &empty.tasks_)));
   }
 
+  void AppendBlockingTask(unsigned id, base::Lock* lock) {
+    const gfx::Size size(1, 1);
+
+    scoped_ptr<ScopedResource> resource(
+        ScopedResource::Create(resource_provider()));
+    resource->Allocate(size, ResourceProvider::TextureUsageAny, RGBA_8888);
+    const Resource* const_resource = resource.get();
+
+    RasterWorkerPool::Task::Set empty;
+    tasks_.push_back(
+        RasterWorkerPool::RasterTask(new BlockingRasterWorkerPoolTaskImpl(
+            const_resource,
+            base::Bind(&RasterWorkerPoolTest::OnTaskCompleted,
+                       base::Unretained(this),
+                       base::Passed(&resource),
+                       id),
+            lock,
+            &empty.tasks_)));
+  }
+
   virtual void OnTaskCompleted(scoped_ptr<ScopedResource> resource,
                                unsigned id,
                                const PicturePileImpl::Analysis& analysis,
@@ -287,6 +340,40 @@ class RasterWorkerPoolTestFailedMapResource : public
RasterWorkerPoolTest {
 
 PIXEL_BUFFER_AND_IMAGE_TEST_F(RasterWorkerPoolTestFailedMapResource);
 
+class RasterWorkerPoolTestFalseThrottling : public RasterWorkerPoolTest {
+ public:
+  // Overridden from RasterWorkerPoolTest:
+  virtual void BeginTest() OVERRIDE {
+    // This test checks that replacing a pending raster task with another does
+    // not prevent the DidFinishRunningTasks notification from being sent.
+
+    // Schedule a task that is prevented from completing with a lock.
+    lock_.Acquire();
+    AppendBlockingTask(0u, &lock_);
+    ScheduleTasks();
+
+    // Schedule another task to replace the still-pending task. Because the old
+    // task is not a throttled task in the new task set, it should not prevent
+    // DidFinishRunningTasks from getting signaled.
+    tasks_.clear();
+    AppendTask(1u);
+    ScheduleTasks();
+
+    // Unblock the first task to allow the second task to complete.
+    lock_.Release();
+  }
+
+  virtual void AfterTest() OVERRIDE {}
+
+  virtual void DidFinishRunningTasks() OVERRIDE {
+    EndTest();
+  }
+
+  base::Lock lock_;
+};
+
+PIXEL_BUFFER_AND_IMAGE_TEST_F(RasterWorkerPoolTestFalseThrottling);
+
 }  // namespace
 
 }  // namespace cc

[commit-bot: I haz the power, 2014-01-09 18:43:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/131763003/50001

[commit-bot: I haz the power, 2014-01-09 20:18:57]

Change committed as 243962