Cache gpu suitability in DisplayItemList, remove SetUnsuitable...ForTesting

cc/playback/display_item_list.cc

Index: cc/playback/display_item_list.cc
diff --git a/cc/playback/display_item_list.cc b/cc/playback/display_item_list.cc
index 1275f3aaa2b9976a1007b7ef56b61836e9989a0b..769e09bdab69b9f95f8d4f6b163e1635cbb513de 100644
--- a/cc/playback/display_item_list.cc
+++ b/cc/playback/display_item_list.cc
@@ -55,7 +55,7 @@ DisplayItemList::DisplayItemList(gfx::Rect layer_rect,
       use_cached_picture_(settings.use_cached_picture),
       retain_individual_display_items_(retain_individual_display_items),
       layer_rect_(layer_rect),
-      all_items_are_suitable_for_gpu_rasterization_(true),
+      is_suitable_for_gpu_rasterization_(true),
       approximate_op_count_(0),
       picture_memory_usage_(0),
       external_memory_usage_(0) {
@@ -123,13 +123,19 @@ void DisplayItemList::ProcessAppendedItems() {
   needs_process_ = false;
 #endif
   for (const DisplayItem* item : items_) {
-    all_items_are_suitable_for_gpu_rasterization_ &=
-        item->is_suitable_for_gpu_rasterization();
-    approximate_op_count_ += item->approximate_op_count();
-
     if (use_cached_picture_) {
+      // When using a cached picture we will calculate gpu suitability on the
+      // entire cached picture instead of the items. This is more permissive
+      // than since none of the items might individually trigger a veto even
+      // though they collectively have enough "bad" operations that a
+      // corresponding Picture would get vetoed. See crbug.com/513016.
       DCHECK(canvas_);
+      approximate_op_count_ += item->approximate_op_count();
       item->Raster(canvas_.get(), gfx::Rect(), NULL);
+    } else {
+      is_suitable_for_gpu_rasterization_ &=
+          item->is_suitable_for_gpu_rasterization();
+      approximate_op_count_ += item->approximate_op_count();
     }
 
     if (retain_individual_display_items_) {
@@ -146,7 +152,7 @@ void DisplayItemList::ProcessAppendedItems() {
 void DisplayItemList::RasterIntoCanvas(const DisplayItem& item) {
   DCHECK(canvas_);
   DCHECK(!retain_individual_display_items_);
-  all_items_are_suitable_for_gpu_rasterization_ &=
+  is_suitable_for_gpu_rasterization_ &=

[danakj, 2015/09/17 21:11:32]

why do we do this here too? don't we want to compute all of this at recording
time? we use it to decide about how to raster

[pdr., 2015/09/18 22:40:14]

This was a mistake. I had refactored this so ProcessAppendedItems called
RasterIntoCanvas but un-factored it and forgot this line. Good catch.

       item.is_suitable_for_gpu_rasterization();
   approximate_op_count_ += item.approximate_op_count();
 
@@ -179,19 +185,14 @@ void DisplayItemList::Finalize() {
         SkPictureUtils::ApproximateBytesUsed(picture_.get());
     recorder_.reset();
     canvas_.clear();
+    is_suitable_for_gpu_rasterization_ =
+        picture_->suitableForGpuRasterization(NULL);

[danakj, 2015/09/17 21:11:32]

nullptr

[pdr., 2015/09/18 22:40:14]

We should make a pass through and remove all the NULLs so I can copy&paste with
reckless abandon.

   }
 }
 
 bool DisplayItemList::IsSuitableForGpuRasterization() const {
   DCHECK(ProcessAppendedItemsCalled());
-  if (use_cached_picture_)
-    return picture_->suitableForGpuRasterization(NULL);
-
-  // This is more permissive than Picture's implementation, since none of the
-  // items might individually trigger a veto even though they collectively have
-  // enough "bad" operations that a corresponding Picture would get vetoed. See
-  // crbug.com/513016.
-  return all_items_are_suitable_for_gpu_rasterization_;
+  return is_suitable_for_gpu_rasterization_;
 }
 
 int DisplayItemList::ApproximateOpCount() const {