Bug 100584

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.