Bug 100584 – Add support for text-based repaint testing

• Component: WebCore Misc.
• Status: RESOLVED FIXED

Repaint testing is currently done using testRunner.display(), which ultimately requires brittle pixel comparisons. Ideally, we would be able to dump the repaint rects as text (which would hopefully be more consistent across ports).

Created attachment 171070 [details]
Patch

Initial revision -- windows build and layout tests will likely fail.

[Build Bot]

Comment on attachment 171070 [details]
Patch

Attachment 171070 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14613285

[EFL EWS Bot]

Comment on attachment 171070 [details]
Patch

Attachment 171070 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14612250

[Build Bot]

Comment on attachment 171070 [details]
Patch

Attachment 171070 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14603554

[kov's GTK+ EWS bot]

Comment on attachment 171070 [details]
Patch

Attachment 171070 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14606326

[Raphael Kubo da Costa (:rakuco)]

Please see bug 99480 as well.

[Sami Kyöstilä]

Comment on attachment 171070 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171070&action=review

Great stuff! This conflicts a bit with the patch in bug 97801, right? Maybe we should land that one first to get the foundation in place.

> LayoutTests/fast/repaint/canvas-putImageData-expected.txt:1
> + (repaint rects

Any idea where the extra space at the start of the line is coming from and is it expected? The other test doesn't seem to have it.

> LayoutTests/fast/repaint/canvas-putImageData.html:-3
> -    <script src="resources/repaint.js"></script>

We can't delete repaint.js yet, but it might be worth adding a comment banner there saying that pixel repaint tests are deprecated.

[Sami Kyöstilä]

(In reply to comment #7)
> This conflicts a bit with the patch in bug 97801, right? Maybe we should land that one first to get the foundation in place.

Ah, I only now realized that this is the FrameView-part of that patch split off, right? Never mind.

Created attachment 171484 [details]
Patch

This revision should hopefully pass on mac/chromium. Should fail on win.

[Simon Fraser (smfr)]

Comment on attachment 171484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171484&action=review

> Source/WebCore/ChangeLog:33
> +        * WebCore.exp.in:
> +        * page/Frame.cpp:
> +        (WebCore::Frame::trackedRepaintRectsAsText):
> +        (WebCore):
> +        * page/Frame.h:
> +        (Frame):
> +        * page/FrameView.cpp:
> +        (WebCore::FrameView::setTracksRepaints):
> +        (WebCore::FrameView::resetTrackedRepaints):
> +        (WebCore):
> +        (WebCore::FrameView::trackedRepaintRectsAsText):
> +        * page/FrameView.h:
> +        (FrameView):
> +        * testing/Internals.cpp:
> +        (WebCore::Internals::repaintRectsAsText):
> +        (WebCore):
> +        (WebCore::Internals::startTrackingRepaints):
> +        (WebCore::Internals::stopTrackingRepaints):
> +        * testing/Internals.h:
> +        * testing/Internals.idl:

You should remove redundant lines, and annotate the method to describe the changes you made.

> Source/WebCore/testing/Internals.cpp:1414
> +    frameView->setTracksRepaints(true);
> +    frameView->resetTrackedRepaints();

These two lines are all over the place. If this is the contract (that starting tracking clears existing), then the reset should be moved to the FrameView code.

> Source/WebCore/testing/Internals.idl:204
> +    void startTrackingRepaints(in Document document) raises (DOMException);
> +    void stopTrackingRepaints(in Document document) raises (DOMException);

It's not clear whether starting clears previous rects, and whether rects are cleared on stopping. Is start/stop/dump OK? Or does it have to be start/dump/stop? Some comments should be added to clarify.

[Build Bot]

Comment on attachment 171484 [details]
Patch

Attachment 171484 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14641568

Created attachment 171510 [details]
Patch

(In reply to comment #10)
> (From update of attachment 171484 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171484&action=review
> 
> You should remove redundant lines, and annotate the method to describe the changes you made.
Done.
> 
> > Source/WebCore/testing/Internals.cpp:1414
> > +    frameView->setTracksRepaints(true);
> > +    frameView->resetTrackedRepaints();
> 
> These two lines are all over the place. If this is the contract (that starting tracking clears existing), then the reset should be moved to the FrameView code.
I had done all this to allow start/stop/dump (I didn't want stop to clear the rects), but in retrospect, start/dump/stop makes more sense -- far less code would need to be modified.

So I've gone back to having FrameView::setTracksRepaints clear the list of repaint rects. This let me stop littering everyone's code with resetTrackedRepaints.
> 
> > Source/WebCore/testing/Internals.idl:204
> > +    void startTrackingRepaints(in Document document) raises (DOMException);
> > +    void stopTrackingRepaints(in Document document) raises (DOMException);
> 
> It's not clear whether starting clears previous rects, and whether rects are cleared on stopping. Is start/stop/dump OK? Or does it have to be start/dump/stop? Some comments should be added to clarify.
Done.

[Build Bot]

Comment on attachment 171510 [details]
Patch

Attachment 171510 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14632633

Created attachment 171515 [details]
Patch

Fixes a copy/paste error in WebKit2*.def

[Raphael Kubo da Costa (:rakuco)]

*** Bug 99480 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 171515 [details]
Patch

Rejecting attachment 171515 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
st/repaint/layer-full-repaint-expected.txt'
rm 'LayoutTests/platform/qt/fast/repaint/layer-full-repaint-expected.png'
patching file LayoutTests/platform/qt/fast/repaint/layer-full-repaint-expected.txt
rm 'LayoutTests/platform/qt/fast/repaint/layer-full-repaint-expected.txt'
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Simon Fras..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14678101

Created attachment 171626 [details]
Patch

Rebasing. Hopefully it'll make it through the queue this time.

[WebKit Review Bot]

Comment on attachment 171626 [details]
Patch

Clearing flags on attachment: 171626

Committed r133033: <http://trac.webkit.org/changeset/133033>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.