[webkit-reviews] review denied: [Bug 118328] Optimize addStrackTraceIfNecessary to be faster in the case when it's not necessary : [Attachment 206285] Corrected and added info to the change log.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied  review:
Bug 118328: Optimize addStrackTraceIfNecessary to be faster in the case when
it's not necessary
https://bugs.webkit.org/show_bug.cgi?id=118328

Attachment 206285: Corrected and added info to the change log.
https://bugs.webkit.org/attachment.cgi?id=206285&action=review

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


> Source/JavaScriptCore/ChangeLog:15
> +	   Changes to decode and eval come from
LayoutTests/inspector/console/console-exception-stack-traces.html.

A little clearer to say "are tested by" instead of "come from".

> Source/JavaScriptCore/ChangeLog:23
> +	   (JSC::Interpreter::eval): The current frame was the errored frame.
This is popping

"errored frame" is a misleading phrase. The top frame is the frame we allocated
for running eval code. There's nothing wrong with it. 

We need to roll back .topCallFrame because we set it before we fully committed
to running the eval code, and now, late in the game, we've discovered that we
aren't going to run the eval code. It would probably also work to delay setting
.topCallFrame until we knew for sure we were going to run the code. (I'm not
asking you to do this necessarily, only trying to illustrate the logic here.)

Please try to clarify this comment.

> Source/JavaScriptCore/ChangeLog:28
> +	   * runtime/JSGlobalObjectFunctions.cpp:
> +	   (JSC::runtime::decode): See above ^^

I'm still not clear about why you think an exception thrown from decodeURI()
should behave the same way as a syntax error when trying to run eval code.

If f() calls decodeURI(), and decodeURI() throws a URI error, shouldn't the
backtrace show f()->decodeURI()? In general, if f() calls any host function,
which throws an error, shouldn't the backtrace always be f()->host()?
Otherwise, how do I know that host() is the function that threw the error?

eval() is special because it's a keyword that invokes a program "as if" it were
a part of the text of the calling function. decodeURI() is not special in this
way.