Don't access DisplayItemClient::VisualRect() for cached display items.

- Access DisplayItemClient::VisualRect() when a new DisplayItem is created, and save it in the new DisplayItem. - When a cached display item is copied, don't access DisplayItemClient:: VisualRect() but fetch from the cached DisplayItem. - Remove DisplayItemList::visual_rects_. - Save original visual rects (LayoutRect) and apply GraphicsLayer offset with EnclosingIntRect() when the display items are added into WebDisplayItemList. Please see the bug for the reason. BUG=724263 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2894093002 Cr-Commit-Position: refs/heads/master@{#473371}
qweri0p · May 19, 2017 · 3c7f1c4 · 3c7f1c4
1 parent d00385a
commit 3c7f1c4
Show file tree

Hide file tree

Showing 38 changed files with 197 additions and 243 deletions.
diff --git a/third_party/WebKit/Source/core/frame/FrameView.cpp b/third_party/WebKit/Source/core/frame/FrameView.cpp
@@ -3303,7 +3303,7 @@ void FrameView::PaintTree() {
       if (RuntimeEnabledFeatures::printBrowserEnabled())
         graphics_context.SetPrinting(true);
       Paint(graphics_context, CullRect(LayoutRect::InfiniteIntRect()));
-      paint_controller_->CommitNewDisplayItems(LayoutSize());
+      paint_controller_->CommitNewDisplayItems();
     }
   } else {
     // A null graphics layer can occur for painting of SVG images that are not

diff --git a/third_party/WebKit/Source/core/paint/PaintControllerPaintTest.h b/third_party/WebKit/Source/core/paint/PaintControllerPaintTest.h
@@ -118,7 +118,7 @@ class TestDisplayItem final : public DisplayItem {
       : DisplayItem(client, type, sizeof(*this)) {}
 
   void Replay(GraphicsContext&) const final { NOTREACHED(); }
-  void AppendToWebDisplayItemList(const IntRect&,
+  void AppendToWebDisplayItemList(const LayoutSize&,
                                   WebDisplayItemList*) const final {
     NOTREACHED();
   }

diff --git a/third_party/WebKit/Source/platform/BUILD.gn b/third_party/WebKit/Source/platform/BUILD.gn
@@ -1858,8 +1858,8 @@ test("blink_platform_unittests") {
     "graphics/gpu/SharedGpuContextTest.cpp",
     "graphics/gpu/WebGLImageConversionTest.cpp",
     "graphics/paint/DisplayItemClientTest.cpp",
-    "graphics/paint/DisplayItemListTest.cpp",
     "graphics/paint/DisplayItemTest.cpp",
+    "graphics/paint/DrawingDisplayItemTest.cpp",
     "graphics/paint/FloatClipRectTest.cpp",
     "graphics/paint/GeometryMapperTest.cpp",
     "graphics/paint/PaintChunkTest.cpp",

diff --git a/third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp b/third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp
@@ -90,6 +90,7 @@ void ContentLayerDelegate::PaintContents(
     graphics_layer_->Paint(nullptr, disabled_mode);
 
   paint_controller.GetPaintArtifact().AppendToWebDisplayItemList(
+      graphics_layer_->OffsetFromLayoutObjectWithSubpixelAccumulation(),
       web_display_item_list);
 
   paint_controller.SetDisplayItemConstructionIsDisabled(false);

diff --git a/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp b/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
@@ -277,8 +277,7 @@ IntRect GraphicsLayer::InterestRect() {
 void GraphicsLayer::Paint(const IntRect* interest_rect,
                           GraphicsContext::DisabledMode disabled_mode) {
   if (PaintWithoutCommit(interest_rect, disabled_mode)) {
-    GetPaintController().CommitNewDisplayItems(
-        OffsetFromLayoutObjectWithSubpixelAccumulation());
+    GetPaintController().CommitNewDisplayItems();
     if (RuntimeEnabledFeatures::paintUnderInvalidationCheckingEnabled()) {
       sk_sp<PaintRecord> record = CaptureRecord();
       CheckPaintUnderInvalidations(record);
@@ -1282,8 +1281,7 @@ void GraphicsLayer::CheckPaintUnderInvalidations(
   recorder.beginRecording(rect);
   recorder.getRecordingCanvas()->drawBitmap(new_bitmap, rect.X(), rect.Y());
   sk_sp<PaintRecord> record = recorder.finishRecordingAsPicture();
-  GetPaintController().AppendDebugDrawingAfterCommit(
-      *this, record, OffsetFromLayoutObjectWithSubpixelAccumulation());
+  GetPaintController().AppendDebugDrawingAfterCommit(*this, record);
 }
 
 }  // namespace blink

diff --git a/third_party/WebKit/Source/platform/graphics/paint/ClipDisplayItem.cpp b/third_party/WebKit/Source/platform/graphics/paint/ClipDisplayItem.cpp
@@ -26,7 +26,7 @@ void ClipDisplayItem::Replay(GraphicsContext& context) const {
 }
 
 void ClipDisplayItem::AppendToWebDisplayItemList(
-    const IntRect& visual_rect,
+    const LayoutSize&,
     WebDisplayItemList* list) const {
   WebVector<SkRRect> web_rounded_rects(rounded_rect_clips_.size());
   for (size_t i = 0; i < rounded_rect_clips_.size(); ++i)
@@ -40,7 +40,7 @@ void EndClipDisplayItem::Replay(GraphicsContext& context) const {
 }
 
 void EndClipDisplayItem::AppendToWebDisplayItemList(
-    const IntRect& visual_rect,
+    const LayoutSize&,
     WebDisplayItemList* list) const {
   list->AppendEndClipItem();
 }

diff --git a/third_party/WebKit/Source/platform/graphics/paint/ClipDisplayItem.h b/third_party/WebKit/Source/platform/graphics/paint/ClipDisplayItem.h
@@ -33,7 +33,7 @@ class PLATFORM_EXPORT ClipDisplayItem final : public PairedBeginDisplayItem {
   }
 
   void Replay(GraphicsContext&) const override;
-  void AppendToWebDisplayItemList(const IntRect&,
+  void AppendToWebDisplayItemList(const LayoutSize&,
                                   WebDisplayItemList*) const override;
 
  private:
@@ -60,7 +60,7 @@ class PLATFORM_EXPORT EndClipDisplayItem final : public PairedEndDisplayItem {
   }
 
   void Replay(GraphicsContext&) const override;
-  void AppendToWebDisplayItemList(const IntRect&,
+  void AppendToWebDisplayItemList(const LayoutSize&,
                                   WebDisplayItemList*) const override;
 
  private:

diff --git a/third_party/WebKit/Source/platform/graphics/paint/ClipPathDisplayItem.cpp b/third_party/WebKit/Source/platform/graphics/paint/ClipPathDisplayItem.cpp
@@ -18,7 +18,7 @@ void BeginClipPathDisplayItem::Replay(GraphicsContext& context) const {
 }
 
 void BeginClipPathDisplayItem::AppendToWebDisplayItemList(
-    const IntRect& visual_rect,
+    const LayoutSize&,
     WebDisplayItemList* list) const {
   list->AppendClipPathItem(clip_path_, true);
 }
@@ -36,7 +36,7 @@ void EndClipPathDisplayItem::Replay(GraphicsContext& context) const {
 }
 
 void EndClipPathDisplayItem::AppendToWebDisplayItemList(
-    const IntRect& visual_rect,
+    const LayoutSize&,
     WebDisplayItemList* list) const {
   list->AppendEndClipPathItem();
 }

diff --git a/third_party/WebKit/Source/platform/graphics/paint/ClipPathDisplayItem.h b/third_party/WebKit/Source/platform/graphics/paint/ClipPathDisplayItem.h
@@ -21,7 +21,7 @@ class PLATFORM_EXPORT BeginClipPathDisplayItem final
         clip_path_(clip_path.GetSkPath()) {}
 
   void Replay(GraphicsContext&) const override;
-  void AppendToWebDisplayItemList(const IntRect&,
+  void AppendToWebDisplayItemList(const LayoutSize&,
                                   WebDisplayItemList*) const override;
 
   int NumberOfSlowPaths() const override;
@@ -46,7 +46,7 @@ class PLATFORM_EXPORT EndClipPathDisplayItem final
       : PairedEndDisplayItem(client, kEndClipPath, sizeof(*this)) {}
 
   void Replay(GraphicsContext&) const override;
-  void AppendToWebDisplayItemList(const IntRect&,
+  void AppendToWebDisplayItemList(const LayoutSize&,
                                   WebDisplayItemList*) const override;
 
  private:

diff --git a/third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.cpp b/third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.cpp
@@ -17,7 +17,7 @@ void BeginCompositingDisplayItem::Replay(GraphicsContext& context) const {
 }
 
 void BeginCompositingDisplayItem::AppendToWebDisplayItemList(
-    const IntRect& visual_rect,
+    const LayoutSize&,
     WebDisplayItemList* list) const {
   SkRect bounds = bounds_;
   list->AppendCompositingItem(
@@ -46,7 +46,7 @@ void EndCompositingDisplayItem::Replay(GraphicsContext& context) const {
 }
 
 void EndCompositingDisplayItem::AppendToWebDisplayItemList(
-    const IntRect& visual_rect,
+    const LayoutSize&,
     WebDisplayItemList* list) const {
   list->AppendEndCompositingItem();
 }

diff --git a/third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.h b/third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.h
@@ -33,7 +33,7 @@ class PLATFORM_EXPORT BeginCompositingDisplayItem final
   }
 
   void Replay(GraphicsContext&) const override;
-  void AppendToWebDisplayItemList(const IntRect&,
+  void AppendToWebDisplayItemList(const LayoutSize&,
                                   WebDisplayItemList*) const override;
 
  private:
@@ -69,7 +69,7 @@ class PLATFORM_EXPORT EndCompositingDisplayItem final
       : PairedEndDisplayItem(client, kEndCompositing, sizeof(*this)) {}
 
   void Replay(GraphicsContext&) const override;
-  void AppendToWebDisplayItemList(const IntRect&,
+  void AppendToWebDisplayItemList(const LayoutSize&,
                                   WebDisplayItemList*) const override;
 
  private:

diff --git a/third_party/WebKit/Source/platform/graphics/paint/DisplayItem.cpp b/third_party/WebKit/Source/platform/graphics/paint/DisplayItem.cpp
@@ -9,6 +9,7 @@ namespace blink {
 struct SameSizeAsDisplayItem {
   virtual ~SameSizeAsDisplayItem() {}  // Allocate vtable pointer.
   void* pointer;
+  LayoutRect rect;
   int i;
 #ifndef NDEBUG
   WTF::String debug_string_;
@@ -244,6 +245,8 @@ void DisplayItem::DumpPropertiesAsDebugString(
     string_builder.Append(' ');
     string_builder.Append(ClientDebugString());
   }
+  string_builder.Append("\", visualRect: \"");
+  string_builder.Append(VisualRect().ToString());
   string_builder.Append("\", type: \"");
   string_builder.Append(TypeAsDebugString(GetType()));
   string_builder.Append('"');

diff --git a/third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h b/third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h
@@ -20,7 +20,7 @@
 namespace blink {
 
 class GraphicsContext;
-class IntRect;
+class LayoutSize;
 class WebDisplayItemList;
 
 class PLATFORM_EXPORT DisplayItem {
@@ -199,6 +199,7 @@ class PLATFORM_EXPORT DisplayItem {
 
   DisplayItem(const DisplayItemClient& client, Type type, size_t derived_size)
       : client_(&client),
+        visual_rect_(client.VisualRect()),
         type_(type),
         derived_size_(derived_size),
         skipped_cache_(false)
@@ -233,6 +234,12 @@ class PLATFORM_EXPORT DisplayItem {
     DCHECK(client_);
     return *client_;
   }
+
+  // This equals to Client().VisualRect() as long as the client is alive and is
+  // not invalidated. Otherwise it saves the previous visual rect of the client.
+  // See DisplayItemClient::VisualRect() about its coordinate space.
+  const LayoutRect& VisualRect() const { return visual_rect_; }
+
   Type GetType() const { return type_; }
 
   // Size of this object in memory, used to move it with memcpy.
@@ -246,9 +253,11 @@ class PLATFORM_EXPORT DisplayItem {
   void SetSkippedCache() { skipped_cache_ = true; }
   bool SkippedCache() const { return skipped_cache_; }
 
-  // TODO(wkorman): Only DrawingDisplayItem needs the visual rect argument.
-  // Consider refactoring class hierarchy to make this more explicit.
-  virtual void AppendToWebDisplayItemList(const IntRect&,
+  // Appends this display item to the WebDisplayItemList, if applicable.
+  // |visual_rect_offset| is the offset between the space of the GraphicsLayer
+  // which owns the display item and the coordinate space of VisualRect().
+  // TODO(wangxianzhu): Remove the parameter for slimming paint v2.
+  virtual void AppendToWebDisplayItemList(const LayoutSize& visual_rect_offset,
                                           WebDisplayItemList*) const {}
 
 // See comments of enum Type for usage of the following macros.
@@ -351,6 +360,7 @@ class PLATFORM_EXPORT DisplayItem {
   // constructed at the source location.
   template <typename T, unsigned alignment>
   friend class ContiguousContainer;
+  friend class DisplayItemList;
 
   DisplayItem()
       : client_(nullptr),
@@ -359,6 +369,8 @@ class PLATFORM_EXPORT DisplayItem {
         skipped_cache_(false) {}
 
   const DisplayItemClient* client_;
+  LayoutRect visual_rect_;
+
   static_assert(kTypeLast < (1 << 16),
                 "DisplayItem::Type should fit in 16 bits");
   const Type type_ : 16;

diff --git a/third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h b/third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
@@ -53,7 +53,7 @@ class PLATFORM_EXPORT DisplayItemClient {
 
   // The visual rect of this DisplayItemClient, in the object space of the
   // object that owns the GraphicsLayer, i.e. offset by
-  // offsetFromLayoutObjectWithSubpixelAccumulation().
+  // GraphicsLayer::OffsetFromLayoutObjectWithSubpixelAccumulation().
   virtual LayoutRect VisualRect() const = 0;
 
   // This is declared here instead of in LayoutObject for verifying the

diff --git a/third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.cpp b/third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.cpp
@@ -14,28 +14,6 @@
 
 namespace blink {
 
-DisplayItem& DisplayItemList::AppendByMoving(DisplayItem& item) {
-#ifndef NDEBUG
-  String original_debug_string = item.AsDebugString();
-#endif
-  DCHECK(item.HasValidClient());
-  DisplayItem& result =
-      ContiguousContainer::AppendByMoving(item, item.DerivedSize());
-  // ContiguousContainer::appendByMoving() calls an in-place constructor
-  // on item which replaces it with a tombstone/"dead display item" that
-  // can be safely destructed but should never be used.
-  DCHECK(!item.HasValidClient());
-#ifndef NDEBUG
-  // Save original debug string in the old item to help debugging.
-  item.SetClientDebugString(original_debug_string);
-#endif
-  return result;
-}
-
-void DisplayItemList::AppendVisualRect(const IntRect& visual_rect) {
-  visual_rects_.push_back(visual_rect);
-}
-
 DisplayItemList::Range<DisplayItemList::iterator>
 DisplayItemList::ItemsInPaintChunk(const PaintChunk& paint_chunk) {
   return Range<iterator>(begin() + paint_chunk.begin_index,
@@ -98,14 +76,9 @@ std::unique_ptr<JSONArray> DisplayItemList::SubsequenceAsJSON(
       }
 #endif
     }
-    if (HasVisualRect(i)) {
-      IntRect local_visual_rect = VisualRect(i);
-      json->SetString(
-          "visualRect",
-          String::Format("[%d,%d %dx%d]", local_visual_rect.X(),
-                         local_visual_rect.Y(), local_visual_rect.Width(),
-                         local_visual_rect.Height()));
-    }
+
+    json->SetString("visualRect", display_item.VisualRect().ToString());
+
     json_array->PushObject(std::move(json));
   }
   return json_array;

diff --git a/third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h b/third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
@@ -32,27 +32,32 @@ class PLATFORM_EXPORT DisplayItemList
   DisplayItemList(size_t initial_size_bytes)
       : ContiguousContainer(kMaximumDisplayItemSize, initial_size_bytes) {}
   DisplayItemList(DisplayItemList&& source)
-      : ContiguousContainer(std::move(source)),
-        visual_rects_(std::move(source.visual_rects_)) {}
+      : ContiguousContainer(std::move(source)) {}
 
   DisplayItemList& operator=(DisplayItemList&& source) {
     ContiguousContainer::operator=(std::move(source));
-    visual_rects_ = std::move(source.visual_rects_);
     return *this;
   }
 
-  DisplayItem& AppendByMoving(DisplayItem&);
-
-  bool HasVisualRect(size_t index) const {
-    return index < visual_rects_.size();
-  }
-  IntRect VisualRect(size_t index) const {
-    DCHECK(HasVisualRect(index));
-    return visual_rects_[index];
+  DisplayItem& AppendByMoving(DisplayItem& item) {
+#ifndef NDEBUG
+    String original_debug_string = item.AsDebugString();
+#endif
+    DCHECK(item.HasValidClient());
+    DisplayItem& result =
+        ContiguousContainer::AppendByMoving(item, item.DerivedSize());
+    // ContiguousContainer::AppendByMoving() calls an in-place constructor
+    // on item which replaces it with a tombstone/"dead display item" that
+    // can be safely destructed but should never be used.
+    DCHECK(!item.HasValidClient());
+#ifndef NDEBUG
+    // Save original debug string in the old item to help debugging.
+    item.SetClientDebugString(original_debug_string);
+#endif
+    item.visual_rect_ = result.visual_rect_;
+    return result;
   }
 
-  void AppendVisualRect(const IntRect& visual_rect);
-
   // Useful for iterating with a range-based for loop.
   template <typename Iterator>
   class Range {
@@ -81,9 +86,6 @@ class PLATFORM_EXPORT DisplayItemList
   std::unique_ptr<JSONArray> SubsequenceAsJSON(size_t begin_index,
                                                size_t end_index,
                                                JsonFlags options) const;
-
- private:
-  Vector<IntRect> visual_rects_;
 };
 
 }  // namespace blink