Clean up ExceptionCode enumeration

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Clean up ExceptionCode enumeration.

[Darin Adler]

Some things I noticed:

URLMismatchError is not generated by any WebKit code that is currently in the tree. I think it was needed only in older versions of the Shared Worker specification and so may never end up needing to be used.

StackOverflowError sounds a little bit like UnknownError or maybe RangeError. It seems to be generated in a case that could exist in other web browsers too so might be worth getting into the standard.

ExistingExceptionError is the one really special case; totally different from actual exceptions. The comment on that one misspells "propagated" and also doesn’t seem to make sufficiently clear that the thrown exception object is already stored the JavaScript VM in that case.

We should consider using the terminology from the WebIDL specification, calling TypeError and RangeError simple exceptions. Except that the terms "error" and "exception" don’t seem to be clearly enough distinguished.

I think we should have probably have first NoException, then the DOM exception error names sorted alphabetically, then the simple exceptions sorted alphabetically, then our two special ones last each in their own separate paragraph.

[Chris Dumez]

Created attachment 316354 [details]
Patch

[Chris Dumez]

Sorry, I uploaded my patch before seeing your comments.

(In reply to Darin Adler from comment #1)
> Some things I noticed:
> 
> URLMismatchError is not generated by any WebKit code that is currently in
> the tree. I think it was needed only in older versions of the Shared Worker
> specification and so may never end up needing to be used.

Hmm. It is in the Web IDL specification and not deprecated so not sure I want to remove it.

> 
> StackOverflowError sounds a little bit like UnknownError or maybe
> RangeError. It seems to be generated in a case that could exist in other web
> browsers too so might be worth getting into the standard.

Yes, we may want to standardize it indeed.

> 
> ExistingExceptionError is the one really special case; totally different
> from actual exceptions. The comment on that one misspells "propagated" and
> also doesn’t seem to make sufficiently clear that the thrown exception
> object is already stored the JavaScript VM in that case.

I fixed the typo but kept ExistingExceptionError as is for now.

> 
> We should consider using the terminology from the WebIDL specification,
> calling TypeError and RangeError simple exceptions. Except that the terms
> "error" and "exception" don’t seem to be clearly enough distinguished.

I tried to match the spec naming in my patch.

> 
> I think we should have probably have first NoException,

I actually dropped it in favor of std::optional<>.

> then the DOM
> exception error names sorted alphabetically,

Hmm, I used the order of the spec. I could be convinced to do alphabetically but matching the spec is nice too.

> then the simple exceptions
> sorted alphabetically, then our two special ones last each in their own
> separate paragraph.

[Chris Dumez]

Created attachment 316355 [details]
Patch

[Darin Adler]

Comment on attachment 316355 [details]
Patch

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

Looks pretty good. Are you sure there’s nowhere that is initializing the exception to 0 or to { } or something that will now give us IndexSizeError rather than no error?

> Source/WebCore/Modules/indexeddb/shared/IDBError.h:38
> +    WEBCORE_EXPORT IDBError(std::optional<ExceptionCode> = std::nullopt, const String& message = emptyString());

Maybe null string instead of empty string? Slightly more efficient. But I guess doesn’t match the old behavior.

> Source/WebCore/dom/ExceptionCode.h:58
> +    MaximumDOMExceptionCode = NotAllowedError,

Not sure we really need this. Is this better than asserting that it’s NotAllowedError?

[Build Bot]

Comment on attachment 316355 [details]
Patch

Attachment 316355 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4182815

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 316358 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 316372 [details]
Patch

[Darin Adler]

Comment on attachment 316372 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/shared/IDBError.h:38
> +    WEBCORE_EXPORT IDBError(std::optional<ExceptionCode> = std::nullopt, const String& message = { });

I think that maybe this should be marked explicit.

[Chris Dumez]

Committed r219901: <http://trac.webkit.org/changeset/219901>