1311198 - FontFaceSet should be a setlike

Status: ASSIGNED

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

Description

[Boris Zbarsky [:bzbarsky]]

It has some hacks around lack of setlike support and comments in the interface, etc.  Any reason it can't just be converted, offhand, since we support setlike?

Comment 1

[Cameron McCormack (:heycam)]

No reason I can think of we can't use setlike now that we support it.

Comment 2

[Cameron McCormack (:heycam)]

Well, I guess I would have to investigate what the setlike C++ interface is since I haven't used it so far, to give a more definitive answer.  Does the interface we use allow responding to add() and remove() calls with custom behaviour?

When I tried to comment out all of those IDL methods that are emulating it, and replace it with setlike<FontFaceSet>, I get this:

WebIDL.WebIDLError: error: Member 'addEventListener' conflicts with reserved setlike method., /z/moz/b/dom/webidl/FontFaceSet.webidl line 47:2
  setlike<FontFace>;
  ^
/z/moz/b/dom/webidl/EventTarget.webidl line 33:2
  void addEventListener(DOMString type,
  ^

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> When I tried to comment out all of those IDL methods that are emulating it,
> and replace it with setlike<FontFaceSet>, I get this:

Er, replace it with setlike<FontFace>, which the error messages show I did actually type. :-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Does the interface we use allow responding to add() and remove() calls with custom behaviour?

Yes, if you define them yourself, as the spec does.  So for example you would do this:

  setlike<FontFace>;
  [Throws] void add(FontFace font);

and this will generate the following things:

1)  A JS add() method which will call the Add() method on your underlying
    C++ FontFaceSet object, so you can do whatever custom behavior you need.
2)  A C++ mozilla::dom::FontFaceSetBinding::SetlikeHelpers::Add method with this signature:

      void
      Add(mozilla::dom::FontFaceSet* self, FontFace& aKey, ErrorResult& aRv);

    which your Add() method is responsible for calling to actually add the FontFace to the set.

Similar for Delete() and Clear(); those have these signatures:

      void
      Clear(mozilla::dom::FontFaceSet* self, ErrorResult& aRv);
      bool
      Delete(mozilla::dom::FontFaceSet* self, FontFace& aKey, ErrorResult& aRv);

We should really get this documented at <https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings>.  qdot, do you have time to do that, by any chance?

> When I tried to comment out all of those IDL methods that are 
> emulating it, and replace it with setlike<FontFaceSet>, I get this:

Codegen bug.  Filed bug 1311362 with patch.

Comment 5

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

Yeah, there's currently no documentation about generated *like/iterable stuff on the bindings page, and 2 outstanding bugs on that otherwise (bug 1255247, bug 1289611). I'll try to sit down and just get most of the documentation done later this week if I can.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So I looked at this a bit.  FontFaceSet currently does work not just on modification but also on queries (has() and the iterator bits); specifically it does some sort of flushing.  Is that flushing actually needed there?

Note that the iterator thing doesn't make sense to me, since it doesn't flush on calls to next() and whatever is being flushed can presumably become dirty between those calls...

Comment 7

[Cameron McCormack (:heycam)]

Oh, good point about not flushing on next().  We have this wording in the spec:

  The set entries for a document’s font source must be initially populated with all the
  CSS-connected FontFace objects from all of the CSS @font-face rules in the document’s
  stylesheets, in document order. As @font-face rules are added or removed from a
  stylesheet, or stylesheets containing @font-face rules are added or removed, the
  corresponding CSS-connected FontFace objects must be added or removed from the
  document’s font source, and maintain this ordering.

  https://drafts.csswg.org/css-font-loading-3/#document-font-face-set

which I take to mean the FontFaceSet should appear live to script, in terms of which FontFace objects for @font-face rules are in it.

Flushing inside next() is not really compatible with what https://heycam.github.io/webidl/#es-iterator says to do for setlike iterators (i.e., defer to the backing set's @@iterator).  And there's no way to override it with an |iterator;| declaration like you can for individual setlike methods...

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #7)
> which I take to mean the FontFaceSet should appear live to script, in terms
> of which FontFace objects for @font-face rules are in it.

i.e. it would be consistent with that requirement to flush inside next().

Comment 9

[Boris Zbarsky [:bzbarsky]]

So we don't add font-face rules to the set until the flush happens?  How viable is just changing that?

Note that flushing in has() is also not compatible with the IDL spec's description of how setlike works...

Comment 10

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

Documentation done, clearing ni?.

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> So we don't add font-face rules to the set until the flush happens?  How
> viable is just changing that?

Right.  Well, it would be mean doing this work in response to any style sheet change.  We already eagerly parse style sheets when <style> element contents change, but the gathering of @font-face rules is currently done as part of updating the cascade in nsCSSRuleProcessor::RefreshRuleCascade, and I guess we don't want to do the majority of that work until we flush style.

> Note that flushing in has() is also not compatible with the IDL spec's
> description of how setlike works...

