[webkit-reviews] review denied: [Bug 174738] Web Inspector: Refactoring: remove async stack trace logic from InspectorInstrumentation : [Attachment 316157] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 174738: Web Inspector: Refactoring: remove async stack trace logic from
InspectorInstrumentation
https://bugs.webkit.org/show_bug.cgi?id=174738

Attachment 316157: Patch

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




--- Comment #2 from Brian Burg <bburg at apple.com> ---
Comment on attachment 316157
  --> https://bugs.webkit.org/attachment.cgi?id=316157
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.