[webkit-reviews] review granted: [Bug 174757] Drop ExceptionCodeDescription class : [Attachment 316219] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 174757: Drop ExceptionCodeDescription class
https://bugs.webkit.org/show_bug.cgi?id=174757

Attachment 316219: Patch

https://bugs.webkit.org/attachment.cgi?id=316219&action=review




--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 316219
  --> https://bugs.webkit.org/attachment.cgi?id=316219
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316219&action=review

Looks fantastic. I have comments but none are critical enough that they must be
addressed before checking in.

> Source/WebCore/dom/DOMException.cpp:75
> +    static const Description emptyDescription { nullptr, nullptr, 0 };

I would define this right on the line before it is used instead of at the top
of the function.

> Source/WebCore/dom/DOMException.cpp:77
> +    size_t index = ec - INDEX_SIZE_ERR;

Not new, but peculiar to hardcode INDEX_SIZE_ERR here to make the table above
one line shorter. Nothing makes it clear that INDEX_SIZE_ERR is subtracted
because it happens to be the first exception. Maybe we can add a line for "0"
above and dump that or maybe just say "-1" here or maybe OK to just leave this
alone.

> Source/WebCore/dom/DOMException.cpp:79
> +    if (index < WTF_ARRAY_LENGTH(descriptions))
> +	   return descriptions[index];

Not new, but nothing guarantees that the descriptions array is kept matching
the ExceptionCode enum, which is unfortunate. Would be nice to find some easy
way to guarantee they are kept in sync.

> Source/WebCore/dom/DOMException.cpp:80
> +    return emptyDescription;

It would be a programming error to call this function with StackOverflowError,
ExistingException, TypeError, or RangeError. Maybe some day we can add an
assertion that those never get passed in here. On the other hand, the same is
probably true of all other values with nullptr for name, so maybe asserting
here before returning empty description is the thing to do some day.

> Source/WebCore/dom/DOMException.cpp:83
>  static ExceptionCode errorCodeFromName(const String& name)

This returns the unsigned short which is the legacy integer code, it does not
return our internal ExceptionCode value, which is something different. So we
should not use the type name ExceptionCode here. See comment below with further
thoughts about this:

> Source/WebCore/dom/DOMException.cpp:88
> +    for (auto& description : descriptions) {
> +	   if (description.name == name)
> +	       return description.code;
>      }

Seems funny that we are willing to do a linear search for this. Would be worth
cleaning up later; maybe there is no reason we need to support passing in an
exception name as a String; worth checking out who is using this and why.

> Source/WebCore/dom/DOMException.cpp:103
> +DOMException::DOMException(unsigned short ec, const String& name, const
String& message)

The argument name should be "code" or "legacy code", not "ec". Our traditional
use of "ec" is for our internal exception codes, and this is something subtly
different, the legacy integer code that is only for certain errors. For
example, this can never be TypeError or DataError, although both of those are
valid for ExceptionCode. So it would be good to not use ec here.

> Source/WebCore/dom/DOMException.cpp:104
>      : m_code(ec)

We should consider renaming m_code to m_legacyCode.

> Source/WebCore/dom/DOMException.cpp:110
> +const char* DOMException::name(ExceptionCode ec)

I think this should be inline in the header.

> Source/WebCore/dom/DOMException.cpp:115
> +const char* DOMException::message(ExceptionCode ec)

Ditto.

> Source/WebCore/dom/DOMException.h:31
> +#include "ExceptionCode.h"

Why is this include needed?

> Source/WebCore/dom/DOMException.h:38
> +    static Ref<DOMException> create(ExceptionCode, const String* message =
nullptr);

Could we use overloading for the version without a string instead of using
String*? I guess really I should ask that question about the createDOMException
function in JSDOMExceptionHandling instead of asking here. I think the use of
the pointer to make the argument optional is not great.

> Source/WebCore/dom/DOMException.h:41
> +    // For DOM bindings.
> +    static Ref<DOMException> create(const String& message, const String&
name);

Do DOM bindings really need this? Might be worth removing at some point. I
guess I already said that in a comment above.

> Source/WebCore/dom/DOMException.h:43
> +    unsigned short code() const { return m_code; }

I suggest using a named type for this. After reading the current draft of the
WebIDL specification maybe it should be named DOMException::LegacyCode?

    using LegacyCode = uint8_t;

If we are going to use something smaller than int for this, I suggest "uint8_t"
rather than "unsigned short" since we are using it to hold values in the range
0-25. It’s also a little annoying that "no legacy code" is represented with by
"0" but I guess we can live with that.

