1436329 - WebIDL parser doesn't support "partial dictionary"

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), enhancement, P2)

Description

[Tim Taubert [:ttaubert] (inactive)]

The term "partial dictionary" is defined here:

https://heycam.github.io/webidl/#dfn-partial-dictionary

The WebAuthn spec uses it in their extension model, all extensions are optional.

https://w3c.github.io/webauthn/#dictdef-authenticationextensionsclientinputs

partial dictionary AuthenticationExtensionsClientInputs {
  USVString appid;
};

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Actually, bumping this to P2 since this is blocking WebAuthn. 

Tim, is this stopping extension definitions from working?

Comment 2

[J.C. Jones [:jcj] (he/they)]

It's not blocking, but it has made us sort of hack around it. See [1].


[1] https://searchfox.org/mozilla-central/source/dom/webidl/WebAuthentication.webidl#106

Comment 3

[Diego Pino]

Attachment: Bug-1436329-Parse-WebIDL-partial-dictionary.patch

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 9026331 [details] [diff] [review]
Bug-1436329-Parse-WebIDL-partial-dictionary.patch

Review of attachment 9026331 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/parser/WebIDL.py
@@ +629,2 @@
>  
>      def addExtendedAttributes(self, attrs):

I don't think it makes sense to reuse IDLPartialInterfaceOrNamespace, dictionaries don't have extended attributes. Probably better to have a IDLPartialDictionary class.

@@ +1845,5 @@
>          self.parent = parent
>          self._finished = False
>          self.members = list(members)
> +        self._partialDictionaries = []
> +        self._extendedAttrDict = {}

Again, dictionaries don't have extended attributes.

@@ +1885,5 @@
>  
> +        # Now go ahead and merge in our partial dictionaries.
> +        for partial in self._partialDictionaries:
> +            partial.finish(scope)
> +            self.addExtendedAttributes(partial.propagatedExtendedAttrs)

And here.

@@ +1996,5 @@
> +    def addPartialDictionary(self, partial):
> +        assert self.identifier.name == partial.identifier.name
> +        self._partialDictionaries.append(partial)
> +
> +    def getExtendedAttribute(self, name):

And here.

@@ +5713,5 @@
>              [location, identifier, members])
>  
> +    def p_PartialDictionary(self, p):
> +        """
> +            PartialDictionary : DICTIONARY IDENTIFIER Inheritance LBRACE DictionaryMembers RBRACE SEMICOLON

This is not what's in the spec. A partial dictionary doesn't provide inheritance:

PartialDictionary ::
    dictionary identifier { DictionaryMembers } ;

::: dom/bindings/parser/tests/test_dictionary.py
@@ +49,5 @@
> +    harness.check(dict1.members[2].identifier.name, "g",
> +                  "g should come after d")
> +    harness.check(dict1.members[3].identifier.name, "h",
> +                  "h should be last")
> +

I think you should also add a test for name duplication across normal and partial dictionary (see following test).

Comment 5

[Diego Pino]

Attachment: Bug-1436329-Parse-WebIDL-partial-dictionary.patch

Thanks for the detailed review. I updated the patch addressing the comments in https://bugzilla.mozilla.org/show_bug.cgi?id=1436329#c4

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 9028400 [details] [diff] [review]
Bug-1436329-Parse-WebIDL-partial-dictionary.patch

Review of attachment 9028400 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/parser/WebIDL.py
@@ +5666,5 @@
>      def handlePartialObject(self, location, identifier, nonPartialConstructor,
>                              nonPartialConstructorArgs,
>                              partialConstructorArgs):
>          """
>          This handles partial objects (interfaces and namespaces) by checking for

This comment needs updating (adding dictionary and IDLDictionary in the right places).

@@ +5706,5 @@
> +        if isinstance(nonPartialObject, IDLDictionary):
> +            partialDictionary = IDLPartialDictionary(
> +                *(partialConstructorArgs + [nonPartialObject]))
> +            return partialDictionary
> +        else:

We usually don't do else after return. Please also assert here that isinstance(nonPartialObject, (IDLInterface, IDLNamespace)).

::: dom/bindings/parser/tests/test_dictionary.py
@@ +82,5 @@
> +        results = parser.finish()
> +    except:
> +        threw = True
> +
> +    harness.ok(threw, "Should not allow name duplication across normal a partial dictionary")

s/a/and/

Comment 7

[Diego Pino]

Attachment: Bug-1436329-Parse-WebIDL-partial-dictionary.patch

Addressed comments in latest review.

Comment 8

[Diego Pino]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d85f82228bee18421ba3a83fe49f8221a1e585cb

Comment 9

[Diego Pino]

Try seems Ok. I'd need to get the bug assigned so I can add tag :checkin-needed, or otherwise I'd someone to land it for me.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f32ccd251ef
Parse WebIDL "partial dictionary" r=peterv

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/7f32ccd251ef