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

cc/playback/display_item_list.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CC_PLAYBACK_DISPLAY_ITEM_LIST_H_
 #define CC_PLAYBACK_DISPLAY_ITEM_LIST_H_
 
 #include <stddef.h>
 
 #include <memory>
@@ skipping 52 matching lines @@
               const gfx::Rect& canvas_target_playback_rect,
               float contents_scale) const;
 
   void Raster(SkCanvas* canvas, SkPicture::AbortCallback* callback) const;
 
   // This is a fast path for use only if canvas_ is set and
   // retain_individual_display_items_ is false. This method also updates
   // 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
   // applicable, create an internally cached SkPicture.
   void Finalize();
 
   void SetIsSuitableForGpuRasterization(bool is_suitable) {
     inputs_.is_suitable_for_gpu_rasterization = is_suitable;
   }
   bool IsSuitableForGpuRasterization() const;
   int ApproximateOpCount() const;
   size_t ApproximateMemoryUsage() const;
   bool ShouldBeAnalyzedForSolidColor() const;
 
   bool RetainsIndividualDisplayItems() const;
 
   std::unique_ptr<base::trace_event::ConvertableToTraceFormat> AsValue(
       bool include_items) const;
 
   void EmitTraceSnapshot() const;
 
   void GenerateDiscardableImagesMetadata();
   void GetDiscardableImagesInRect(const gfx::Rect& rect,
                                   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];
   }
 
   ContiguousContainer<DisplayItem>::const_iterator begin() const {
     return inputs_.items.begin();
   }
 
   ContiguousContainer<DisplayItem>::const_iterator end() const {
     return inputs_.items.end();
   }
 
  private:
   DisplayItemList(gfx::Rect layer_rect,
                   const DisplayItemListSettings& display_list_settings,
                   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_;
 
   bool retain_individual_display_items_;
   int approximate_op_count_;
 
   // Memory usage due to the cached SkPicture.
   size_t picture_memory_usage_;
 
   DiscardableImageMap image_map_;
 
   struct Inputs {
     Inputs(gfx::Rect layer_rect, const DisplayItemListSettings& settings);
     ~Inputs();
 
     ContiguousContainer<DisplayItem> items;
     // The visual rects associated with each of the display items in the
     // display item list. There is one rect per display item, and the
     // position in |visual_rects| matches the position of the item in
     // |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;
   };
 
   Inputs inputs_;
 
   friend class base::RefCountedThreadSafe<DisplayItemList>;
   FRIEND_TEST_ALL_PREFIXES(DisplayItemListTest, ApproximateMemoryUsage);
   DISALLOW_COPY_AND_ASSIGN(DisplayItemList);
 };
 
 }  // namespace cc
 
 #endif  // CC_PLAYBACK_DISPLAY_ITEM_LIST_H_