Bug 67986 – Web Inspector: requestAnimationFrame callbacks don't show up in the timeline panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[James Robinson]

See title.  The calls into javascript are in Source/WebCore/dom/ScriptedAnimationController.cpp, looks like we just need to hook something up from there through InspectorInstrumentation through the TimelineAgent and so forth.

[Ilya Tikhonovsky]

Created attachment 107155 [details]
[patch] initial version

[Ilya Tikhonovsky]

Created attachment 107156 [details]
screenshot

[Ilya Tikhonovsky]

Created attachment 107163 [details]
[patch] second iteration.

[Ilya Tikhonovsky]

Created attachment 107164 [details]
screenshot

[Ilya Tikhonovsky]

Created attachment 107165 [details]
[patch] second iteration. Fire Animation Callback was renamed to Animation Frame Event in Timeline UI

[Pavel Feldman]

Comment on attachment 107165 [details]
[patch] second iteration. Fire Animation Callback was renamed to Animation Frame Event in Timeline UI

View in context: https://bugs.webkit.org/attachment.cgi?id=107165&action=review

Could you please add a test to the new type of the events?

> Source/WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:88
> +    if (InspectorInstrumentation::hasFrontends()) {

This copy/paste seems a bit too large.

> Source/WebCore/dom/ScriptedAnimationController.cpp:131
> +                InspectorInstrumentation::didFireAnimationFrameEvent(m_document);

You should surround only handleEvent with these, no need to include reset of the code.

> Source/WebCore/inspector/front-end/TimelinePanel.js:413
> +            this._registeredAnimationCallbackRecords[record.data.callbackId] = record;

callbackId -> id

> Source/WebCore/inspector/front-end/TimelinePanel.js:1015
> +    } else if (record.type === recordTypes.FireAnimationFrameEvent || record.type === recordTypes.CancelAnimationFrameCallback) {

I don't think we should set original call site to the cancel animation event (to be consistent with timer).

[Ilya Tikhonovsky]

Created attachment 107185 [details]
[patch] third version. comments addressed.

[Ilya Tikhonovsky]

Created attachment 107187 [details]
[patch] third version. with fixed style error

[WebKit Review Bot]

Attachment 107185 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1

Source/WebCore/bindings/v8/V8Proxy.h:192:  The parameter name "function" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 107187 [details]
[patch] third version. with fixed style error

View in context: https://bugs.webkit.org/attachment.cgi?id=107187&action=review

> LayoutTests/inspector/timeline/timeline-test.js:55
> +        if (async && waitForTypeName) {

I'd move this into the new test unless there are more clients.

> Source/WebCore/bindings/v8/V8Proxy.cpp:844
> +void V8Proxy::functionOrigin(v8::Handle<v8::Function> function, String* resourceName, int* lineNumber)

v8::Local<v8::Value> V8Proxy::instrumentFunctionCall(Page*, function, args, argv)
{
    InspectorInstrumentation::willCallFunction(page);
    function->Call();
    InspectorInstrumentation::didCallFunction(cookie);
}

> Source/WebCore/dom/ScriptedAnimationController.cpp:129
> +                InspectorInstrumentation::didFireAnimationFrameEvent(m_document);

You should use cookie here as well.

[Ilya Tikhonovsky]

Created attachment 107319 [details]
[patch] next iteration.

comments addressed

[Pavel Feldman]

Comment on attachment 107319 [details]
[patch] next iteration.

View in context: https://bugs.webkit.org/attachment.cgi?id=107319&action=review

Almost there.

> LayoutTests/inspector/timeline/timeline-animation-frame-expected.txt:6
> +    stackTrace : <object>

You need to generate custom expectations (these are chromium's)

> LayoutTests/inspector/timeline/timeline-animation-frame.html:26
> +        InspectorTest.evaluateInPage("performActions()", step2);

You could set a console sniffer and do console.log from animation frame callback instead.

> LayoutTests/inspector/timeline/timeline-animation-frame.html:39
> +        setTimeout(step2, 10);

Then you would not need this timeout that is going to be flaky.

> Source/WebCore/bindings/v8/V8Proxy.cpp:518
> +

Too

> Source/WebCore/bindings/v8/V8Proxy.cpp:520
> +

many

> Source/WebCore/bindings/v8/V8Proxy.cpp:522
> +

blank lines...

> Source/WebCore/bindings/v8/V8Proxy.h:-192
> -

No need to change this

> Source/WebCore/dom/ScriptedAnimationController.cpp:136
>  

No need to add line

> Source/WebCore/inspector/InspectorInstrumentation.h:503
>      FAST_RETURN_IF_NO_FRONTENDS(InspectorInstrumentationCookie());

if (!page)
    return;

Otherwise ASSERT(page) will trigger in instrumentingAgentsForPage.

[Ilya Tikhonovsky]

Created attachment 107324 [details]
[patch] next iteration.

(In reply to comment #12)
> (From update of attachment 107319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107319&action=review
> 
> Almost there.
> 
> > LayoutTests/inspector/timeline/timeline-animation-frame-expected.txt:6
> > +    stackTrace : <object>

fixed


> You need to generate custom expectations (these are chromium's)
> 
> > LayoutTests/inspector/timeline/timeline-animation-frame.html:26
> > +        InspectorTest.evaluateInPage("performActions()", step2);
> 
> You could set a console sniffer and do console.log from animation frame callback instead.

done

> > LayoutTests/inspector/timeline/timeline-animation-frame.html:39
> > +        setTimeout(step2, 10);
> 
> Then you would not need this timeout that is going to be flaky.

done
> 
> > Source/WebCore/bindings/v8/V8Proxy.cpp:518
> > +
> 
> Too
> 
> > Source/WebCore/bindings/v8/V8Proxy.cpp:520
> > +
> 
> many
> 
> > Source/WebCore/bindings/v8/V8Proxy.cpp:522
> > +
> 
> blank lines...

done
> 
> > Source/WebCore/bindings/v8/V8Proxy.h:-192
> > -
> 
> No need to change this
> 

done

> > Source/WebCore/dom/ScriptedAnimationController.cpp:136
> >  
> 
> No need to add line
> 
> > Source/WebCore/inspector/InspectorInstrumentation.h:503
> >      FAST_RETURN_IF_NO_FRONTENDS(InspectorInstrumentationCookie());
> 
> if (!page)
>     return;
> 
> Otherwise ASSERT(page) will trigger in instrumentingAgentsForPage.

We have this check exists in instrumentingAgentsForPage

[Ilya Tikhonovsky]

Created attachment 107325 [details]
[patch] next iteration. minor fixes.

[Pavel Feldman]

Comment on attachment 107325 [details]
[patch] next iteration. minor fixes.

View in context: https://bugs.webkit.org/attachment.cgi?id=107325&action=review

(make sure you add a page check).

> Source/WebCore/bindings/v8/V8Proxy.h:177
> +        // call the function with the given receiver and arguments and report times to DevTools.

_C_all the, to the "InspectorIntrumentation"

[Ilya Tikhonovsky]

Committed r95091: <http://trac.webkit.org/changeset/95091>

[Csaba Osztrogonác]

(In reply to comment #16)
> Committed r95091: <http://trac.webkit.org/changeset/95091>

It was rolled out: http://trac.webkit.org/changeset/95302, see https://bugs.webkit.org/show_bug.cgi?id=68232 for details.

[Ilya Tikhonovsky]

landed as r95404