Changes in DisplayItemClient for spv2 paint invalidation

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

Index: third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
diff --git a/third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h b/third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
index 383c57c4a145b599735722ff2a6eaa0c1a3ad277..0bb369a83ab6c02fed290a26d2ff13adf8430888 100644
--- a/third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
+++ b/third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
@@ -7,6 +7,7 @@
 
 #include "platform/PlatformExport.h"
 #include "platform/geometry/LayoutRect.h"
+#include "platform/graphics/PaintInvalidationReason.h"
 #include "wtf/Assertions.h"
 #include "wtf/text/WTFString.h"
 
@@ -32,25 +33,38 @@ namespace blink {
 class PLATFORM_EXPORT DisplayItemCacheGeneration {
     DISALLOW_NEW();
 public:
-    DisplayItemCacheGeneration() : m_value(kInvalidGeneration) { }
+    DisplayItemCacheGeneration() { invalidate(); }
 
-    void invalidate() { m_value = kInvalidGeneration; }
-    static DisplayItemCacheGeneration next() { return DisplayItemCacheGeneration(s_nextGeneration++); }
-    bool matches(const DisplayItemCacheGeneration& other)
+    void invalidate(PaintInvalidationReason reason = PaintInvalidationFull) { m_value = static_cast<Generation>(reason); }
+
+    static DisplayItemCacheGeneration next()
     {
-        return m_value != kInvalidGeneration && other.m_value != kInvalidGeneration && m_value == other.m_value;
+        // In case the value overflowed in the previous call.
+        if (s_nextGeneration < kFirstValidGeneration)

[pdr., 2016/06/27 18:55:04]

I did some back-of-the-envelope math and this would take over a month if every
frame does a full invalidation. Instead of supporting overflowing, what if we
just crash instead?

[Xianzhu, 2016/06/27 20:02:19]

There is analysis about cache generation overflow in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph....
Do you think the analysis makes sense?

+            s_nextGeneration = kFirstValidGeneration;
+        return DisplayItemCacheGeneration(s_nextGeneration++);
+    }
+
+    bool matches(const DisplayItemCacheGeneration& other) const
+    {
+        return m_value >= kFirstValidGeneration && other.m_value >= kFirstValidGeneration && m_value == other.m_value;
+    }
+
+    PaintInvalidationReason getPaintInvalidationReason() const
+    {
+        return m_value < kFirstValidGeneration ? static_cast<PaintInvalidationReason>(m_value) : PaintInvalidationNone;
     }
 
 private:
     typedef uint32_t Generation;
-    DisplayItemCacheGeneration(Generation value) : m_value(value) { }
+    explicit DisplayItemCacheGeneration(Generation value) : m_value(value) { }
 
-    static const Generation kInvalidGeneration = 0;
+    static const Generation kFirstValidGeneration = static_cast<Generation>(PaintInvalidationReasonMax) + 1;
     static Generation s_nextGeneration;
     Generation m_value;
 };
 
-// The interface for objects that can be associated with display items.
+// The class for objects that can be associated with display items.
 // A DisplayItemClient object should live at least longer than the document cycle
 // in which its display items are created during painting.
 // After the document cycle, a pointer/reference to DisplayItemClient should be
@@ -70,13 +84,6 @@ public:
     // PaintController commits new display items or the subsequence owner is invalidated.
     static void endShouldKeepAliveAllClients(const void* owner);
     static void endShouldKeepAliveAllClients();
-
-    // Called to clear should-keep-alive of DisplayItemClients in a subsequence if this
-    // object is a subsequence.
-#define ON_DISPLAY_ITEM_CLIENT_INVALIDATION() endShouldKeepAliveAllClients(this)
-#else
-    virtual ~DisplayItemClient() { }
-#define ON_DISPLAY_ITEM_CLIENT_INVALIDATION()
 #endif
 
     virtual String debugName() const = 0;
@@ -85,25 +92,23 @@ public:
     // offset by offsetFromLayoutObjectWithSubpixelAccumulation().
     virtual LayoutRect visualRect() const = 0;
 
-    virtual bool displayItemsAreCached(DisplayItemCacheGeneration) const = 0;
-    virtual void setDisplayItemsCached(DisplayItemCacheGeneration) const = 0;
-    virtual void setDisplayItemsUncached() const = 0;
-};
+    bool displayItemsAreCached(DisplayItemCacheGeneration cacheGeneration) const { return m_cacheGeneration.matches(cacheGeneration); }
+    void setDisplayItemsCached(DisplayItemCacheGeneration cacheGeneration) const { m_cacheGeneration = cacheGeneration; }
+    void setDisplayItemsUncached(PaintInvalidationReason reason = PaintInvalidationFull) const
+    {
+        m_cacheGeneration.invalidate(reason);
+#if CHECK_DISPLAY_ITEM_CLIENT_ALIVENESS
+        // Clear should-keep-alive of DisplayItemClients in a subsequence if this
+        // object is a subsequence.
+        endShouldKeepAliveAllClients(this);
+#endif
+    }
 
-#define DISPLAY_ITEM_CACHE_STATUS_IMPLEMENTATION \
-    bool displayItemsAreCached(DisplayItemCacheGeneration cacheGeneration) const final { return m_cacheGeneration.matches(cacheGeneration); } \
-    void setDisplayItemsCached(DisplayItemCacheGeneration cacheGeneration) const final { m_cacheGeneration = cacheGeneration; } \
-    void setDisplayItemsUncached() const final \
-    { \
-        m_cacheGeneration.invalidate(); \
-        ON_DISPLAY_ITEM_CLIENT_INVALIDATION(); \
-    } \
-    mutable DisplayItemCacheGeneration m_cacheGeneration;
+    PaintInvalidationReason getPaintInvalidationReason() const { return m_cacheGeneration.getPaintInvalidationReason(); }
 
-#define DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION \
-    bool displayItemsAreCached(DisplayItemCacheGeneration) const final { return false; } \
-    void setDisplayItemsCached(DisplayItemCacheGeneration) const final { } \
-    void setDisplayItemsUncached() const final { }
+private:
+    mutable DisplayItemCacheGeneration m_cacheGeneration;

[pdr., 2016/06/27 18:55:04]

It's confusing to me that we're using cache generation for both a generation and
for bits for the paint invalidation reason. Can this be hidden behind a struct
with just a single uint32 member and functions to hide the details?

[Xianzhu, 2016/06/27 20:02:19]

I'm not sure if I understand your comment. I think DisplayItemCacheGeneration is
the struct to hide the details but it may need a better name.

In the new patch set I renamed DisplayItemCacheGeneration to
CacheGenerationOrInvalidationReason and moved it into the private section of
DisplayItemClient.

+};
 
 inline bool operator==(const DisplayItemClient& client1, const DisplayItemClient& client2) { return &client1 == &client2; }
 inline bool operator!=(const DisplayItemClient& client1, const DisplayItemClient& client2) { return &client1 != &client2; }