[webkit-reviews] review denied: [Bug 134420] [ftlopt] DebuggerCallFrame::scope() should return a DebuggerScope : [Attachment 234038] the patch.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 134420: [ftlopt] DebuggerCallFrame::scope() should return a DebuggerScope
https://bugs.webkit.org/show_bug.cgi?id=134420

Attachment 234038: the patch.
https://bugs.webkit.org/attachment.cgi?id=234038&action=review

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


> Source/JavaScriptCore/debugger/Debugger.cpp:183
> +    globalObject->ensureDebuggerScopeStructureIsInitialized();

I don't think that saving 32 bytes is worth this complexity (unless you
measured otherwise).

> Source/JavaScriptCore/debugger/DebuggerCallFrame.h:88
> +    Strong<DebuggerScope> m_scope;

What prevents this from being a reference cycle? Any time you add a Strong<> as
a data member, you should include a comment explaining what prevents it from
being a reference cycle.

I suspect that this is a reference cycle. One way to solve that problem is to
use a Weak<> instead.

> Source/JavaScriptCore/debugger/DebuggerScope.cpp:51
> +    if (!scope) {

This isn't right. Every function has a scope, so a null check is not the right
way to find out if the function has allocated an activation object.

> Source/JavaScriptCore/debugger/DebuggerScope.cpp:138
> +void DebuggerScope::invalidateChain()

Why do we need to do this? I don't think we should do this.

> Source/JavaScriptCore/debugger/DebuggerScope.cpp:171
> +JSObject* DebuggerScope::objectAtScope() const
> +{
> +    ASSERT(isValid());
> +    return JSScope::objectAtScope(m_scope.get());
>  }

I think this helper function just obfuscates things. It would be better to
write this one line of code at the call site.

> Source/JavaScriptCore/debugger/DebuggerScope.h:95
> +    JSScope* toJSScope() { return m_scope.get(); }

We use the "to" prefix to indicate a conversion -- usually, a type case. In
this case, you're just returning a data member, so you should use normal
accessor naming: The data member should be named "m_jsScope" and the accessor
should be named "jsScope()".