Move visual rect unioning between paired items to cc::DisplayItemList.

cc/playback/display_item_list.h

Index: cc/playback/display_item_list.h
diff --git a/cc/playback/display_item_list.h b/cc/playback/display_item_list.h
index 402e73f421f99678de974f9b5680d71c9afc2e44..8db83dd0d4e83808044edb2b52b00e977860f0b1 100644
--- a/cc/playback/display_item_list.h
+++ b/cc/playback/display_item_list.h
@@ -70,18 +70,41 @@ class CC_EXPORT DisplayItemList
   // approximate_op_count_.
   void RasterIntoCanvas(const DisplayItem& display_item);
 
-  // Because processing happens in this function, all the set up for
-  // this item should be done via the args, which is why the return
-  // type needs to be const, to prevent set-after-processing mistakes.
+  // Because processing happens in these CreateAndAppend functions, all the set
+  // up for the item should be done via the args, which is why the return type
+  // needs to be const, to prevent set-after-processing mistakes.
   template <typename DisplayItemType, typename... Args>
-  const DisplayItemType& CreateAndAppendItem(const gfx::Rect& visual_rect,
-                                             Args&&... args) {
+  const DisplayItemType& CreateAndAppendPairedBeginItem(Args&&... args) {
+    size_t item_index = inputs_.visual_rects.size();
+    inputs_.visual_rects.push_back(gfx::Rect());
+    inputs_.begin_item_indices.push_back(item_index);
+
+    return AllocateAndConstruct<DisplayItemType>(std::forward<Args>(args)...);
+  }
+
+  template <typename DisplayItemType, typename... Args>
+  const DisplayItemType& CreateAndAppendPairedEndItem(Args&&... args) {
+    size_t last_begin_index = inputs_.begin_item_indices.back();

[vmpstr, 2016/08/10 19:47:31]

DCHECK(!inputs_.begin_item_indices.empty());

[wkorman, 2016/08/10 21:04:25]

Done.

+    inputs_.begin_item_indices.pop_back();
+
+    // Ending bounds match the starting bounds.
+    inputs_.visual_rects.push_back(inputs_.visual_rects[last_begin_index]);
+
+    // The block that ended needs to be included in the bounds of the enclosing
+    // block.
+    GrowCurrentBeginItemVisualRect(inputs_.visual_rects[last_begin_index]);

[vmpstr, 2016/08/10 19:47:31]

I think inputs_.visual_rects.back() is cleaner. Either that or

const auto& visual_rect = inputs_.visual_rects[last_begin_index];
and then use visual_rect both here and in the push_back above.

[wkorman, 2016/08/10 21:04:25]

Neither seems equivalent to existing logic:
1. back() doesn't reference the same rect in all circumstances since
last_begin_index could reference an item other than the currently-last one
2. const auto& grabs a ref to an element in the visual_rects vector which does
not look to survive through vector mutation in line 91.

Sample test logging below to illustrate:

[ RUN      ] DisplayItemListTest.AppendVisualRectTwoBlocksTwoDrawings
[26331:26331:0810/134228:702083247033:ERROR:display_item_list.h(93)] 1:
last_begin_index=2, rect=7,8 1x1, back() rect=7,8 1x1, auto rect=7,8 1x1
[26331:26331:0810/134228:702083247127:ERROR:display_item_list.h(100)] 1:
last_begin_index=2, rect=7,8 1x1, back() rect=7,8 1x1, auto
rect=-842150451,-842150451 -842150451x-842150451
[26331:26331:0810/134228:702083247198:ERROR:display_item_list.h(93)] 1:
last_begin_index=0, rect=5,6 3x3, back() rect=7,8 1x1, auto rect=5,6 3x3
[26331:26331:0810/134228:702083247254:ERROR:display_item_list.h(100)] 1:
last_begin_index=0, rect=5,6 3x3, back() rect=5,6 3x3, auto rect=5,6 3x3

Perhaps we should add a comment here to make this clearer though, since all
three of us have now at one point or another thought this could be streamlined,
I took a pass, open to thoughts.

Or, we can do it with your second option (const auto&) but we'd need to make a
copy of the rect rather than use a reference. I prefer it as is but open to
switching if you don't mind the copy.

[vmpstr, 2016/08/10 21:09:43]

Err, yeah you're right. You can ignore my comment. 

+
+    return AllocateAndConstruct<DisplayItemType>(std::forward<Args>(args)...);
+  }
+
+  template <typename DisplayItemType, typename... Args>
+  const DisplayItemType& CreateAndAppendDrawingItem(
+      const gfx::Rect& visual_rect,
+      Args&&... args) {
     inputs_.visual_rects.push_back(visual_rect);
-    auto* item = &inputs_.items.AllocateAndConstruct<DisplayItemType>(
-        std::forward<Args>(args)...);
-    approximate_op_count_ += item->ApproximateOpCount();
-    ProcessAppendedItem(item);
-    return *item;
+    GrowCurrentBeginItemVisualRect(visual_rect);
+
+    return AllocateAndConstruct<DisplayItemType>(std::forward<Args>(args)...);
   }
 
   // Called after all items are appended, to process the items and, if
@@ -108,6 +131,8 @@ class CC_EXPORT DisplayItemList
                                   float raster_scale,
                                   std::vector<DrawImage>* images);
 
+  size_t size() const { return inputs_.items.size(); }
+
   gfx::Rect VisualRectForTesting(int index) {
     return inputs_.visual_rects[index];
   }
@@ -126,8 +151,20 @@ class CC_EXPORT DisplayItemList
                   bool retain_individual_display_items);
   ~DisplayItemList();
 
+  // If we're currently within a paired display item block, unions the
+  // given visual rect with the begin display item's visual rect.
+  void GrowCurrentBeginItemVisualRect(const gfx::Rect& visual_rect);
   void ProcessAppendedItem(const DisplayItem* item);
 
+  template <typename DisplayItemType, typename... Args>
+  const DisplayItemType& AllocateAndConstruct(Args&&... args) {
+    auto* item = &inputs_.items.AllocateAndConstruct<DisplayItemType>(
+        std::forward<Args>(args)...);
+    approximate_op_count_ += item->ApproximateOpCount();
+    ProcessAppendedItem(item);
+    return *item;
+  }
+
   sk_sp<SkPicture> picture_;
 
   std::unique_ptr<SkPictureRecorder> recorder_;
@@ -151,7 +188,7 @@ class CC_EXPORT DisplayItemList
     // |items| . These rects are intentionally kept separate
     // because they are not needed while walking the |items| for raster.
     std::vector<gfx::Rect> visual_rects;
-
+    std::vector<size_t> begin_item_indices;
     const DisplayItemListSettings settings;
     gfx::Rect layer_rect;
     bool is_suitable_for_gpu_rasterization;