1121623 - Consider a better default for 'concrete'

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I was considering something like "not concrete if has descendants".

It turns out that this requires _more_ 'concrete' annotations than we have now, at least for the moment.

That said, it also caught a bug: StyleSheet was not marked 'concrete':False, but should have been to avoid generating useless code.

This would be the main reason to change the default: make the failure mode be that code is not generated as opposed to it being generated...  Thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch I tried

Comment 2

[Boris Zbarsky [:bzbarsky]]

And if we decide not to do this, we should just flag StyleSheet as not concrete.

Comment 3

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 4

[Peter Van der Beken [:peterv]]

I think I'm ok with this, even if there will be more annotations. As you said, it's better to not generate unused code.

I would have expected concrete with descendants to be rare :-/.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, I tried merging this to tip, with a refinement to default to 'concrete': True for interfaces with a constructor. At that point, we can remove 34 'concrete': False annotations, at the cost of adding 20 'concrete': True ones. I also discovered that we had a bunch of pointless WrapObject methods that we're now able to remove. Patches coming up.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1121623 part 1. Switch PromiseDebugging to being a namespace, since that's how we use it. r=peterv

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1121623 part 2. Switch AddonManagerPermissions to being a namespace. r=peterv

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1121623 part 3. Switch WebrtcGlobalInformation to being a namespace. r=peterv

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1121623 part 4. Remove AudioScheduledSourceNode::WrapObject. r=padenot

It's dead code, because AudioScheduledSourceNode is an abstract class and all
subclasses override WrapObject.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1121623 part 5. Remove PerformanceEntry::WrapObject. r=peterv

It's dead code, because we never create PerformanceEntry objects directly and
subclasses override WrapObject.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1121623 part 6. Remove ReportBody::WrapObject. r=baku

It's dead code, because ReportBody is an abstract class and subclasses override
WrapObject.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1121623 part 7. Remove AuthenticatorResponse::WrapObject. r=peterv

It's dead code because we never create AuthenticatorResponse objects directly,
and all subclasses override WrapObject.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1121623 part 8. Remove MIDIPort::WrapObject. r=baku

It's dead code because we never create MIDIPort objects directly,
and all subclasses override WrapObject.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1121623 part 9. Change example codegen to not spit out WrapObject bits for non-concrete interfaces. r=peterv

This should keep people from marking things concrete unnecessarily just so
their example-generated WrapObject works.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1121623 part 10. Use a more-accurate default value for 'concrete' in Web IDL bindings. r=peterv

The idea is that we should only generate concreate-binding (wrap methods, etc)
machinery for an interface by default if we have reason to expect that the
interface is used as the primary interface for some objects. Two clear signals
that would indicate that are the interface being a leaf interface (with no
descendants) and the interface having a constructor. Other cases would require
a 'concrete' annotation in Bindings.conf.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08973de3de9e
part 1.  Switch PromiseDebugging to being a namespace, since that's how we use it.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/5d10803def3e
part 2.  Switch AddonManagerPermissions to being a namespace.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/8e672b4e0a3e
part 3.  Switch WebrtcGlobalInformation to being a namespace.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/4d9f7977a103
part 4.  Remove AudioScheduledSourceNode::WrapObject.  r=padenot
https://hg.mozilla.org/integration/autoland/rev/1e7a53dbff7b
part 5.  Remove PerformanceEntry::WrapObject.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/47671db88526
part 6.  Remove ReportBody::WrapObject.  r=baku
https://hg.mozilla.org/integration/autoland/rev/b5e4781f21b3
part 7.  Remove AuthenticatorResponse::WrapObject.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/9fc4bc908818
part 8.  Remove MIDIPort::WrapObject.  r=baku
https://hg.mozilla.org/integration/autoland/rev/99d7cbadce92
part 9.  Change example codegen to not spit out WrapObject bits for non-concrete interfaces.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/1bec5b571ed1
part 10.  Use a more-accurate default value for 'concrete' in Web IDL bindings.  r=peterv

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/08973de3de9e
https://hg.mozilla.org/mozilla-central/rev/5d10803def3e
https://hg.mozilla.org/mozilla-central/rev/8e672b4e0a3e
https://hg.mozilla.org/mozilla-central/rev/4d9f7977a103
https://hg.mozilla.org/mozilla-central/rev/1e7a53dbff7b
https://hg.mozilla.org/mozilla-central/rev/47671db88526
https://hg.mozilla.org/mozilla-central/rev/b5e4781f21b3
https://hg.mozilla.org/mozilla-central/rev/9fc4bc908818
https://hg.mozilla.org/mozilla-central/rev/99d7cbadce92
https://hg.mozilla.org/mozilla-central/rev/1bec5b571ed1

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I'm puzzled with https://hg.mozilla.org/mozilla-central/rev/1bec5b571ed1 change when we have documentation that concrete is True by default
https://searchfox.org/mozilla-central/rev/1133b6716d9a8131c09754f3f29288484896b8b6/dom/bindings/Bindings.conf#21-24
When is one supposed to use 'concrete' now?

Comment 19

[Boris Zbarsky [:bzbarsky]]

The only reason to use 'concrete' now is if you have a non-leaf interface without a constructor that is nevertheless being used as the primary interface of some object. I put up https://phabricator.services.mozilla.com/D35465 to fix the docs.