[webkit-reviews] review denied: [Bug 30412] Web Inspector should get human readable DOM Exceptions : [Attachment 41251] DOM Exceptions on Page JavaScript content - not console

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 41251: DOM Exceptions on Page JavaScript content - not console
https://bugs.webkit.org/attachment.cgi?id=41251&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    if (DOMCoreException* domException = toDOMCoreException(exception))
> +	   errorMessage = domException->longDescription();
> +    else if (RangeException* rangeException = toRangeException(exception))
> +	   errorMessage = rangeException->longDescription();
> +    else if (EventException* eventException = toEventException(exception))
> +	   errorMessage = eventException->longDescription();
> +    else if (XMLHttpRequestException* xmlhttpException =
toXMLHttpRequestException(exception))
> +	   errorMessage = xmlhttpException->longDescription();
> +#if ENABLE(SVG)
> +    else if (SVGException* svgException = toSVGException(exception))
> +	   errorMessage = svgException->longDescription();
> +#endif
> +#if ENABLE(XPATH)
> +    else if (XPathException* pathException = toXPathException(exception))
> +	   errorMessage = pathException->longDescription();
> +#endif

I'd suggest putting all this logic into a toExceptionBase function that returns
an ExceptionBase* or 0. I will need this same list, but having a separate
function will make things a bit clearer if we have to do this elsewhere, and
also factor out the call to the longDescription function.

> +String ExceptionBase::longDescription() const
> +{
> +    return toString() + ": " + m_description;
> +}

I think it would be better to just have a function returning m_description. The
code to construct an error message by combining the "Error: " with the message
and then with ": " and then with the description could go closer to where it's
actually logged. I think having the format built into the exception object is
not really so good. We have to do that in the case of toString but in this case
we can, and should, factor it out.

What about test cases?

review- because of the lack of test cases for this new feature