Erk. :(

Comment 12

[Emilio Cobos Álvarez [:emilio]]

The spec has significantly changed here, in a way that is supposed to allow us to do this. I filed bug 1782846 for that but I guess I should just dupe here.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Attachment: WIP: Bug 1311198 - FontFaceSet should be a setlike.

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Thanks Peter!

Comment 19

[Daniel Holbert [:dholbert]]

FWIW this is actively causing cosmetic issues (wrong font and/or broken linewrapping) in evite invitations; see bug 1831949 comment 18 for details.

Comment 20

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 1 - Implement ECMA{Set|Map} in Gecko. r=edgar!

Comment 21

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 1 - Implement ECMA{Set|Map} in Gecko. r=edgar!

Comment 22

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1311198, part 2 - Introduce [DOMBackedStorage] for WebIDL setlike/maplike, provide SetlikeMixin/MaplikeMixin. r=edgar!

WebIDL setlike<T> and maplike<K,V> declarations previously used a JS
Set/Map stored in a reserved slot. C++ code could not iterate the
backing data, forcing every interface to maintain a parallel C++ mirror.

This patch changes the WebIDL codegen so that setlike/maplike interfaces
are backed by C++-owned ECMASet/ECMAMap collections, provided through a
SetlikeMixin/MaplikeMixin base class. Generated method bodies call
directly into the C++ data structure instead of the JS API.

• Codegen emits SetlikeMixin/MaplikeMixin typedefs per binding namespace
• CGECMACollectionMethodGenerator generates method bodies for non-object
  value types; interfaces with maplike<DOMString, object> fall back to
  the existing JS Map/Set path
• Physical-index iterators (ECMACollectionIterator.h) for ES-compatible
  mutation-during-iteration semantics
• New [NativeKeyType=Atom] extended attribute allows using nsAtom* as
  the native key type with automatic conversion at the JS/C++ boundary

Comment 23

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 3 - Migrate CustomStateSet to SetlikeMixin. r=keithamus!

Comment 24

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 4 - Migrate ViewTransitionTypeSet to SetlikeMixin. r=emilio!

Comment 25

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 5 - Migrate GPUSupportedFeatures and WGSLLanguageFeatures to SetlikeMixin. r=#webgpu-reviewers!

Comment 26

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 6 - Migrate EventCounts to MaplikeMixin. r=smaug!

Comment 27

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 7 - Migrate PlacesEventCounts to MaplikeMixin. r=mak!

Comment 28

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 8 - Migrate AudioParamMap to MaplikeMixin. r=padenot!

Comment 29

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 9 - Migrate MIDIInputMap and MIDIOutputMap to MaplikeMixin. r=padenot!

Comment 30

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 10 - Migrate Highlight to SetlikeMixin, remove mRanges. r=emilio!

Highlight now inherits Highlight_Binding::SetlikeMixin which provides
the ECMASet storage and SetData() accessor. The parallel mRanges array
is removed since ECMASet supports direct C++ iteration.

Comment 31

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198, part 11 - Migrate HighlightRegistry to MaplikeMixin with [NativeKeyType=Atom], remove mHighlightsOrdered. r=emilio!

HighlightRegistry now uses [NativeKeyType=Atom] on its maplike declaration,
which stores nsAtom* keys natively with automatic string-to-atom conversion
at the JS/C++ boundary. This eliminates the parallel mHighlightsOrdered
array that was needed because the old JS Map used string keys while C++
code worked with atoms.

Comment 32

[Phabricator Automation]

Comment on attachment 9567034 [details]
WIP: Bug 1311198, part 1 - Implement ECMA{Set|Map} in Gecko. r=edgar!

Revision D292469 was moved to bug 2030981. Setting attachment 9567034 [details] to obsolete.

Comment 33

[Phabricator Automation]

Comment on attachment 9567035 [details]
Bug 1311198, part 2 - Introduce [DOMBackedStorage] for WebIDL setlike/maplike, provide SetlikeMixin/MaplikeMixin. r=edgar!

Revision D292470 was moved to bug 2030981. Setting attachment 9567035 [details] to obsolete.

Comment 34

[Phabricator Automation]

Comment on attachment 9567036 [details]
WIP: Bug 1311198, part 3 - Migrate CustomStateSet to SetlikeMixin. r=keithamus!

Revision D292471 was moved to bug 2030984. Setting attachment 9567036 [details] to obsolete.

Comment 35

[Phabricator Automation]

Comment on attachment 9567037 [details]
WIP: Bug 1311198, part 4 - Migrate ViewTransitionTypeSet to SetlikeMixin. r=emilio!

Revision D292472 was moved to bug 2030984. Setting attachment 9567037 [details] to obsolete.

Comment 36

[Phabricator Automation]

Comment on attachment 9567038 [details]
WIP: Bug 1311198, part 5 - Migrate GPUSupportedFeatures and WGSLLanguageFeatures to SetlikeMixin. r=#webgpu-reviewers!

Revision D292473 was moved to bug 2030984. Setting attachment 9567038 [details] to obsolete.

Comment 37

[Phabricator Automation]

Comment on attachment 9567040 [details]
WIP: Bug 1311198, part 6 - Migrate EventCounts to MaplikeMixin. r=smaug!

Revision D292475 was moved to bug 2030984. Setting attachment 9567040 [details] to obsolete.

Comment 38

[Phabricator Automation]

Comment on attachment 9567041 [details]
WIP: Bug 1311198, part 7 - Migrate PlacesEventCounts to MaplikeMixin. r=mak!

Revision D292476 was moved to bug 2030984. Setting attachment 9567041 [details] to obsolete.

Comment 39

[Phabricator Automation]

Comment on attachment 9567042 [details]
WIP: Bug 1311198, part 8 - Migrate AudioParamMap to MaplikeMixin. r=padenot!

Revision D292477 was moved to bug 2030984. Setting attachment 9567042 [details] to obsolete.

Comment 40

[Phabricator Automation]

Comment on attachment 9567044 [details]
WIP: Bug 1311198, part 9 - Migrate MIDIInputMap and MIDIOutputMap to MaplikeMixin. r=padenot!

Revision D292479 was moved to bug 2030984. Setting attachment 9567044 [details] to obsolete.

Comment 41

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1311198 - Implement FontFaceSet as setlike. r=emilio!