Factor PaintArtifact out of DisplayItemList.

third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h

Index: third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
diff --git a/third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h b/third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
index 588e52bd9262c92d09f6d21025271ce8dd9af624..22660f019b9909bf49e30fc2780f7601611d355e 100644
--- a/third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
+++ b/third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
@@ -12,6 +12,8 @@
 #include "platform/graphics/ContiguousContainer.h"
 #include "platform/graphics/PaintInvalidationReason.h"
 #include "platform/graphics/paint/DisplayItem.h"
+#include "platform/graphics/paint/DisplayItems.h"
+#include "platform/graphics/paint/PaintArtifact.h"
 #include "platform/graphics/paint/PaintChunk.h"
 #include "platform/graphics/paint/PaintChunker.h"
 #include "platform/graphics/paint/Transform3DDisplayItem.h"
@@ -26,36 +28,11 @@ namespace blink {
 class GraphicsLayer;
 class GraphicsContext;
 
-// kDisplayItemAlignment must be a multiple of alignof(derived display item) for
-// each derived display item; the ideal value is the least common multiple.
-// Currently the limiting factor is TransformtionMatrix (in
-// BeginTransform3DDisplayItem), which requests 16-byte alignment.
-static const size_t kDisplayItemAlignment = WTF_ALIGN_OF(BeginTransform3DDisplayItem);
 static const size_t kInitialDisplayItemsCapacity = 64;
-static const size_t kMaximumDisplayItemSize = sizeof(BeginTransform3DDisplayItem);
-
-class DisplayItems : public ContiguousContainer<DisplayItem, kDisplayItemAlignment> {
-public:
-    DisplayItems(size_t initialSizeBytes)
-        : ContiguousContainer(kMaximumDisplayItemSize, initialSizeBytes) {}
-
-    DisplayItem& appendByMoving(DisplayItem& item)
-    {
-#ifndef NDEBUG
-        WTF::String originalDebugString = item.asDebugString();
-#endif
-        ASSERT(item.isValid());
-        DisplayItem& result = ContiguousContainer::appendByMoving(item, item.derivedSize());
-        // ContiguousContainer::appendByMoving() called in-place constructor on item, which invalidated it.
-        ASSERT(!item.isValid());
-#ifndef NDEBUG
-        // Save original debug string in the old item to help debugging.
-        item.setClientDebugString(originalDebugString);
-#endif
-        return result;
-    }
-};
 
+// Responsible for processing display items as they are produced, and producing
+// a final paint artifact when complete. This class includes logic for caching,
+// cache invalidation, and merging.

[pdr., 2015/10/09 03:57:50]

+1, I think this is the right abstraction with the possible exception of
processing the items as they are produced.

 class PLATFORM_EXPORT DisplayItemList {

[pdr., 2015/10/09 03:57:50]

I'm worried about the name of this class because it is confusing how this
differs from DisplayItems and it is not clear that this is where the previous
and current paint data comes together.

What do you think about naming this "PaintController"?

[jbroman, 2015/10/13 18:20:56]

Right, as I said in the original review email, I'd like to rename it. Doing the
massive search+replace in this CL seemed like it would make the review harder.
Works for me, though I'm happy to bikeshed. The working title in my head was
"PaintArtifactBuilder", but I think I like yours more.

[pdr., 2015/10/13 18:30:32]

I sort of think of builders as having temporary state, whereas "controller" kind
of implies long-living state. On the other hand, it really is building more than
controlling. I looked at the design patterns book and couldn't find a name for
what we're doing (other than MVC-ish things). I'm happy with whatever as long as
it isn't DisplayItemList :)

     WTF_MAKE_NONCOPYABLE(DisplayItemList);
     WTF_MAKE_FAST_ALLOCATED(DisplayItemList);
@@ -136,24 +113,13 @@ public:
     // Should only be called right after commitNewDisplayItems.
     size_t approximateUnsharedMemoryUsage() const;
 
-    // Get the paint list generated after the last painting.
-    const DisplayItems& displayItems() const;
-
-    // Get the paint chunks generated after the last painting.
-    const Vector<PaintChunk>& paintChunks() const;
+    // Get the artifact generated after the last commit.
+    const PaintArtifact& paintArtifact() const;
+    const DisplayItems& displayItems() const { return paintArtifact().displayItems(); }

[pdr., 2015/10/09 03:57:50]

We could just return PaintArtifact from a finish/commit/etc step, so it's not
possible to forget to commit:

const PaintArtifact finishPaint()
{
    // commit new display items
    // do the merge algorithm
    // m_newDisplayItems = [];
    // m_newPaintChunks = [];
    // m_currentDisplayItems = result of merge algorithm.
    // m_currentPaintChunks = result of merge algorithm.

    return PaintArtifact(m_currentDisplayItems, m_currentPaintChunks);
        or
    return m_currentPaintArtifact;
}

ContentLayerDelegate's PaintContents then looks like:
displayItemList->finishPaint().appendToWebDisplayItemList(webDisplayItemList);

instead of:
displayItemList->commitNewDisplayItems();
displayItemList->paintArtifact().appendToWebDisplayItemList(webDisplayItemList);

[jbroman, 2015/10/13 18:20:56]

The tricky thing here is that this class does need to hold on to the current
display items and chunks, in order to do the merge algorithm on the next frame.
So we can't just release everything, and it's big enough that I'd like to avoid
a copy if possible.

Given that, I see two options:
- have the current paint artifact be owned by this class, and have the next step
of the pipeline access it by const ref (this is what I've targeted with this CL)
- have PaintArtifact not own the data it refers to, but only contain references
to it, in a StringPiece kind of way (this seems weird to me, since I would
intuitively expect an artifact to contain its data)
We can do that by making commitNewDisplayItems (or finishPaint, if we change its
name) return a const ref to m_currentPaintArtifact. That's an OK syntactic
shortcut, though IMHO it makes it slightly less clear what's going on (and the
number of places where this is done is few enough that I'm not worried about the
extra line of code). I'm not super committed to the later point, though.

+    const Vector<PaintChunk>& paintChunks() const { return paintArtifact().paintChunks(); }
 
     bool clientCacheIsValid(DisplayItemClient) const;
 
-    // Commits the new display items and plays back the updated display items into the given context.
-    void commitNewDisplayItemsAndReplay(GraphicsContext& context)
-    {
-        commitNewDisplayItems();
-        replay(context);
-    }
-
-    void appendToWebDisplayItemList(WebDisplayItemList*);
-    void commitNewDisplayItemsAndAppendToWebDisplayItemList(WebDisplayItemList*);
-
     bool displayItemConstructionIsDisabled() const { return m_constructionDisabled; }
     void setDisplayItemConstructionIsDisabled(const bool disable) { m_constructionDisabled = disable; }
 
@@ -197,8 +163,7 @@ public:
 
 protected:
     DisplayItemList()
-        : m_currentDisplayItems(0)
-        , m_newDisplayItems(kInitialDisplayItemsCapacity * kMaximumDisplayItemSize)
+        : m_newDisplayItems(kInitialDisplayItemsCapacity * kMaximumDisplayItemSize)
         , m_validlyCachedClientsDirty(false)
         , m_constructionDisabled(false)
         , m_textPainted(false)
@@ -238,15 +203,12 @@ private:
     void checkNoRemainingCachedDisplayItems();
 #endif
 
-    void replay(GraphicsContext&) const;
+    // The last complete paint artifact.
+    // In SPv2, this includes paint chunks as well as display items.
+    PaintArtifact m_currentPaintArtifact;

[pdr., 2015/10/09 03:57:51]

Replacing m_currentDisplayItems and m_currentPaintChunks with
m_currentPaintArtifact mostly just adds a level of indirection in this class. I
don't feel too strongly about this though.

[jbroman, 2015/10/13 18:20:56]

See my previous comment for why I did this.

Arguably I should have replaced m_currentDisplayItems with a private
currentDisplayItems() method that encompasses this indirection, but I'm not sure
whether that's substantively better.

 
-    DisplayItems m_currentDisplayItems;
+    // Data being used to build the next paint artifact.
     DisplayItems m_newDisplayItems;
-
-    // In Slimming Paint v2, paint properties (e.g. transform) useful for
-    // compositing are stored in corresponding paint chunks instead of in the
-    // display items.
-    Vector<PaintChunk> m_currentPaintChunks;
     PaintChunker m_newPaintChunks;
 
     // Contains all clients having valid cached paintings if updated.