[webkit-reviews] review denied: [Bug 22631] Streamline Console.cpp, abstract out the use of JSC::ExecState and JSC::ArgList by introducing ScriptCallFrame and ScriptStackTrace abstractions : [Attachment 25846] ScriptCallStack and ScriptCallFrame v3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 22631: Streamline Console.cpp, abstract out the use of JSC::ExecState and
JSC::ArgList by introducing ScriptCallFrame and ScriptStackTrace abstractions
https://bugs.webkit.org/show_bug.cgi?id=22631

Attachment 25846: ScriptCallStack and ScriptCallFrame v3
https://bugs.webkit.org/attachment.cgi?id=25846&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    bindings/js/ScriptCalFrame.cpp \

Mispelling here.

> +ScriptCallFrame::ScriptCallFrame(const JSC::UString& functionName,
> +				    const JSC::UString& urlString,
> +				    int lineNumber,
> +				    JSC::ExecState* exec,
> +				    const JSC::ArgList& args,
> +				    unsigned skipArgumentCount)

You should not need all those JSC prefixes here.

> +    for(unsigned i = skipArgumentCount; i < args.size(); ++i)

We normally call size() outside the loop. We normally use a space between "for"
and "(".

> +const ScriptValue &ScriptCallFrame::argumentAt(unsigned index) const

We put the & next to the type name. ScriptValue&.

> +#include "runtime/ArgList.h"

This include should have angle brackets, I believe. We may also need a new
forwarding header.

> +	   const ScriptString &functionName() const { return m_functionName; }
> +	   const KURL &sourceURL() const { return m_sourceURL; }

We put the & next to the type name. ScriptString& and KURL&.

> +	   const ScriptValue &argumentAt(unsigned) const;

Ditto.

> +ScriptCallStack::ScriptCallStack(ExecState* exec, const ArgList& args,
> +				      unsigned skipArgumentCount)

Strange indentation here. We normally try to avoid lining up second lines with
parentheses for just this reason. Things don't line up any more if you rename.
Instead we put things on one line or indent the second line by some fixed
amount instead of lining up.

> +    JSValue* func;

Maybe "function" instead of "func".

> +    exec->interpreter()->retrieveLastCaller(exec, signedLineNumber,
sourceID,
> +					       urlString, func);

Another case if the "lining up a second line with parentheses".

> +	   unsigned lineNumber =  signedLineNumber >= 0 ? signedLineNumber : 0;


Extra space here after the = sign.

> +	   m_frames.append(
> +	       ScriptCallFrame(m_caller->name(&m_exec->globalData()),
urlString,
> +			       lineNumber, exec, args, skipArgumentCount));

Not a big fan of the line break before the ScriptCallFrame. I think that trying
to line up subsequent lines with the open parenthesis leads to that decision
because you want to preserve space for the second line. If the second line is
just indented by 4 spaces I think that works better.

> +	   // caller is unknown, but we should still add the frame, because
> +	   // something called us, and gave us arguments

We almost always use sentence capitalization and periods for comments like this
one.

> +    JSValue* func = m_exec->interpreter()->retrieveCaller(m_exec, m_caller);

> +    while(!func->isNull()) {

Space after while here before parenthesis here.

> +	   InternalFunction* internalFunction = asInternalFunction(func);
> +	   ArgList emptyArgList;
> +	   m_frames.append(
> +	       ScriptCallFrame(
> +		   internalFunction->name(&m_exec->globalData()), UString(), 0,
0,
> +		   emptyArgList, 0));

Same comments about indentation.

> +    if (!m_value.get())
> +	   return false;

This get() here should not be necessary. I'm a little concerned that when
m_value is 0 we return false from both isNull() and isUndefined(). What is the
name for that state when m_value is 0? Why do we allow that state at all?

I notice that you're not using isUndefined() yet. We may find later that
isNullOrUndefined() is more common than isUndefined().

> -    ConsoleMessage(MessageSource s, MessageLevel l, ExecState* exec, const
ArgList& args, unsigned li, const String& u, unsigned g)
> +    ConsoleMessage(MessageSource s, MessageLevel l, ScriptCallStack*
callStack, unsigned g, bool storeTrace = false)

Maybe const ScriptCallStack& would be a better type than ScriptCallStack*. Is
there a reason to prefer a pointer here?

> +	   if (storeTrace)
> +	       for(unsigned i = 0; i < callStack->size(); ++i)
> +		   frames[i] = callStack->at(i).functionName();

Our coding style puts braces around a multi-line if statement like this one.
Also a space after the for before the parenthesis.

I believe that assigning to a UString requires holding the JSLock, so this new
code should be added after the JSLock line. But maybe my information about that
is out of date. It could date back to an earlier stage of our threading model.

> -void InspectorController::addMessageToConsole(MessageSource source,
MessageLevel level, ExecState* exec, const ArgList& arguments, unsigned
lineNumber, const String& sourceURL)
> +void InspectorController::addMessageToConsole(MessageSource source,
MessageLevel level, ScriptCallStack* callStack)

Again, I think a const ScriptCallStack& would be better here.

> -void InspectorController::startGroup(MessageSource source, ExecState* exec,
const ArgList& arguments, unsigned lineNumber, const String& sourceURL)
> +void InspectorController::startGroup(MessageSource source, ScriptCallStack*
callStack)

Ditto.

>  #include "PlatformString.h"
> +#if USE(JSC)
>  #include <profiler/Profile.h>
> +#endif
>  #include <wtf/RefCounted.h>
>  #include <wtf/PassRefPtr.h>

Includes inside an #if should be in a separate paragraph.

My review comments are mostly minor things, but there are enough that I think
I'll say review- for now. Seems like you're on the right track, though. Looking
forward to the next patch. Tim, sorry I "grabbed this one out from under you".