Merge repaintRects and paintInvalidationObjects in text-based-repaint test results

third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp

Index: third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp b/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
index e7cdddf419d77fe823c6e5bf894baa111557dcf1..78bf1071c74a5dfdc4ffc06e1f9170446b941bf7 100644
--- a/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
+++ b/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
@@ -71,14 +71,14 @@ namespace blink {
 
 static bool s_drawDebugRedFill = true;
 
-// TODO(wangxianzhu): Remove this when we no longer invalidate rects.
-struct PaintInvalidationTrackingInfo {
+struct PaintInvalidationInfo {
     DISALLOW_NEW_EXCEPT_PLACEMENT_NEW();
-    Vector<FloatRect> invalidationRects;
-    Vector<String> invalidationObjects;
+    String object;

[pdr., 2016/04/12 00:20:06]

Why pass a string instead of a DisplayItemClient? Is it just for the case of
##ALL##? If so, what if we just dumped all of the invalidations instead of
outputting ##ALL##?

[Xianzhu, 2016/04/12 01:14:26]

Another case is LinkHighlightImpl which is not a DisplayItemClient.

Now I changed the parameter of trackPaintInvalidation to DisplayItemClient, but
we still can't just use DisplayItemClient here because invalidated objects may
have died when we dump the results.

+    FloatRect rect;
+    const char* reason;

[pdr., 2016/04/12 00:20:06]

Could we use PaintInvalidationReason directly here? If not, why don't we use a
String instead of a char array?

[Xianzhu, 2016/04/12 01:14:26]

Changed to PaintInvalidationReason.

Used const char* because reason is always a literal and I wanted to use
non-pre-defined reason e.g. for LinkHightlight. Now use PaintInvalidationFull.

 };
 
-typedef HashMap<const GraphicsLayer*, PaintInvalidationTrackingInfo> PaintInvalidationTrackingMap;
+typedef HashMap<const GraphicsLayer*, Vector<PaintInvalidationInfo>> PaintInvalidationTrackingMap;
 static PaintInvalidationTrackingMap& paintInvalidationTrackingMap()
 {
     DEFINE_STATIC_LOCAL(PaintInvalidationTrackingMap, map, ());
@@ -120,6 +120,7 @@ GraphicsLayer::GraphicsLayer(GraphicsLayerClient* client)
     , m_contentsLayerId(0)
     , m_scrollableArea(nullptr)
     , m_3dRenderingContext(0)
+    , m_lastSetNeedsDisplayInRectDisplayItemClient(nullptr)
 {
 #if ENABLE(ASSERT)
     if (m_client)
@@ -537,43 +538,23 @@ bool GraphicsLayer::hasTrackedPaintInvalidations() const
 {
     PaintInvalidationTrackingMap::iterator it = paintInvalidationTrackingMap().find(this);
     if (it != paintInvalidationTrackingMap().end())
-        return !it->value.invalidationRects.isEmpty();
+        return !it->value.isEmpty();
     return false;
 }
 
-void GraphicsLayer::trackPaintInvalidationRect(const FloatRect& rect)
+void GraphicsLayer::trackPaintInvalidation(const String& object, const FloatRect& rect, const char* reason)
 {
-    if (rect.isEmpty())
-        return;
-
     // The caller must check isTrackingPaintInvalidations() before calling this method
     // to avoid constructing the rect unnecessarily.
     ASSERT(isTrackingPaintInvalidations());
 
-    paintInvalidationTrackingMap().add(this, PaintInvalidationTrackingInfo()).storedValue->value.invalidationRects.append(rect);
+    PaintInvalidationInfo info = { object, rect, reason };
+    paintInvalidationTrackingMap().add(this, Vector<PaintInvalidationInfo>()).storedValue->value.append(info);
 }
 
-void GraphicsLayer::trackPaintInvalidationObject(const String& objectDebugString)
+static bool comparePaintInvalidationInfo(const PaintInvalidationInfo& a, const PaintInvalidationInfo& b)
 {
-    if (objectDebugString.isEmpty())
-        return;
-
-    // The caller must check isTrackingPaintInvalidations() before calling this method
-    // because constructing the debug string will be costly.
-    ASSERT(isTrackingPaintInvalidations());
-
-    paintInvalidationTrackingMap().add(this, PaintInvalidationTrackingInfo()).storedValue->value.invalidationObjects.append(objectDebugString);
-}
-
-static bool compareFloatRects(const FloatRect& a, const FloatRect& b)
-{
-    if (a.x() != b.x())
-        return a.x() > b.x();
-    if (a.y() != b.y())
-        return a.y() > b.y();
-    if (a.width() != b.width())
-        return a.width() > b.width();
-    return a.height() > b.height();
+    return codePointCompareLessThan(a.object, b.object);
 }
 
 template <typename T>
@@ -725,27 +706,19 @@ PassRefPtr<JSONObject> GraphicsLayer::layerTreeAsJSON(LayerTreeFlags flags, Rend
 
     PaintInvalidationTrackingMap::iterator it = paintInvalidationTrackingMap().find(this);
     if (it != paintInvalidationTrackingMap().end()) {
-        if (flags & LayerTreeIncludesPaintInvalidationRects) {
-            Vector<FloatRect>& rects = it->value.invalidationRects;
-            if (!rects.isEmpty()) {
-                std::sort(rects.begin(), rects.end(), &compareFloatRects);
-                RefPtr<JSONArray> rectsJSON = JSONArray::create();
-                for (auto& rect : rects) {
-                    if (rect.isEmpty())
-                        continue;
-                    rectsJSON->pushArray(rectAsJSONArray(rect));
+        if (flags & LayerTreeIncludesPaintInvalidations) {
+            Vector<PaintInvalidationInfo>& infos = it->value;
+            if (!infos.isEmpty()) {
+                std::stable_sort(infos.begin(), infos.end(), &comparePaintInvalidationInfo);
+                RefPtr<JSONArray> paintInvalidationsJSON = JSONArray::create();
+                for (auto& info : infos) {
+                    RefPtr<JSONObject> infoJSON = JSONObject::create();
+                    infoJSON->setString("object", info.object);
+                    infoJSON->setArray("rect", rectAsJSONArray(info.rect));
+                    infoJSON->setString("reason", info.reason);
+                    paintInvalidationsJSON->pushObject(infoJSON);
                 }
-                json->setArray("repaintRects", rectsJSON);
-            }
-        }
-
-        if (flags & LayerTreeIncludesPaintInvalidationObjects) {
-            Vector<String>& clients = it->value.invalidationObjects;
-            if (!clients.isEmpty()) {
-                RefPtr<JSONArray> clientsJSON = JSONArray::create();
-                for (auto& clientString : clients)
-                    clientsJSON->pushString(clientString);
-                json->setArray("paintInvalidationClients", clientsJSON);
+                json->setArray("paintInvalidations", paintInvalidationsJSON);
             }
         }
     }
@@ -1038,7 +1011,7 @@ void GraphicsLayer::setContentsNeedsDisplay()
     if (WebLayer* contentsLayer = contentsLayerIfRegistered()) {
         contentsLayer->invalidate();
         if (isTrackingPaintInvalidations())
-            trackPaintInvalidationRect(m_contentsRect);
+            trackPaintInvalidation("GraphicsLayerContents", m_contentsRect, "");
     }
 }
 
@@ -1049,17 +1022,15 @@ void GraphicsLayer::setNeedsDisplay()
 
     // TODO(chrishtr): stop invalidating the rects once FrameView::paintRecursively does so.
     m_layer->layer()->invalidate();
-    if (isTrackingPaintInvalidations())
-        trackPaintInvalidationRect(FloatRect(FloatPoint(), m_size));
     for (size_t i = 0; i < m_linkHighlights.size(); ++i)
         m_linkHighlights[i]->invalidate();
-
     getPaintController().invalidateAll();
+
     if (isTrackingPaintInvalidations())
-        trackPaintInvalidationObject("##ALL##");
+        trackPaintInvalidation("##ALL##", FloatRect(FloatPoint(), m_size), "");

[pdr., 2016/04/12 00:20:06]

I only see this output in 4 tests, but there are 16 callers to
setNeedsDisplay(). Is there a bug here, or do we just have poor test coverage?

[Xianzhu, 2016/04/12 01:14:26]

We may have test coverage for the other setNeedsDisplay(), but just not covered
by repaint tests.

 }
 
-void GraphicsLayer::setNeedsDisplayInRect(const IntRect& rect, PaintInvalidationReason invalidationReason)
+void GraphicsLayer::setNeedsDisplayInRect(const IntRect& rect, PaintInvalidationReason invalidationReason, const DisplayItemClient& client)
 {
     if (!drawsContent())
         return;
@@ -1067,20 +1038,24 @@ void GraphicsLayer::setNeedsDisplayInRect(const IntRect& rect, PaintInvalidation
     m_layer->layer()->invalidateRect(rect);
     if (firstPaintInvalidationTrackingEnabled())
         m_debugInfo.appendAnnotatedInvalidateRect(rect, invalidationReason);
-    if (isTrackingPaintInvalidations())
-        trackPaintInvalidationRect(rect);
     for (size_t i = 0; i < m_linkHighlights.size(); ++i)
         m_linkHighlights[i]->invalidate();
+
+    if (isTrackingPaintInvalidations()) {
+        m_lastSetNeedsDisplayInRectDisplayItemClient = &client;
+        trackPaintInvalidation(client.debugName(), rect, paintInvalidationReasonToString(invalidationReason));
+    }
 }
 
-void GraphicsLayer::invalidateDisplayItemClient(const DisplayItemClient& displayItemClient, PaintInvalidationReason)
+void GraphicsLayer::invalidateDisplayItemClient(const DisplayItemClient& displayItemClient, PaintInvalidationReason invalidationReason)
 {
     if (!drawsContent())
         return;
 
     getPaintController().invalidate(displayItemClient);
-    if (isTrackingPaintInvalidations())
-        trackPaintInvalidationObject(displayItemClient.debugName());
+
+    if (isTrackingPaintInvalidations() && m_lastSetNeedsDisplayInRectDisplayItemClient != &displayItemClient)
+        trackPaintInvalidation(displayItemClient.debugName(), FloatRect(), paintInvalidationReasonToString(invalidationReason));
 }
 
 void GraphicsLayer::setContentsRect(const IntRect& rect)