Bug 67986 - Web Inspector: requestAnimationFrame callbacks don't show up in the timeline panel
Summary: Web Inspector: requestAnimationFrame callbacks don't show up in the timeline ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL:
Keywords:
Depends on: 68232
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-12 22:33 PDT by James Robinson
Modified: 2012-01-10 03:37 PST (History)
13 users (show)

See Also:


Attachments
[patch] initial version (23.75 KB, patch)
2011-09-13 02:26 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
screenshot (138.94 KB, image/png)
2011-09-13 02:30 PDT, Ilya Tikhonovsky
no flags Details
[patch] second iteration. (25.39 KB, patch)
2011-09-13 04:27 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
screenshot (118.88 KB, image/png)
2011-09-13 04:28 PDT, Ilya Tikhonovsky
no flags Details
[patch] second iteration. Fire Animation Callback was renamed to Animation Frame Event in Timeline UI (25.29 KB, patch)
2011-09-13 05:04 PDT, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
[patch] third version. comments addressed. (32.73 KB, patch)
2011-09-13 09:46 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] third version. with fixed style error (32.72 KB, patch)
2011-09-13 09:48 PDT, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
[patch] next iteration. (33.13 KB, patch)
2011-09-14 04:35 PDT, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
[patch] next iteration. (34.77 KB, patch)
2011-09-14 06:53 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] next iteration. minor fixes. (33.64 KB, patch)
2011-09-14 06:58 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-09-12 22:33:43 PDT
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.
Comment 1 Ilya Tikhonovsky 2011-09-13 02:26:45 PDT
Created attachment 107155 [details]
[patch] initial version
Comment 2 Ilya Tikhonovsky 2011-09-13 02:30:07 PDT
Created attachment 107156 [details]
screenshot
Comment 3 Ilya Tikhonovsky 2011-09-13 04:27:57 PDT
Created attachment 107163 [details]
[patch] second iteration.
Comment 4 Ilya Tikhonovsky 2011-09-13 04:28:32 PDT
Created attachment 107164 [details]
screenshot
Comment 5 Ilya Tikhonovsky 2011-09-13 05:04:28 PDT
Created attachment 107165 [details]
[patch] second iteration. Fire Animation Callback was renamed to Animation Frame Event in Timeline UI
Comment 6 Pavel Feldman 2011-09-13 06:07:20 PDT
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).
Comment 7 Ilya Tikhonovsky 2011-09-13 09:46:11 PDT
Created attachment 107185 [details]
[patch] third version. comments addressed.
Comment 8 Ilya Tikhonovsky 2011-09-13 09:48:35 PDT
Created attachment 107187 [details]
[patch] third version. with fixed style error
Comment 9 WebKit Review Bot 2011-09-13 09:50:12 PDT
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.
Comment 10 Pavel Feldman 2011-09-14 01:41:07 PDT
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.
Comment 11 Ilya Tikhonovsky 2011-09-14 04:35:31 PDT
Created attachment 107319 [details]
[patch] next iteration.

comments addressed
Comment 12 Pavel Feldman 2011-09-14 04:51:02 PDT
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.
Comment 13 Ilya Tikhonovsky 2011-09-14 06:53:26 PDT
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
Comment 14 Ilya Tikhonovsky 2011-09-14 06:58:40 PDT
Created attachment 107325 [details]
[patch] next iteration. minor fixes.
Comment 15 Pavel Feldman 2011-09-14 07:20:28 PDT
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"
Comment 16 Ilya Tikhonovsky 2011-09-14 07:33:45 PDT
Committed r95091: <http://trac.webkit.org/changeset/95091>
Comment 17 Csaba Osztrogonác 2011-09-16 10:28:03 PDT
(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.
Comment 18 Ilya Tikhonovsky 2012-01-10 03:37:17 PST
landed as r95404