Bug 85624 – Web Inspector: HTMLCanvasElement::didDraw instrumentation.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

Append record to timeline when drawing on HTML Canvas causes invalidation, which leads to deferred repaint.

Created attachment 140233 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Pavel Feldman]

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.

[Pavel Feldman]

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

[Pavel Feldman]

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.

[Pavel Feldman]

@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?

[Nat Duca]

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".

[Yury Semikhatsky]

Comment on attachment 141676 [details]
Patch

Clearing r? until we come to an agreement on whether it should be done this way.

[Brian Burg]

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.