[webkit-reviews] review denied: [Bug 21918] A JavaScript Exception abstraction : [Attachment 24718] Removed ExceptionCatcher, added inlines

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 21918: A JavaScript Exception abstraction
https://bugs.webkit.org/show_bug.cgi?id=21918

Attachment 24718: Removed ExceptionCatcher, added inlines
https://bugs.webkit.org/attachment.cgi?id=24718&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
> +
> +#include <wtf/Noncopyable.h>
> +#include "ScriptController.h"
These should be sorted.

> +
> +typedef JSC::JSValue* JSException;
No JSException is being defined for V8 here.  I don't really see the point of
this typedef since JSException is never used.

> +#if USE(V8)
> +class ExceptionCatcher;
What does an ExceptionCatcher do?  Why is necessary to store both it and the
exception.

> +    ExceptionContext(Node*);
> +#if USE(V8)
> +    ExceptionContext();
How is this empty constructor used.  Perhaps a link to the corresponding
Chromium code would help.

> +    bool hadException();
> +    JSException exception() const;
This method seems to be unused.
> +
> +    // Returns a non-exception code object.
> +    static JSException noException();
As does this one.

> +
> +private:
> +#if USE(V8)
> +    friend class ExceptionCatcher;
> +    void setException(JSException exception) { m_exception = exception; }
> +    void setExceptionCatcher(ExceptionCatcher*);
> +    JSException m_exception;
> +    ExceptionCatcher* m_exceptionCatcher;
> +#elif USE(JSC)
> +    JSC::ExecState* m_exec;
> +#endif

This is a lot of engine specific code.	Can this be abstracted better?

It also seems like this is something that should work with other bindings as
well.  So that a Objective-C NodeFilter could store a thrown exception as well.


r-.