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!