Bug 67986

Created attachment 107155 [details] [patch] initial version

Created attachment 107156 [details] screenshot

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

Created attachment 107164 [details] screenshot

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

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

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

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

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

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

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.

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

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

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"

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

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

landed as r95404