Factor PaintArtifact out of DisplayItemList.

third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.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 DisplayItemList_h
 #define DisplayItemList_h
 
 #include "platform/PlatformExport.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/geometry/IntRect.h"
 #include "platform/geometry/LayoutPoint.h"
 #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"
 #include "wtf/Alignment.h"
 #include "wtf/HashMap.h"
 #include "wtf/PassOwnPtr.h"
 #include "wtf/Utility.h"
 #include "wtf/Vector.h"
 
 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);
 public:
     static PassOwnPtr<DisplayItemList> create()
     {
         return adoptPtr(new DisplayItemList());
     }
 
     // These methods are called during paint invalidation (or paint if SlimmingPaintSynchronizedPainting is on).
     void invalidate(const DisplayItemClientWrapper&, PaintInvalidationReason, const IntRect& previousPaintInvalidationRect, const IntRect& newPaintInvalidationRect);
@@ skipping 59 matching lines @@
 
     // Must be called when a painting is finished. If passed, invalidations are recorded on the given
     // GraphicsLayer.
     void commitNewDisplayItems(GraphicsLayer* = 0);
 
     // Returns the approximate memory usage, excluding memory likely to be
     // shared with the embedder after copying to WebDisplayItemList.
     // 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; }
 
     bool textPainted() const { return m_textPainted; }
     void setTextPainted() { m_textPainted = true; }
 
     // Returns displayItems added using createAndAppend() since beginning or the last
     // commitNewDisplayItems(). Use with care.
     DisplayItems& newDisplayItems() { return m_newDisplayItems; }
 
@@ skipping 23 matching lines @@
         return m_clientsCheckedPaintInvalidation.contains(client);
     }
     void setClientHasCheckedPaintInvalidation(DisplayItemClient client)
     {
         ASSERT(RuntimeEnabledFeatures::slimmingPaintSynchronizedPaintingEnabled());
         m_clientsCheckedPaintInvalidation.add(client);
     }
 
 protected:
     DisplayItemList()
-        : m_currentDisplayItems(0)
-        , m_newDisplayItems(kInitialDisplayItemsCapacity * kMaximumDisplayItemSize)
+        : m_newDisplayItems(kInitialDisplayItemsCapacity * kMaximumDisplayItemSize)
         , m_validlyCachedClientsDirty(false)
         , m_constructionDisabled(false)
         , m_textPainted(false)
         , m_skippingCacheCount(0)
         , m_numCachedItems(0)
         , m_nextScope(1) { }
 
 private:
     // Set new item state (scopes, cache skipping, etc) for a new item.
     void processNewItem(DisplayItem&);
@@ skipping 19 matching lines @@
     void copyCachedSubsequence(DisplayItems::iterator& currentIt, DisplayItems& updatedList);
 
 #if ENABLE(ASSERT)
     // The following two methods are for checking under-invalidations
     // (when RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled).
     void checkUnderInvalidation(DisplayItems::iterator& newIt, DisplayItems::iterator& currentIt);
     void checkCachedDisplayItemIsUnchanged(const char* messagePrefix, const DisplayItem& newItem, const DisplayItem& oldItem);
     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.
     // It's lazily updated in updateValidlyCachedClientsIfNeeded().
     // TODO(wangxianzhu): In the future we can replace this with client-side repaint flags
     // to avoid the cost of building and querying the hash table.
     mutable HashSet<DisplayItemClient> m_validlyCachedClients;
     mutable bool m_validlyCachedClientsDirty;
 
     // Used during painting. Contains clients that have checked paint invalidation and
@@ skipping 34 matching lines @@
     // the duplicated ids are from.
     DisplayItemIndicesByClientMap m_newDisplayItemIndicesByClient;
 #endif
 
     OwnPtr<Vector<String>> m_trackedPaintInvalidationObjects;
 };
 
 } // namespace blink
 
 #endif // DisplayItemList_h