Bug 174738 – Web Inspector: Refactoring: extract async stack trace logic from InspectorInstrumentation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Notifying the debugger agent of events in the lifecycle of an asynchronous operation should be done by the relevant agent. Currently this is the job of InspectorInstrumenation, which handles timers and rAF. When support is added for XHR.send, postMessage, addEventListener, etc, the layering violation will get worse.

Move the AsynchronousCallType enum to its own file, and other code specific to async stacks to the relevant agent.

[Matt Baker]

Created attachment 316157 [details]
Patch

[Blaze Burg]

Comment on attachment 316157 [details]
Patch

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

r- unless everyone else disagrees on the enum vs int signature issue. I wasn't paying attention when this code originally landed.

> Source/WebCore/inspector/InspectorAsynchronousCallType.h:28
> +namespace WebCore {

Please, use namespace Inspector unless it's really not possible.

> Source/WebCore/inspector/InspectorAsynchronousCallType.h:31
> +    AnimationFrame = 1,

Nit: RequestAnimationFrame

> Source/WebCore/inspector/InspectorAsynchronousCallType.h:32
> +    Timer,

Nit: can we call this DOMTimer to be consistent with the relevant C++ class?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:349
> +        debuggerAgent->didCancelAsyncCall(static_cast<int>(AsynchronousCallType::Timer), timerId);

Nooo stop it :(

EDIT: I see that we did this to avoid mentioning Web-specific concepts in JSC / Debugger.json. I have no such aversion. This code should be in Inspector namespace anyway. Passing around ints is worse than some optional enum values having no meaning in JavaScriptCore proper.

> Source/WebCore/inspector/PageDebuggerAgent.cpp:160
> +    JSC::ExecState* scriptState = document->execState();

The dereference of document needs a null check, or passed to here by reference.

[Joseph Pecoraro]

Comment on attachment 316157 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Web Inspector: Refactoring: remove async stack trace logic from InspectorInstrumentation

I'd prefer "extract" over "remove".

[Matt Baker]

Created attachment 316338 [details]
Patch

[Blaze Burg]

Comment on attachment 316338 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        Move AsyncCallType enum to InspectoprDebuggerAgent, which manages async

Nit: InspectorDebuggerAgent

[Matt Baker]

Created attachment 316389 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316389 [details]
Patch

Clearing flags on attachment: 316389

Committed r219889: <http://trac.webkit.org/changeset/219889>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.