Kind of sad that we have the ExceptionCode concept in our DOM implementation
and the separate but similar "legacy integer code for a DOM exception", which
is a concept in the WebIDL itself but only for certain historical DOM errors. I
think we should try to keep these concepts separate now if we make them at all
different. It would be good to not have the ExceptionCode enum values be web
exposed at all; just an internal implementation detail. Maybe some day we can
even move ExceptionCode into Exception and have it be Exception::Code.

While researching this I noticed that <WebCore/ExceptionCode.h> has some
comments that are now not accurate. For example "we [...] use different
numerical ranges for different types of DOM exception" is no longer correct.
Right now there is nothing that depends on the values in ExceptionCode matching
the web-exposed values in DOMException, so I think it’s confusing that we try
to keep them matching. And I would love to change ExceptionCode into almost any
kind of more normal enumeration instead of a typedef paired with enum-defined
constants. It’s long term role is to just help us when we construct
WebCore::Exception objects and also be used here, but I think that’s it.

> Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMAttr.cpp:69
>      if (result.hasException()) {
> -	   WebCore::ExceptionCodeDescription
description(result.releaseException().code());
> +	   auto description =
WebCore::DOMException::description(result.releaseException().code());
>	   g_set_error_literal(error, g_quark_from_string("WEBKIT_DOM"),
description.code, description.name);

Given that we have to change every single call site, I think if we wanted to
improve maintainability of the GTK bindings we could make a helper function
that does all of this. Called like this:

    if (convertExceptionToError(result, error))
	return false;

The convertExceptionToError function template would take ExceptionOr<T>& and
there would probably be another part not in the header that would take an
Exception&&. Maybe could be named something even better that makes it clearer
that if there is no exception it return false.

Not sure what header to stick it in. One neat thing is that then none of these
files would even have to include DOMException.h. Obviously I could have done
that when I did the ExceptionOr project too.

> Source/WebKitLegacy/mac/DOM/ExceptionHandlers.mm:35
>  NSString * const DOMRangeException = @"DOMRangeException";

This string constant and the two below it are really strange now. They are
exported but serve no purpose other than that; they are never used. The entire
public headers <WebKit/DOMRangeException.h>, <WebKit/DOMEventException.h>, and
<WebKit/DOMXPathException.h> are now full of useless stuff. I think we should
take steps to mark all the things in those headers deprecated.

I think we should separate these from DOMException, which is still used in the
Objective-C DOM API. Unlike those constants this one is now no more deprecated
than the rest of that API. If we are trying to keep this Objective-C API alive
and up to date, DOMException should be annotated with NSExceptionName so that
Swift bindings will understand it is an exception.

> Source/WebKitLegacy/mac/DOM/ExceptionHandlers.mm:43
> +    auto description = WebCore::DOMException::description(ec);

The NSException objects we use to communicate DOM errors in the Objective-C API
have various problems that I noticed while looking this over.

The <WebKit/DOMException.h> header has named constants for 15 error codes, but
it does not have the other 10 codes for the newer errors (codes 16-25), nor
does it offer a way to differentiate exceptions produced for the newer DOM
errors that have names but not numeric codes; they all simply have error codes
of 0.

The NSException objects include the error codes in the exception userInfo
dictionary, but strangely they are under an undocumented key, which is the name
of the NSException. That now means they will always be under the key
DOMException. That choice seems a bit peculiar--there is no reason to think
that the exception’s name would also be used as a key in the userInfo
dictionary--but I suppose we can leave it alone for compatibility reasons.
Would be nice to document at least.

The NSException objects we create for DOM errors don’t include the error names
separately, only as substrings in the reason string. It seems to me that if
callers actually need to figure out which DOM error the exception represents,
we want to provide them with the DOM error name somewhere they can get at it,
and I think that would need to be in the userInfo dictionary. So if we wanted
to make this useful we would want to define a key that the error name would be
stored under and that probably would need to go into the
<WebKit/DOMException.h> header. We could even define constants for the error
names put those in the <WebKit/DOMException.h> header too.

> Source/WebKitLegacy/mac/DOM/ExceptionHandlers.mm:45
> +    NSString *exceptionName = DOMException;

I see no reason to use a variable for this any more. Should just use
DOMException when calling exceptionWithName, and include the literal characters
"DOMException" in the reason string; in fact I don’t think we need to repeat
the exception name there any more. In the old days the exception name went hand
in hand with the error code and was needed to correctly understand the meaning
of that number; that consideration is now obsolete.

It also seems strange to include the code "0" explicitly in the reason strings
for all the many errors which don’t have numeric codes.

I’m not sure why the reason strings we create always include "***", but no need
to change that right at this moment I guess. It would be nice to think through
how to make these reason strings as helpful as possible.

It’s unfortunate that we don’t include the error description anywhere in the
reason string. That’s the kind of written-out human-readable string that a
reason string should include to make it helpful.