Add support for extended attributes on types in Web IDL

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: d, Assigned: manishearth)

Tracking

Trunk
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(11 attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Reporter

Description

2 years ago
Web IDL was recently revised to add extended attributes on types: https://github.com/heycam/webidl/pull/286. The existing extended attributes [EnforceRange], [Clamp], and [TreatNullAs] were moved to this model, so that they can no longer be used on attributes/arguments, but instead only on types. For example.

This is now making its way into specifications, see e.g. https://github.com/whatwg/html/pull/2580. It will also be crucial for SharedArrayBuffer API safelisting (although from what I understand Gecko has a workaround for that for now).

Gecko's IDL parser should be updated for this change.
Peter, is this something you can take?
Flags: needinfo?(peterv)
Priority: -- → P2
Assignee

Comment 2

4 months ago

I added support for this in Servo (which shares the webidl parser), might try to upstream it

https://github.com/servo/servo/pull/22874

That would be much appreciated! There are some codegen bits that would need to be fixed too, but I think the parser is the bulk of the work.

Assignee

Comment 4

4 months ago

An interesting complication I'm hitting is that this should also remove the ability to use these attributes on dictionary members and attrs, however we use extra attributes like [ChromeOnly] in a bunch of places, so we'll have to special-case parsing for those (not something I needed to do on the servo side).

I'll probably make it such that they continue to work the same, but resolve the parser ambiguity for attributes on the inside.

Assignee

Comment 5

4 months ago

Do we have tests for ensuring that things like [Clamp] etc work? Or do we just rely on their usage in the rest of the code?

Assignee

Comment 8

4 months ago

Depends on D19734

This should work.

A potential bit of cleanup I can do is to stop carrying around clamp/enforce/treatnullas info on arguments, attributes, and dictionaries, instead choosing to forward those to their child types. Not sure if I need to.

Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Flags: needinfo?(peterv)
Flags: needinfo?(bzbarsky)

Added patches making things hard errors, and updating specs.

I'm sorry about the review lag here. I needed to sit down and carefully check the changes against the spec grammar... Future reviews should be much faster.

Flags: needinfo?(bzbarsky)

Addressed review comments for everything except writing tests). Working on these now.

Tests pushed

Updated.

Comment 23

4 months ago
Pushed by mgoregaokar@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb694b612b1b
Part 1: Add parser support for attributes on types in WebIDL; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/c3f37539cb31
Part 2: Add codegen support for attributes on types in WebIDL; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/7fd8a3c1f827
Part 3: Add a codegen test; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/7705b1ebb154
Part 4: Update HTML spec idl files to use attributes on types; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/4bb00dc53459
Part 5: Add support for attributes on types in arguments; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/1430913c5e9e
Part 6: Add support for attributes on types in dictionaries; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/bfb153c7f9c3
Part 7: Add ArgumentRest entry to simplify parsing; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/8f9509e82439
Part 8: Make it a hard error to apply type attributes on arguments, attrs, and dict members when it's unambiguous, update webidls; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/ae30401e7988
Part 9: Clean up codegen now that Clamp/EnforceRange can only be applied to types; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/6c09eccc4bd1
Part 10: Make it a hard error to apply TreatNullAs on non-types; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/00461a29f650
Part 11: Add a bunch of parser tests; r=bzbarsky

Ah, some failures in the testsuite got introduced with the changes. Fixed.

Flags: needinfo?(manishearth)

Comment 28

4 months ago
Pushed by mgoregaokar@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21deba7e36c5
Part 1: Add parser support for attributes on types in WebIDL; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/87d4c51be96f
Part 2: Add codegen support for attributes on types in WebIDL; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/ee5af9ad9e45
Part 3: Add a codegen test; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/3fccace0ea88
Part 4: Update HTML spec idl files to use attributes on types; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/f4ec249fc52a
Part 5: Add support for attributes on types in arguments; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/a64f79c456c9
Part 6: Add support for attributes on types in dictionaries; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/b267c4012f94
Part 7: Add ArgumentRest entry to simplify parsing; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/eadc7a9fc56c
Part 8: Make it a hard error to apply type attributes on arguments, attrs, and dict members when it's unambiguous, update webidls; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/b26067ba0f78
Part 9: Clean up codegen now that Clamp/EnforceRange can only be applied to types; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/759c00e21197
Part 10: Make it a hard error to apply TreatNullAs on non-types; r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/4166cae81546
Part 11: Add a bunch of parser tests; r=bzbarsky
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.