[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