Append record to timeline when drawing on HTML Canvas causes invalidation, which leads to deferred repaint.
Created attachment 140233 [details] Patch
Comment on attachment 140233 [details] Patch Attachment 140233 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12630166 New failing tests: inspector/timeline/timeline-enum-stability.html
Created attachment 140245 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 140233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140233&action=review > Source/WebCore/ChangeLog:8 > + Append record to timeline when drawing on HTML Canvas causes invalidation, Do you want a stack that caused repaint here? Why are you only interested in canvas invalidations? > Source/WebCore/html/HTMLCanvasElement.cpp:232 > + InspectorInstrumentation::didInvalidateRect(this->document(), dirtyRect); This is only a fraction of invalidations, a more specific name is needed. Also, why do we want to instrument invalidation? Are you trying to trace the subsequent paint from InspectorInstrumentation::willPaint to it? > Source/WebCore/inspector/front-end/TimelineModel.js:82 > + InvalidateRect: "InvalidateRect" That's what is making the test fail, you should update the test.
Created attachment 140392 [details] Patch
Comment on attachment 140233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140233&action=review >> Source/WebCore/ChangeLog:8 >> + Append record to timeline when drawing on HTML Canvas causes invalidation, > > Do you want a stack that caused repaint here? Why are you only interested in canvas invalidations? Stack is captured as well. >> Source/WebCore/html/HTMLCanvasElement.cpp:232 >> + InspectorInstrumentation::didInvalidateRect(this->document(), dirtyRect); > > This is only a fraction of invalidations, a more specific name is needed. Also, why do we want to instrument invalidation? Are you trying to trace the subsequent paint from InspectorInstrumentation::willPaint to it? Renamed to didInvalidateCanvasRect. We want to instrument invalidation to show to user code point that initiated deferred painting. Currently we do not link invalidation with subsequent painting. >> Source/WebCore/inspector/front-end/TimelineModel.js:82 >> + InvalidateRect: "InvalidateRect" > > That's what is making the test fail, you should update the test. Fixed.
Comment on attachment 140392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140392&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:232 > + InspectorInstrumentation::didInvalidateCanvasRect(this->document(), dirtyRect); simply document() > Source/WebCore/inspector/TimelineRecordFactory.cpp:196 > +PassRefPtr<InspectorObject> TimelineRecordFactory::createInvalidateCanvasRectData(const IntRect& rect) This is just a rect data, you should re-use createPaintData. > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:519 > + contentHelper._appendTextRow(WebInspector.UIString("Location"), WebInspector.UIString("(%d, %d)", this.data["x"], this.data["y"])); Please add corresponding localizedStrings.
Comment on attachment 140392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140392&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:232 >> + InspectorInstrumentation::didInvalidateCanvasRect(this->document(), dirtyRect); > > simply document() Done. >> Source/WebCore/inspector/TimelineRecordFactory.cpp:196 >> +PassRefPtr<InspectorObject> TimelineRecordFactory::createInvalidateCanvasRectData(const IntRect& rect) > > This is just a rect data, you should re-use createPaintData. Did you mean: convert to IntRect to FloatRect and pass it to createPaintData? >> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:519 >> + contentHelper._appendTextRow(WebInspector.UIString("Location"), WebInspector.UIString("(%d, %d)", this.data["x"], this.data["y"])); > > Please add corresponding localizedStrings. Done.
Created attachment 141108 [details] Patch
Could you provide a screenshot with the instrumented data on any of the public sites as an attachment?
Created attachment 141177 [details] New instrumentation records.
@nduca: do you think it makes sense to instrument canvas's invalidation separately or should it be a part of a more generic invalidate instrumentation?
Comment on attachment 141108 [details] Patch The challenge here is the amount of invalidation that you can get would easily overwhelm a UI. E.g. what if I do this on a canvas? for(var i= 0; i < 1000; i++) ctx.fillRect(i,0,1,100); I think this could would put 1000 different entries in my timeline, each of which was 1 wider than the previous. An event and call stack on *first* invalidation might be good for canvas, but it often just points at recalc/layout for DOM nodes... This is a long winded way of me saying "yes, Pavel, it might be good to understand the general problem a bit more." :)
Created attachment 141674 [details] Patch
Created attachment 141676 [details] Patch
Fixed: now incalidation record appears only when "clean" canvas becomes "dirty".
Comment on attachment 141676 [details] Patch Clearing r? until we come to an agreement on whether it should be done this way.
Closing as invalid, as this bug pertains to the old inspector UI and/or its tests. Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.