Bug 174771 – Make ExceptionCode a proper enumeration

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Make ExceptionCode a proper enumeration instead of a typedef to uint8_t.

[Chris Dumez]

Created attachment 316263 [details]
Patch

[Sam Weinig]

Comment on attachment 316263 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBody.h:52
> -    void formData(FetchBodyOwner&, Ref<DeferredPromise>&& promise) { promise.get().reject(ExceptionCode { 0 }); }
> +    void formData(FetchBodyOwner&, Ref<DeferredPromise>&& promise) { promise.get().reject(NoException); }

How about NOT_SUPPORTED_ERR instead?

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3574
> -        ExceptionCode ec = 0;
> +        ExceptionCode ec = NoException;

It would be nice to switch these to ExceptionOr<>.

> Source/WebKitLegacy/win/DOMCoreClasses.cpp:824
> -    WebCore::ExceptionCode ec = 0;
> +    WebCore::ExceptionCode ec = WebCore::NoException;

Is this dead code?

> Source/WebKitLegacy/win/WebView.cpp:6045
> -        ExceptionCode ec = 0;
> +        ExceptionCode ec = NoException;

Is this dead code?

[Chris Dumez]

Created attachment 316292 [details]
Patch

[Darin Adler]

Comment on attachment 316292 [details]
Patch

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

> Source/WebCore/dom/Exception.h:29
> +#include "ExceptionCode.h"

Because we added this include here we can remove includes of this file from tons of other places, perhaps almost everywhere else it is included.

> Source/WebCore/dom/ExceptionCode.h:25
> +enum ExceptionCode {

For future clean-up here:

I don’t think any of the values in this enumeration need explicitly set values any more--no reason they need to match the legacy codes visible in JavaScript for example--although we do need to keep them in sync with the array inside DOMException.cpp. There’s no need for the 105, 106 thing, and no reason to leave empty slots where no-longer-used values were. I don’t think we need to include all these “introduced in xxx” comments either; not really important when they were introduced. I also think they should be in alphabetical order, not historical order. But we should still distinguish the ones that are web-exposed and the ones that are not.

We should consider renaming each of these internal constants to use the style that matches the error names in the specification such as HierarchyRequestError rather than the legacy code name HIERARCHY_REQUEST_ERR.

Once everything else is done I think we should consider getting rid of NoException and use optional instead when we need a null value.

[Chris Dumez]

Comment on attachment 316292 [details]
Patch

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

>> Source/WebCore/dom/ExceptionCode.h:25
>> +enum ExceptionCode {
> 
> For future clean-up here:
> 
> I don’t think any of the values in this enumeration need explicitly set values any more--no reason they need to match the legacy codes visible in JavaScript for example--although we do need to keep them in sync with the array inside DOMException.cpp. There’s no need for the 105, 106 thing, and no reason to leave empty slots where no-longer-used values were. I don’t think we need to include all these “introduced in xxx” comments either; not really important when they were introduced. I also think they should be in alphabetical order, not historical order. But we should still distinguish the ones that are web-exposed and the ones that are not.
> 
> We should consider renaming each of these internal constants to use the style that matches the error names in the specification such as HierarchyRequestError rather than the legacy code name HIERARCHY_REQUEST_ERR.
> 
> Once everything else is done I think we should consider getting rid of NoException and use optional instead when we need a null value.

> We should consider renaming each of these internal constants to use the style that matches the error names in the specification such as HierarchyRequestError rather than the legacy code name HIERARCHY_REQUEST_ERR.

I had the same thought and this was my next patch :)

[Chris Dumez]

Created attachment 316302 [details]
Patch

[Chris Dumez]

Comment on attachment 316302 [details]
Patch

Clearing flags on attachment: 316302

Committed r219831: <http://trac.webkit.org/changeset/219831>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.