[webkit-reviews] review granted: [Bug 121796] Move breakpoint (and exception break) functionality into JSC::Debugger : [Attachment 216356] patch 3: fixed some issue revealed by a Windows build.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 121796: Move breakpoint (and exception break) functionality into
JSC::Debugger
https://bugs.webkit.org/show_bug.cgi?id=121796

Attachment 216356: patch 3: fixed some issue revealed by a Windows build.
https://bugs.webkit.org/attachment.cgi?id=216356&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=216356&action=review


r=me

This is a reasonable first step. I'm not sure the division of responsibilities
is perfect yet. It seems odd for a JSC-level thing to be in charge of defining
ReasonForPause, and implementing all the reasons you might pause. That
constrains the design of the debugger UI to the set of reasons the VM builds
in. A more flexible design would just build into JSC the ability to pause --
and the debugger UI layer could invent as many reasons as it liked.

> Source/JavaScriptCore/debugger/DebuggerPrimitives.h:37
> +typedef intptr_t SourceID;
> +static const SourceID noSourceID = 0;
> +
> +typedef int32_t BreakpointID;
> +static const BreakpointID noBreakpointID = 0;

Why intptr_t for one and int32_t for another? Seems like you actually want
size_t for both.