[WebIDL] Stop automatically applying the ImplementedBy extended attribute to all partial interfaces/dictionaries

[Sam Weinig]

[WebIDL] Stop automatically applying the ImplementedBy extended attribute to all partial interfaces/dictionaries

[Sam Weinig]

Created attachment 408363 [details]
Patch

[Darin Adler]

Comment on attachment 408363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408363&action=review

> Source/WebCore/ChangeLog:13
> +        behavior mush opt in, using the extended attribute "ImplementedBy", which is what the code

typo: "must"

> Source/WebCore/ChangeLog:25
> +        Remove special casing of partial interfaces / dictionaries automatically getting "ImplementedBy"
> +        applied to all members. Maintain the behavior that "ImplementedBy" should not be used when
> +        an attribute has been marked as "Reflect".

When we say "should not be used" we mean "should be silently ignored"? Is that right? That seems to be new. It’s one thing to not implicitly add ImplementedBy when Reflect is specified. Not sure it’s right to ignore it rather than emitting an error message.

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 408363 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408363&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        behavior mush opt in, using the extended attribute "ImplementedBy", which is what the code
> 
> typo: "must"
> 
> > Source/WebCore/ChangeLog:25
> > +        Remove special casing of partial interfaces / dictionaries automatically getting "ImplementedBy"
> > +        applied to all members. Maintain the behavior that "ImplementedBy" should not be used when
> > +        an attribute has been marked as "Reflect".
> 
> When we say "should not be used" we mean "should be silently ignored"? Is
> that right? 

Yeah, that is more clear.

> That seems to be new. It’s one thing to not implicitly add
> ImplementedBy when Reflect is specified. Not sure it’s right to ignore it
> rather than emitting an error message.

The old behavior was that we would skip adding ImplementedBy (and therefore using the MyClass::staticFunction(...) syntax) if an attribute in partial interface had [Reflect]:

-                 if ($interface->isPartial && !$interface->isMixin) {
-                     $attribute->extendedAttributes->{"ImplementedBy"} = $interfaceName if !$attribute->extendedAttributes->{Reflect};
-                 }

I don't think this ends up being that confusing as [Reflect] really means "there is not attribute getter/setter for this, instead call the right hasAttributeWithoutSynchronization (or whatever the appropriate one is for the type) and pass in the appropriate qualified name). If we remove this special case, it would mean instead of specifying ImplementedBy on the partial interface itself, we would have to do it on each member. I think I am ok with keeping this little bit of weirdness.

[Sam Weinig]

Created attachment 408367 [details]
Patch

[EWS]

Committed r266800: <https://trac.webkit.org/changeset/266800>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408367 [details].

[Radar WebKit Bug Importer]

<rdar://problem/68598852>