Closed Bug 1121623 Opened 5 years ago Closed 3 months ago

Consider a better default for 'concrete'

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(11 files)

22.94 KB, patch
Details | Diff | Splinter 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
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?
Flags: needinfo?(peterv)
And if we decide not to do this, we should just flag StyleSheet as not concrete.
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.
Priority: -- → P5
Component: DOM → DOM: Core & HTML

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 :-/.

Flags: needinfo?(peterv)

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.

Assignee: nobody → bzbarsky

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

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

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

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

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

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

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.

Pushed by bzbarsky@mozilla.com:
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

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?

Flags: needinfo?(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.

Flags: needinfo?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.