[webkit-reviews] review denied: [Bug 119548] Refactoring of throw exception. : [Attachment 208271] patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Chris Curtis
<chris_curtis at apple.com>'s request for review:
Bug 119548: Refactoring of throw exception.
https://bugs.webkit.org/show_bug.cgi?id=119548

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

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


> Source/JavaScriptCore/ChangeLog:44
> +	   (JSC::Interpreter::addStackTraceIfNecessary): Removed this because
it is never necessary now.

Did you remove this function? It seems to be present still.

What about appendSourceToError and addErrorInfo? Can those move into
VM::throwError?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:590
> +NEVER_INLINE HandlerInfo* Interpreter::unwindException(CallFrame*&
callFrame, JSValue& exceptionValue, unsigned bytecodeOffset)

Let's call this "unwind". We're unwinding the stack, not the exception, so
"unwind exception" is an awkward phrase.

> Source/JavaScriptCore/runtime/VM.h:341
>	   JS_EXPORT_PRIVATE void clearExceptionStack();

Can we remove this function? Clearing the exception stack should be tied to
clearing the exception. We want exception and exceptionStack to be tied
together.

> Source/JavaScriptCore/runtime/VM.h:346
> +	   JS_EXPORT_PRIVATE JSValue throwError(ExecState*, JSValue);
> +	   JS_EXPORT_PRIVATE JSObject* throwError(ExecState*, JSObject*);

Let's call these "throwException", to be consistent in our terminology.

> Source/JavaScriptCore/runtime/VM.h:351
> +	   JSValue exception;

Let's make this value private, and add an exception() accessor.

JIT code that used to use OBJECT_OFFSETOF(struct JITStackFrame, vm) can use a
member function instead. See JSCell::structureOffset() for an example.

> Source/WebCore/bindings/js/ScriptFunctionCall.cpp:-128
>      JSLockHolder lock(m_exec);
> -
>      JSValue function = thisObject->get(m_exec, Identifier(m_exec, m_name));
>      if (m_exec->hadException()) {
>	   if (reportExceptions)
>	       reportException(m_exec, m_exec->exception());
> -

Please revert these whitespace changes to maintain a clean svn history.

> LayoutTests/ChangeLog:13
> +	   These tests have only the column number changed.
> +	   * fast/events/window-onerror5-expected.txt:
> +	   * http/tests/workers/worker-importScriptsOnError-expected.txt:

Is the new column number better? If so, why?

> LayoutTests/ChangeLog:72
> -2013-08-06  Filip Pizlo  <fpizlo at apple.com>
> +013-08-06  Filip Pizlo  <fpizlo at apple.com>

Please revert.