Closed Bug 1298756 Opened 3 years ago Closed 3 years ago

[WebIDL] Cannot specify type for enum class

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jeremychen, Assigned: xidorn)

References

()

Details

Attachments

(1 file)

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.
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...
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.
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)?
Flags: needinfo?(jeremychen)
Flags: needinfo?(bzbarsky)
Priority: -- → P3
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.
Flags: needinfo?(bzbarsky)
I think automatically pick the minimum necessary one would be enough.
(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.
Flags: needinfo?(jeremychen)
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 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
Attachment #8826053 - Flags: review?(bzbarsky) → review+
Assignee: nobody → xidorn+moz
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a42a1279c216
Use uint8_t/uint16_t as underlying type for enums from webidl. r=bz
https://hg.mozilla.org/mozilla-central/rev/a42a1279c216
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Removing dev-doc-needed keyword, as this bug doesn't look like it affects web developers in any way.
Keywords: dev-doc-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.