Bug 174812 - Clean up ExceptionCode enumeration
Summary: Clean up ExceptionCode enumeration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-24 21:43 PDT by Chris Dumez
Modified: 2017-07-25 19:35 PDT (History)
11 users (show)

See Also:


Attachments
Patch (15.09 KB, patch)
2017-07-24 22:39 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.11 KB, patch)
2017-07-24 22:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-elcapitan (751.15 KB, application/zip)
2017-07-25 00:00 PDT, Build Bot
no flags Details
Patch (15.60 KB, patch)
2017-07-25 09:57 PDT, Chris Dumez
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-07-24 21:43:50 PDT
Clean up ExceptionCode enumeration.
Comment 1 Darin Adler 2017-07-24 22:31:40 PDT
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.
Comment 2 Chris Dumez 2017-07-24 22:39:25 PDT
Created attachment 316354 [details]
Patch
Comment 3 Chris Dumez 2017-07-24 22:44:48 PDT
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.
Comment 4 Chris Dumez 2017-07-24 22:46:18 PDT
Created attachment 316355 [details]
Patch
Comment 5 Darin Adler 2017-07-24 23:48:56 PDT
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?
Comment 6 Build Bot 2017-07-25 00:00:31 PDT
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.
Comment 7 Build Bot 2017-07-25 00:00:32 PDT
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
Comment 8 Chris Dumez 2017-07-25 09:57:54 PDT
Created attachment 316372 [details]
Patch
Comment 9 Darin Adler 2017-07-25 17:47:17 PDT
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.
Comment 10 Chris Dumez 2017-07-25 19:35:01 PDT
Committed r219901: <http://trac.webkit.org/changeset/219901>