[webkit-reviews] review denied: [Bug 63688] Refactoring: ExceptionCode value which never become non-zero should be replaced with NeverThrown : [Attachment 99455] Fixing release builds

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 63688: Refactoring: ExceptionCode value which never become non-zero should
be replaced with NeverThrown
https://bugs.webkit.org/show_bug.cgi?id=63688

Attachment 99455: Fixing release builds
https://bugs.webkit.org/attachment.cgi?id=99455&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=99455&action=review

This patch changes far too much at once. We need to be careful about where we
choose to ignore exception codes and where we choose to check them. Almost all
the examples here that are ignoring exceptions should instead be asserting them
by default.

Lets do a smaller version of this patch that tackles a much smaller set of
functions at first. I know you held back to only functions in the dom
directory, but I think this needs more thought for each function.

Specifically, adding a checker to assert there is no exception by default
should be the first thing we consider for any given function. Ignoring the
exception instead should be done only rarely when that is helpful for a reason
specific to that function.

> Source/WebCore/dom/CharacterData.h:33
> -    void setData(const String&, ExceptionCode&);
> +    void setData(const String&, ExceptionCode& = IgnoringExceptionCode());

This should not ignore the exception code by default. The only way we’d get an
exception would be if the node was read-only. At the moment that isn’t even
implemented!

Generally speaking the default should be to assert the exception code, not
ignore it, and this is a perfect example.

> Source/WebCore/dom/CharacterData.h:35
> +    String substringData(unsigned offset, unsigned count, ExceptionCode& =
IgnoringExceptionCode());

Since the behavior here is to return the null string if the offset is past the
end of the data, I suppose we could default to ignoring the exception. But I
would prefer that the default be to check the exception, and the call sites
that need to be tolerant can explicitly ask to ignore.

> Source/WebCore/dom/CharacterData.h:36
> +    void appendData(const String&, ExceptionCode& =
IgnoringExceptionCode());

Since this will just throw away the new data if the node is read-only, this
should assert the exception code, not ignore it.

> Source/WebCore/dom/CharacterData.h:70
> +    void checkCharDataOperation(unsigned offset, ExceptionCode& =
IgnoringExceptionCode());

The only reason to call this function is to set the exception code. This should
not have a default argument for the ExceptionCode&.

> Source/WebCore/dom/IgnoringExceptionCode.h:39
> +class IgnoringExceptionCode {

The grammar here is not right. Class names should be nouns. Given this name, an
object of this class would be an "ignoring exception code"? That’s not good
terminology.

I think IgnoredExceptionCode would be an OK name, although it’s a pretty long
name. A possibly-better name is ExceptionCodeIgnorer or ExceptionIgnorer.

> Source/WebCore/dom/IgnoringExceptionCode.h:51
> +#if !defined(ASSERT_DISABLED)
> +    : m_code(0)
> +#endif

If we are ignoring the exception code then we don’t want to initialize m_code
at all. The only reason we have this here is that this class is not really the
“ignored exception code” class, since we plan to derive from it and not ignore
the exception code!

There’s no reason t have this unclear design. I’d rather see repeated code.

> Source/WebCore/dom/IgnoringExceptionCode.h:76
> +class NeverThrownWithLocation : public IgnoringExceptionCode {
> +public:
> +    NeverThrownWithLocation(const char*, size_t, const char*);
> +    ~NeverThrownWithLocation();
> +
> +private:
> +    const char* m_file;
> +    size_t m_line;
> +    const char* m_function;
> +};
> +
> +inline NeverThrownWithLocation::NeverThrownWithLocation(const char* file,
size_t line, const char* function)
> +    : m_file(file), m_line(line), m_function(function)
> +{
> +}
> +
> +inline NeverThrownWithLocation::~NeverThrownWithLocation()
> +{
> +    ASSERT_AT(!m_code, m_file, m_line, m_function);
> +}
> +
> +}

This entire class should be inside an #if !ASSERT_DISABLED since we don’t want
to use it at all in that case.

This is a misuse of inheritance. A derived class should not contradict
something from a base class. The classic example is that ellipse should not
inherit from circle, because an ellipse is not a circle. So having a class that
asserts an exception code is zero be a subclass of something called “ignored
exception code” is not good design.

A class name should be a noun phrase describing the object. The name "never
thrown with location" is not good because it’s not sensible to say “this object
is a never thrown with location”.

I would call the class something like ExceptionCodeAssertionChecker. Or
NoExceptionAssertionChecker.

> Source/WebCore/dom/IgnoringExceptionCode.h:82
> +#if ASSERT_DISABLED
> +#define ASSERT_NEVER_THROWN WebCore::IgnoringExceptionCode()
> +#else
> +#define ASSERT_NEVER_THROWN WebCore::NeverThrownWithLocation(__FILE__,
__LINE__, WTF_PRETTY_FUNCTION)
> +#endif

I am not fond of “never thrown” as a way to say there is no exception. I would
prefer ASSERT_NO_EXCEPTION.

It’s neat that we can tell you exactly where the exception occurred if you use
the macro at the call site, but generally I don’t think callers should be
thinking about this at all. I think that most of the DOM functions should
default to asserting there is no exception. There are very few DOM functions
where we’d want to call them internally and actually ignore the exception.