Bug 192669 - Web Inspector: Timelines: correctly label Intersection Observer callbacks
Summary: Web Inspector: Timelines: correctly label Intersection Observer callbacks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-13 11:01 PST by Simon Fraser (smfr)
Modified: 2018-12-19 15:18 PST (History)
15 users (show)

See Also:


Attachments
Patch (23.63 KB, patch)
2018-12-18 17:17 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2018-12-13 11:01:51 PST
Intersection Observer callbacks currently show as "Script evaluated", which is misleading (no eval is involved). They need to be correctly labeled as "Intersection Observer callbacks" or something.
Comment 1 Radar WebKit Bug Importer 2018-12-13 11:03:24 PST
<rdar://problem/46702490>
Comment 2 Joseph Pecoraro 2018-12-13 11:14:38 PST
Extra data, such as a name, could probably be provided at these points:

    bindings/js/JSCallbackData.cpp
    78:    InspectorInstrumentationCookie cookie = JSExecState::instrumentFunctionCall(context, callType, callData);

    bindings/js/JSEventListener.cpp
    171:    InspectorInstrumentationCookie cookie = JSExecState::instrumentFunctionCall(&scriptExecutionContext, callType, callData);

    bindings/js/JSErrorHandler.cpp
    102:        InspectorInstrumentationCookie cookie = JSExecState::instrumentFunctionCall(&scriptExecutionContext, callType, callData);

    bindings/js/ScheduledAction.cpp
    115:    InspectorInstrumentationCookie cookie = JSExecState::instrumentFunctionCall(&context, callType, callData);
Comment 3 Simon Fraser (smfr) 2018-12-18 17:17:10 PST
Created attachment 357638 [details]
Patch
Comment 4 Build Bot 2018-12-18 17:19:00 PST
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 5 Joseph Pecoraro 2018-12-18 17:29:15 PST
Comment on attachment 357638 [details]
Patch

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

r=me

We should consider writing a test for this in:

    LayoutTests/inspector/timeline

But it looks like we don't have any tests for generic timeline data. Feel free to file a bug on me to add inspector tests, and attach any test page content you have for observers. Or if you want I can guide you through a test that might not be too difficult.

> Source/WebCore/dom/MutationObserver.cpp:253
> +        InspectorInstrumentationCookie cookie = InspectorInstrumentation::willFireObserverCallback(*context, "Mutation Observer"_s);

Should we drop the space? The name of the class is `MutationObserver`.

I think that would read fine without having to localize it to something like "Foo Observer".

> Source/WebCore/inspector/InspectorInstrumentation.h:1433
> +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForContext(context))

Please insert a fast return to do as little work as possible if no inspector is open:

    FAST_RETURN_IF_NO_FRONTENDS(InspectorInstrumentationCookie());

> Source/WebCore/page/IntersectionObserver.cpp:264
> +    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willFireObserverCallback(*context, "Intersection Observer"_s);

Ditto regarding space.

> Source/WebCore/page/PerformanceObserver.cpp:111
> +    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willFireObserverCallback(*context, "Performance Observer"_s);

Ditto regarding space.
Comment 6 Simon Fraser (smfr) 2018-12-19 15:18:35 PST
https://trac.webkit.org/changeset/239397/webkit