Open Bug 1675853 Opened 3 years ago Updated 1 year ago

FontFaceSet.add/remove shouldn't need to call FlushUserFontSet.

Categories

(Core :: Layout, task)

task

Tracking

()

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: leave-open)

Attachments

(4 files)

No description provided.

They only deal with non-rule faces, so we don't need to flush (which
only ensures that rule faces are properly created, for the purposes of
this function).

Also add a couple stylistic cleanups.

There's no way these need to do a flush as they just return data from
the descriptors etc.

Similarly .loaded also relies on the font face set.

These are a bit more tricky because DescriptorUpdated() does check
whether we're in the font set, but only to invalidate it, so I think
it's not needed either (as if it's dirty, we'd invalidate it regardless
when the time comes).

Depends on D96341

This one is even more subtle. This could cause a call from SetStatus()
to OnFontFaceStatusChange() to not be done as part of the load()
function (if the face is about to get added to the set or such).

However, when it's actually added via UpdateRules, we'd properly
invalidate the relevant state as well, so I think this is also fine, and
our existing tests also agree.

Depends on D96342

Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7714e0e74a3
FontFace getters don't need to flush the fontset. r=heycam

Thanks to one of the examples cam found in the patch from comment 3, we found issues everywhere:

emilio
heycam: fun, the font API is so broken :(
heycam: (as in non-interoperable)

This probably shouldn't throw per your example:
<style>@font-face { font-family: Ahem; src: url('/tests/dom/base/test/Ahem.ttf'); }</style>
<iframe src="about:blank"></iframe>
<script>
onload = function() {
  let f = [...document.fonts][0];
  document.fonts.add(f);
  document.querySelector("style").remove();
  document.querySelector("iframe").contentDocument.fonts.add(f);
}
</script>
But it does because we flush the font face set of the wrong document. Blink will silently do nothing, ughhh
Well, actually blink seems like it'd happily share FontFace objects with other documents
Even if CSS-connected
This is all broken
heycam
hmmmm
emilio
Blink never throws in .add(), just silently returns. And the CSS-connected check is basically document->fontfacerules.contains(face), which is clearly broken if face doesn't come from document
Ugh, I'll paste this somewhere and file spec issues / bugs about this tomorrow.
would it make sense to make the "CSS-connected" flag immutable?
(as in, make it just mean "this came from a rule")
assuming people aren't doing this "grab face, remove sheet, try to do stuff with it", it seems like it'd nicely simplify the model...

So, browser/spec issue time...

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9e2f1e514e84
FontFace.load() doesn't need to flush the user fontset. r=heycam

Should be green once bug 1675950 re-lands.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/017aa3b39d73
FontFace.load() doesn't need to flush the user fontset. r=heycam

Backed out changeset 017aa3b39d73 (Bug 1675853) for causing wpt failures in ch-pseudo-recalc-on-font-load.html

Failure log: https://treeherder.mozilla.org/logviewer?job_id=321179115&repo=autoland&lineNumber=1581

Backout by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e5683b64ed3
Backed out changeset 017aa3b39d73 for causing wpt failures in ch-pseudo-recalc-on-font-load.html

The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)

The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)

The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)

The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)

No

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

Attachment

General

Created:
Updated:
Size: