parse() throws error on valid ReflectOnly Blink extended attributes

[jpmedley]

According to the Blink IDL Extended Attributes spec, ReflectOnly has two valid forms:

[Reflect, ReflectOnly="on"] attribute DOMString toggle;
// Parens may contain 2 to n values.
[Reflect=q, ReflectOnly=("anonymous", "use-credentials")] attribute DOMString quarter;

The second format is causing error such as the following to be thrown:

Got an error during or right after parsing `interface HTMLImageElement`: Expected
identifiers but none found, line 33 (tokens: "\"anonymous\",\"use-credentials\"),")

There are 11 files in Chromium (listed below) that trigger this error. I've already verified that the Blink IDL spec is correct.

blink/renderer/core/html/html_element.idl
blink/renderer/core/html/html_link_element.idl
blink/renderer/core/html/html_table_cell_element.idl
blink/renderer/core/html/html_anchor_element.idl
blink/renderer/core/html/forms/html_form_element.idl
blink/renderer/core/html/html_area_element.idl
blink/renderer/core/html/html_script_element.idl
blink/renderer/core/html/html_iframe_element.idl
blink/renderer/core/html/html_image_element.idl
blink/renderer/core/html/media/html_media_element.idl
blink/renderer/core/dom/element.idl

I would appreciate some haste in addressing this. This affects tooling I've built to increase my velocity at documenting new web platform APIs. If I can assist in some further way, will be happy to do so.

Thanks

[jyasskin]

The ReflectOnly Blink extended attribute has a form different from the five forms used in the WebIDL spec. It's closest to ExtendedAttributeIdentList, but it takes strings instead of tokens. https://github.com/w3c/webidl2.js/blob/develop@{01-14-19}/lib/webidl2.js#L595 says webidl2.js takes the shortcut of parsing less than the spec actually allows, so it should probably grow new support for this sixth form.

[saschanaz]

I think it's worth supporting the string extended-form as it seems it's being used frequently enough. Thoughts? @marcoscaceres

Maybe we should require an option (say "extendedAttributeStrings": true) to allow it.

[marcoscaceres]

Yeah, I think it's fine to support it. But yeah, it's non-conforming so the suggested flag to enable it seems good... maybe we should consider a more general "flavor": "blink", in case there are more Blink specific things. We could also support Gecko specific "isms" later if needed.

[saschanaz]

in case there are more Blink specific things.

I doubt that as AFAIK Web IDL spec only allows extensions for the ExtendedAttribute syntax (whatwg/webidl#574).

[saschanaz]

Maybe we can go further and implement an extendable API:

webidl2.parse(str, {
  extendedAttribute: tokens => { ... }
});

[marcoscaceres]

@jyasskin, @jpmedley, what would you prefer? ^^^

[saschanaz]

Note that there is a discussion to make Reflect* custom extended attribute official.

[jyasskin]

I defer to @jpmedley on what would be easiest to use in his tool. I suspect any of the options would be acceptable.

I disagree with the statement that "it's non-conforming": https://heycam.github.io/webidl/#idl-extended-attributes says

Extended attributes are specified with an ExtendedAttributeList, which is a square bracket enclosed, comma separated list of ExtendedAttributes.

The ExtendedAttribute grammar symbol matches nearly any sequence of tokens

The WebIDL spec happens to only use 5 forms, but https://heycam.github.io/webidl/#extensibility expressly allows extensions to define new extended attributes, with no restriction that they also follow the 5 forms used by the standard extended attributes.

That said, it seems fine to ask callers to define what extra forms they expect to receive.

[jpmedley]

Sorry for the delay in responding. I ran into this again and still need to have this fixed. Any of the solutions suggested would work for my tool.

[saschanaz]

We might just add the support for this syntax, because it's not "non-conforming". We miiight add a validation rule, but the syntax is not incorrect.

[saschanaz]

@jpmedley But just my curiosity, what's your use case to parse Blink IDL with webidl2.js?

Ah, okay, generating documentation. Never mind, sorry.