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.
Comment on attachment 171070 [details] Patch Attachment 171070 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14613285
Comment on attachment 171070 [details] Patch Attachment 171070 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14612250
Comment on attachment 171070 [details] Patch Attachment 171070 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14603554
Comment on attachment 171070 [details] Patch Attachment 171070 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14606326
Please see bug 99480 as well.
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.
(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.
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.
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.
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
*** Bug 99480 has been marked as a duplicate of this bug. ***
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.
Comment on attachment 171626 [details] Patch Clearing flags on attachment: 171626 Committed r133033: <http://trac.webkit.org/changeset/133033>
All reviewed patches have been landed. Closing bug.