1298756 - [WebIDL] Cannot specify type for enum class

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Jeremy Chen ([e-mail])]

At present, we declare enum in .webidl, and the code generator would generate c++ enum class for us. However, typing an enum class is not supported yet, so a default uint32_t typed enum class is always generated. This doesn't make sense to me since most of the enumerations should be covered by uint8_t. Adding this support could save some memory usage as well.

This is not specified in https://www.w3.org/TR/WebIDL/, so I assume this is probably just an implementation issue.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

I guess we can probably add a Mozilla-specific extended attribute to enum to indicate its underlying type... Not sure if our code generator supports attaching extended attribute to enum...

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Alternatively, we can make the codegen automatically choose the minimum underlying type which can is enough for putting all values. Not sure whether that would cause any issue.

Comment 3

[Andrew Overholt [:overholt]]

Boris, is this something we've considered?
Jeremy, do you have a test case or some reason to believe this is urgent (not doubting you, just asking to get a sense of priority)?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I don't think we really considered this when implementing enums initially; I seem to recall that predating real enum class support in Gecko's compilers.

We changed that to enum class in bug 869073 but at the time just made them uint32_t.

We can certainly either pick a size class automatically or allow specifying one, sure.  We currently don't support extended attributes on enums, but could add that.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

I think automatically pick the minimum necessary one would be enough.

Comment 6

[Jeremy Chen ([e-mail])]

(In reply to Andrew Overholt [:overholt] from comment #3)
> Jeremy, do you have a test case or some reason to believe this is urgent
> (not doubting you, just asking to get a sense of priority)?

I don't have a particular test, and I also believe this is not something I would say "urgent". Just found this while doing bug 1297306 which would try to check if an enum is fit within an integral type by reusing http://searchfox.org/mozilla-central/rev/3611a7607db7554b9d7065ebfef3d5111f7171b8/layout/style/nsCSSProps.h#335.

I'm not sure if we have such a tool could measure the performance/space improvements between w/ and w/o adding this support. This just seems like a right thing to do.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1298756 - Use uint8_t/uint16_t as underlying type for enums from webidl.

Review commit: https://reviewboard.mozilla.org/r/104098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104098/

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8826053 [details]
Bug 1298756 - Use uint8_t/uint16_t as underlying type for enums from webidl.

https://reviewboard.mozilla.org/r/104098/#review104822

::: dom/media/webaudio/AudioNodeStream.cpp
(Diff revision 1)
> -  // Make sure that we're not clobbering any significant bits by fitting these
> -  // values in 16 bits.
> -  MOZ_ASSERT(int(aChannelCountMode) < INT16_MAX);
> -  MOZ_ASSERT(int(aChannelInterpretation) < INT16_MAX);

FWIW, these assertions were added in bug 865234 part 2, which made these two member fields to be 16bit-length bit field, and thus this assertion.

Those fields have become normal field, so these assertions are not useful now.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5cbf297e7c0c223917732f390d972c86a8e1705

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8826053 [details]
Bug 1298756 - Use uint8_t/uint16_t as underlying type for enums from webidl.

https://reviewboard.mozilla.org/r/104098/#review104994

r=me

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a42a1279c216
Use uint8_t/uint16_t as underlying type for enums from webidl. r=bz

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a42a1279c216

Comment 13

[Chris Mills [:cmills]]

Removing dev-doc-needed keyword, as this bug doesn't look like it affects web developers in any way.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I just documented this at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings?document_saved=true#Enumeration_types