[webkit-reviews] review requested: [Bug 100584] Add support for text-based repaint testing : [Attachment 171510] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 100584: Add support for text-based repaint testing
https://bugs.webkit.org/show_bug.cgi?id=100584

Attachment 171510: Patch
https://bugs.webkit.org/attachment.cgi?id=171510&action=review

------- Additional Comments from vollick at chromium.org
(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.