[webkit-reviews] review granted: [Bug 192669] Web Inspector: Timelines: correctly label Intersection Observer callbacks : [Attachment 357638] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 192669: Web Inspector: Timelines: correctly label Intersection Observer
callbacks
https://bugs.webkit.org/show_bug.cgi?id=192669

Attachment 357638: Patch

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




--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 357638
  --> https://bugs.webkit.org/attachment.cgi?id=357638
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.