Closed Bug 1163877 Opened 5 years ago Closed 4 years ago

allow a FontFace to be inserted into multiple FontFaceSets

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- affected
firefox44 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
(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.
(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
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attached patch Part 4: Tests. (obsolete) — Splinter Review
Attachment #8605564 - Flags: review?(jdaggett)
Attachment #8605561 - Flags: review?(jdaggett) → review+
Attachment #8605562 - Flags: review?(jdaggett) → review+
Attachment #8605563 - Flags: review?(jdaggett) → review+
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.
Blocks: 1196924
Attachment #8605564 - Attachment is obsolete: true
Attachment #8605564 - Flags: review?(jdaggett)
Attachment #8674659 - Flags: review?(jdaggett)
Comment on attachment 8674659 [details] [diff] [review]
Part 4: Tests. (v2)

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

Looks good!
Attachment #8674659 - Flags: review?(jdaggett) → review+
You need to log in before you can comment on or make changes to this bug.