[webkit-reviews] review granted: [Bug 30412] Web Inspector should get human readable DOM Exceptions : [Attachment 41332] Addresses Sam's Comment
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 16 17:54:53 PDT 2009
Darin Adler <darin at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 30412: Web Inspector should get human readable DOM Exceptions
https://bugs.webkit.org/show_bug.cgi?id=30412
Attachment 41332: Addresses Sam's Comment
https://bugs.webkit.org/attachment.cgi?id=41332&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
I was going to suggest putting that function inside JSDOMBinding.cpp rather
than suggesting creating a new file. A new file is OK though.
New source files contributed by Apple engineers normally should have the
BSD-style license, not LGPL.
> + if (DOMCoreException* domException = toDOMCoreException(value))
> + return reinterpret_cast<ExceptionBase*>(domException);
> + else if (RangeException* rangeException = toRangeException(value))
> + return reinterpret_cast<ExceptionBase*>(rangeException);
> + else if (EventException* eventException = toEventException(value))
> + return reinterpret_cast<ExceptionBase*>(eventException);
> + else if (XMLHttpRequestException* xmlHttpException =
toXMLHttpRequestException(value))
> + return reinterpret_cast<ExceptionBase*>(xmlHttpException);
> +#if ENABLE(SVG)
> + else if (SVGException* svgException = toSVGException(value))
> + return reinterpret_cast<ExceptionBase*>(svgException);
> +#endif
> +#if ENABLE(XPATH)
> + else if (XPathException* pathException = toXPathException(value))
> + return reinterpret_cast<ExceptionBase*>(pathException);
> +#endif
We don't use else after return. So you can remove all those else.
> +#ifndef JSExceptionBase_h
> +#define JSExceptionBase_h
> +
> +#include "ExceptionBase.h"
> +
> +namespace WebCore {
> +
> +ExceptionBase* toExceptionBase(JSC::JSValue);
> +
> +} // namespace WebCore
> +
> +#endif // JSExceptionBase_h
This header only needs a forward-declaration of the classes ExceptionBase and
JSC::JSValue, it doesn't actually have to include ExceptionBase.h.
> + "index or size was negative, or greater than the allowed value",
These descriptions look like sentences, so they should have a capital letter at
the start and a period at the end.
> + Adds tests for output of uncaught exceptions to the Web Inspector
> + Console. Tests DOM exceptions 1, 3, and 8 (INDEX_SIZE_ERR,
NOT_FOUND_ERR, and
> + HIERARCHY_REQUEST_ERR).
Why not test them all? Is there any way to test more than one exception in the
same test?
review+, but please consider the comments above
More information about the webkit-reviews
mailing list