1163877 - allow a FontFace to be inserted into multiple FontFaceSets

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Cameron McCormack (:heycam)]

The Font Loading API spec allows for a FontFace to be inserted into multiple FontFaceSets, which we currently disallow by throwing an exception if you try.

I'm wondering how difficult it would be to allow the one gfxUserFontEntry to be used in multiple gfxUserFontSets.  A gfxUserFontEntry does have a pointer back to its gfxUserFontSet, which it uses for logging, noting use of local() rules, generation incrementing, checking for private browsing mode, and calling the various virtual methods for loading that end up calling into the FontFaceSet.

It might be feasible to have a "main" gfxUserFontSet that a gfxUserFontEntry is associated with (the one from the same document whose window the FontFace object was created in) for most of those things, but for things which alter state on a gfxUserFontSet (such as setting mLocalRulesUsed and incrementing the generation) we'd need to go through the list of other gfxUserFontSets we're in to update that state too.

Alternatively, we could clone a gfxUserFontEntry as soon as we add its FontFace to a second FontFaceSet.  If we're in the process of loading, I guess we could note the cloned gfxUserFontEntry on the nsFontFaceLoader.

Comment 1

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #0)
> Alternatively, we could clone a gfxUserFontEntry as soon as we add its
> FontFace to a second FontFaceSet.  If we're in the process of loading, I
> guess we could note the cloned gfxUserFontEntry on the nsFontFaceLoader.

Though if we did clone them before loading, any subsequent load one one wouldn't be reflected in the others.

Comment 2

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #0)
> It might be feasible to have a "main" gfxUserFontSet that a gfxUserFontEntry
> is associated with (the one from the same document whose window the FontFace
> object was created in) for most of those things, but for things which alter
> state on a gfxUserFontSet (such as setting mLocalRulesUsed and incrementing
> the generation) we'd need to go through the list of other gfxUserFontSets
> we're in to update that state too.

This approach seems to be working.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e534635e6651

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add storage for other FontFaceSets a FontFace is in.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 2: Allow FontFaces to be added to multiple FontFaceSets.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 3: Update state on, and reflow documents for, all FontFaceSets that contain a FontFace whose user font entry updated.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Part 4: Tests.

Comment 7

[John Daggett (:jtd)]

Comment on attachment 8605564 [details] [diff] [review]
Part 4: Tests.

Review of attachment 8605564 [details] [diff] [review]:
-----------------------------------------------------------------

The new tests seem fine but maintaining the cross links of FontFaceSet's seems a bit brittle to me. I think it would make sense to have a few more tests that test whether (1) rule-connected faces can't be added across FontFaceSet's and (2) that the FontFaceSet arrays are correct after the UpdateRules method runs. For example, adding faces both directly to a FontFaceSet and via additional @font-face rules.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Part 4: Tests. (v2)

Comment 9

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b7fe600c7c2

Comment 10

[John Daggett (:jtd)]

Comment on attachment 8674659 [details] [diff] [review]
Part 4: Tests. (v2)

Review of attachment 8674659 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d415a463b0cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2c70f62872
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ad23cd3aa0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e822c40683a1

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d415a463b0cd
https://hg.mozilla.org/mozilla-central/rev/2d2c70f62872
https://hg.mozilla.org/mozilla-central/rev/b1ad23cd3aa0
https://hg.mozilla.org/mozilla-central/rev/e822c40683a1