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

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has asked Timothy Hatcher
<timothy at hatcher.name> 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 25871: ScriptCallStack and ScriptCallFrame v4
https://bugs.webkit.org/attachment.cgi?id=25871&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
(In reply to comment #10)

Thanks for reviewing!

> (From update of attachment 25846 [review])
> > +	 bindings/js/ScriptCalFrame.cpp \
> 
> Mispelling here.

Fixed.

> > +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 "(".

Fixed.

> > +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.

Fixed.

> > +	     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&.

Fixed.

> 
> > +	     const ScriptValue &argumentAt(unsigned) const;
> 
> Ditto.

Fixed.

> 
> > +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.

I went ahead and made all of my changes run over 80 cols instead of lining up
on next line.

> > +	 JSValue* func;
> 
> Maybe "function" instead of "func".

Renamed to "function".

> 
> > +	 exec->interpreter()->retrieveLastCaller(exec, signedLineNumber,
sourceID,
> > +						 urlString, func);
> 
> Another case if the "lining up a second line with parentheses".

Fixed.

> 
> > +	     unsigned lineNumber =  signedLineNumber >= 0 ? signedLineNumber :
0;
> 
> Extra space here after the = sign.

Fixed.

> 
> > +	     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.

Fixed.

> 
> > +	     // 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.

Fixed.

> > +	 JSValue* func = m_exec->interpreter()->retrieveCaller(m_exec,
m_caller);
> > +	 while(!func->isNull()) {
> 
> Space after while here before parenthesis here.

Fixed.

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

Fixed.

> > +	 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?

Removed ".get()" from all methods. You're right, there's an overload that takes
care of this.

As for the state, I would think that if someone writes "ProtectedPtr<JSValue>
emptyValue;", they don't necessarily mean that the held value is null or
undefined -- just that there isn't one being held by the wrapper. If we want to
change the meaning of this, we should do it by changing ProtectedPtr by always
populating with some default T constructor, which contains initial value.
Right? I am not really sure, just thinking outloud.

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

I am using both isNull() and isUndefined() in a composite statement that's the
equivalent of isNullOrUndefined(). I thought keeping them separate would be a
good thing, since one can check for either together or separately, but I am
open to just smushing them together into one method.

> 
> > -	 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?

The reason I use a pointer is because ScriptCallFrame vector is lazily
populated when first queried for stack trace, which contradicts the "constness"
qualifier ... Oops, I just realized I have a problem where the laziness is not
really lazy -- access to 0'th call frame should not trigger initialization.
Fixed that.

> > +	     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.

Fixed.

> 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.

Sounds good. Addressed by using ScriptString, which does the locking for you.

> > -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".
> 

Thanks again for reviewing! This really helps me get up to speed on conventions
and general WebKit way of life :)

Additionally, I started testing .trace behavior some more and realized that
with the introduction of a separate frames vector, the comparison of messages
needs a tweak. Added that in v4.

I've been thinking: it is kind of unfortunate that there's all this pouring of
data from one fairly similar object (ScriptCallStack) to another
(ConsoleMessage). Perhaps there needs to be some sort of safe, "frozen-in-time"
ScriptCallStack, which ConsoleMessage can hold and pass around.

What do you think?