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

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

Merge them so that we can easily know which rect is from which object.
Also output paintInvalidationReason.

BUG=602483
Committed: https://crrev.com/3c4c810b9fa4a817d63c9961f517d351477b9783
Cr-Commit-Position: refs/heads/master@{#386594}

[Xianzhu, 2016-04-11 22:57:19]

Description was changed from

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

Merge them so that we can easily know which rect is from which object.
Also output paintInvalidationReason.
==========

to

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

Merge them so that we can easily know which rect is from which object.
Also output paintInvalidationReason.

BUG=602483
==========

[Xianzhu, 2016-04-11 23:27:20]

wangxianzhu@chromium.org changed reviewers:
+ pdr@chromium.org, wkorman@chromium.org

[Xianzhu, 2016-04-11 23:27:21]

Sample new output (for fast/repaint/absolute-position-moved.html) (You'll also
see them in try bot results):

{
  "bounds": [800, 2016],
  "children": [
    {
      "bounds": [800, 2016],
      "contentsOpaque": true,
      "drawsContent": true,
      "paintInvalidations": [
        {
          "object": "LayoutBlockFlow (positioned) DIV id='absoluteDiv'
class='absolute green'",
          "rect": [108, 5100, 100, 100],
          "reason": "layoutObject removal"
        },
        {
          "object": "LayoutBlockFlow (positioned) DIV id='absoluteDiv'
class='absolute green'",
          "rect": [100, 100, 100, 100],
          "reason": "location change"
        }
      ]
    }
  ]
}

Will update TestExpectations for NeedsRebaselines after try jobs finish.

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

This is clever and seems like a good direction. I have a bunch of small
questions though.

A lot of the full paint invalidations have rects of 0,0,0,0. What causes those
0's?

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h:287: void
setBackingNeedsPaintInvalidationInRect(const LayoutRect&,
PaintInvalidationReason, const LayoutObject&) const; // r is in the coordinate
space of this layout object
Not your fault but can you move this comment above the function like so:
// Indicate that the contents of this layoutObject need to be repainted.
// This only has an effect if compositing is being used. The rect is in the
// coordinate space of this layout object.
void setBackingNeedsPaintInvalidationInRect(...)

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1396:
invalidateDisplayItemClientsWithPaintInvalidationState(paintInvalidationContainer,
paintInvalidationState, PaintInvalidationSelection);
This change seems like it would change behavior since a full invalidation no
longer calls invalidateDisplayItemClientsWithPaintInvalidationState. Is that
intended?

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:76: String object;
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##?

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:78: const char*
reason;
Could we use PaintInvalidationReason directly here? If not, why don't we use a
String instead of a char array?

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1030:
trackPaintInvalidation("##ALL##", FloatRect(FloatPoint(), m_size), "");
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?

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:363: const
DisplayItemClient* m_lastSetNeedsDisplayInRectDisplayItemClient;
It doesn't seem right to add debug-only members to GraphicsLayer. Is it possible
to refactor the code so this isn't needed? Can you describe why this wasn't
needed before?

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/LinkHighlightImpl.cpp (right):

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/LinkHighlightImpl.cpp:351:
m_currentGraphicsLayer->trackPaintInvalidation("LinkHighlight",
FloatRect(layer()->position().x, layer()->position().y, layer()->bounds().width,
layer()->bounds().height), "");
These cases with empty reasons seem like bugs to me. Should we add
PaintInvalidationReasons for these?

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

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h:287: void
setBackingNeedsPaintInvalidationInRect(const LayoutRect&,
PaintInvalidationReason, const LayoutObject&) const; // r is in the coordinate
space of this layout object
On 2016/04/12 00:20:06, pdr wrote:
> Not your fault but can you move this comment above the function like so:
> // Indicate that the contents of this layoutObject need to be repainted.
> // This only has an effect if compositing is being used. The rect is in the
> // coordinate space of this layout object.
> void setBackingNeedsPaintInvalidationInRect(...)

Done.

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1396:
invalidateDisplayItemClientsWithPaintInvalidationState(paintInvalidationContainer,
paintInvalidationState, PaintInvalidationSelection);
On 2016/04/12 00:20:06, pdr wrote:
> This change seems like it would change behavior since a full invalidation no
> longer calls invalidateDisplayItemClientsWithPaintInvalidationState. Is that
> intended?

Oh I missed the view()->doingFullPaintInvalidation() condition, under which we
don't need to invalidate the rect, but need to invalidate the client. Corrected
and added a TODO.

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:76: String object;
On 2016/04/12 00:20:06, pdr wrote:
> 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##?

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.

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:78: const char*
reason;
On 2016/04/12 00:20:06, pdr wrote:
> Could we use PaintInvalidationReason directly here? If not, why don't we use a
> String instead of a char array?

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.

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1030:
trackPaintInvalidation("##ALL##", FloatRect(FloatPoint(), m_size), "");
On 2016/04/12 00:20:06, pdr wrote:
> 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?

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

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:363: const
DisplayItemClient* m_lastSetNeedsDisplayInRectDisplayItemClient;
On 2016/04/12 00:20:06, pdr wrote:
> It doesn't seem right to add debug-only members to GraphicsLayer. Is it
possible
> to refactor the code so this isn't needed? Can you describe why this wasn't
> needed before?

Found a better way to handle this.

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/LinkHighlightImpl.cpp (right):

https://codereview.chromium.org/1880763002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/LinkHighlightImpl.cpp:351:
m_currentGraphicsLayer->trackPaintInvalidation("LinkHighlight",
FloatRect(layer()->position().x, layer()->position().y, layer()->bounds().width,
layer()->bounds().height), "");
On 2016/04/12 00:20:06, pdr wrote:
> These cases with empty reasons seem like bugs to me. Should we add
> PaintInvalidationReasons for these?

Now use default PaintInvalidationFull. The client name already conveys enough
information for the paint invalidation.

[wkorman, 2016-04-12 02:34:03]

lgtm

LGTM pending pdr@ approval.

[pdr., 2016-04-12 03:09:24]

LGTM

https://codereview.chromium.org/1880763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/1880763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:213: void
trackPaintInvalidation(const DisplayItemClient&, const FloatRect&,
PaintInvalidationReason = PaintInvalidationFull);
Nit: can you remove the default value here? I think it will help us catch bugs
if we force the callsites to list "PaintInvalidationFull".

[Xianzhu, 2016-04-12 04:05:23]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-04-12 04:05:24]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/1880763002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-04-12 04:05:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1880763002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1880763002/40001

[Xianzhu, 2016-04-12 04:09:59]

https://codereview.chromium.org/1880763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/1880763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:213: void
trackPaintInvalidation(const DisplayItemClient&, const FloatRect&,
PaintInvalidationReason = PaintInvalidationFull);
On 2016/04/12 03:09:24, pdr wrote:
> Nit: can you remove the default value here? I think it will help us catch bugs
> if we force the callsites to list "PaintInvalidationFull".

Done.

[commit-bot: I haz the power, 2016-04-12 05:41:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-12 05:43:09]

Description was changed from

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

Merge them so that we can easily know which rect is from which object.
Also output paintInvalidationReason.

BUG=602483
==========

to

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

Merge them so that we can easily know which rect is from which object.
Also output paintInvalidationReason.

BUG=602483

Committed: https://crrev.com/3c4c810b9fa4a817d63c9961f517d351477b9783
Cr-Commit-Position: refs/heads/master@{#386594}
==========

[commit-bot: I haz the power, 2016-04-12 05:43:11]

Patchset 3 (id:??) landed as
https://crrev.com/3c4c810b9fa4a817d63c9961f517d351477b9783
Cr-Commit-Position: refs/heads/master@{#386594}