Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fonts: Store web fonts in the per-Layout FontContext #32303

Merged
merged 1 commit into from
May 20, 2024

Conversation

mrobinson
Copy link
Member

This moves mangement of web fonts to the per-Layout FontContext,
preventing web fonts from being available in different Documents.

Fixes #12920.

Co-authored-by: Mukilan Thiyagarajan mukilan@igalia.com
Signed-off-by: Martin Robinson mrobinson@igalia.com


@servo-wpt-sync
Copy link
Collaborator

🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#46341) with upstreamable changes.

@servo-wpt-sync
Copy link
Collaborator

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#46341).

@servo-wpt-sync
Copy link
Collaborator

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#46341).

font_template_cache: RwLock<HashMap<FontTemplateCacheKey, Vec<FontTemplateRef>>>,
font_group_cache:
RwLock<HashMap<FontGroupCacheKey, Arc<RwLock<FontGroup>>, BuildHasherDefault<FnvHasher>>>,
font_source: Arc<ReentrantMutex<S>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to remove the Arc around font_source and resource_threads.

I'm able to compile and run servo with this patch:

diff --git a/components/gfx/font_context.rs b/components/gfx/font_context.rs
index 560247efe7..0aff4abde2 100644
--- a/components/gfx/font_context.rs
+++ b/components/gfx/font_context.rs
@@ -44,8 +44,8 @@ static SMALL_CAPS_SCALE_FACTOR: f32 = 0.8; // Matches FireFox (see gfxFont.h)
 /// paint code. It talks directly to the font cache thread where
 /// required.
 pub struct FontContext<S: FontSource> {
-    font_source: Arc<ReentrantMutex<S>>,
-    resource_threads: Arc<ReentrantMutex<CoreResourceThread>>,
+    font_source: ReentrantMutex<S>,
+    resource_threads: ReentrantMutex<CoreResourceThread>,
     cache: CachingFontSource<S>,
     web_fonts: CrossThreadFontStore,
     webrender_font_store: CrossThreadWebRenderFontStore,
@@ -60,11 +60,10 @@ impl<S: FontSource> MallocSizeOf for FontContext<S> {
 impl<S: FontSource> FontContext<S> {
     pub fn new(font_source: S, resource_threads: ResourceThreads) -> FontContext<S> {
         #[allow(clippy::default_constructed_unit_structs)]
-        let font_source = Arc::new(ReentrantMutex::new(font_source));
         FontContext {
-            font_source: font_source.clone(),
-            resource_threads: Arc::new(ReentrantMutex::new(resource_threads.core_thread)),
-            cache: CachingFontSource::new(font_source),
+            font_source: ReentrantMutex::new(font_source.clone()),
+            resource_threads: ReentrantMutex::new(resource_threads.core_thread.clone()),
+            cache: CachingFontSource::new(font_source.clone()),
             web_fonts: Arc::new(RwLock::default()),
             webrender_font_store: Arc::new(RwLock::default()),
         }
@@ -508,7 +507,7 @@ impl<FCT: FontSource + Send + 'static> RemoteWebFontDownloader<FCT> {
 
 #[derive(Default)]
 pub struct CachingFontSource<FCT: FontSource> {
-    font_cache_thread: Arc<ReentrantMutex<FCT>>,
+    font_cache_thread: ReentrantMutex<FCT>,
     fonts: RwLock<HashMap<FontCacheKey, Option<FontRef>>>,
     templates: RwLock<HashMap<FontTemplateCacheKey, Vec<FontTemplateRef>>>,
     resolved_font_groups:
@@ -516,9 +515,9 @@ pub struct CachingFontSource<FCT: FontSource> {
 }
 
 impl<FCT: FontSource> CachingFontSource<FCT> {
-    fn new(font_cache_thread: Arc<ReentrantMutex<FCT>>) -> Self {
+    fn new(font_cache_thread: FCT) -> Self {
         Self {
-            font_cache_thread,
+            font_cache_thread: ReentrantMutex::new(font_cache_thread),
             fonts: Default::default(),
             templates: Default::default(),
             resolved_font_groups: Default::default(),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I've gone ahead and integrated your changes.

@servo-wpt-sync
Copy link
Collaborator

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#46341).

@servo-wpt-sync
Copy link
Collaborator

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#46341).

@servo-wpt-sync
Copy link
Collaborator

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#46341).

@mukilan mukilan added this pull request to the merge queue May 20, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 20, 2024
This moves mangement of web fonts to the per-Layout `FontContext`,
preventing web fonts from being available in different Documents.

Fixes #12920.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>
Co-authored-by: Mukilan Thiyagarajan <mukilan@igalia.com>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 20, 2024
This moves mangement of web fonts to the per-Layout `FontContext`,
preventing web fonts from being available in different Documents.

Fixes servo#12920.

Co-authored-by: Mukilan Thiyagarajan <mukilan@igalia.com>
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
@servo-wpt-sync
Copy link
Collaborator

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#46341).

@mrobinson mrobinson added this pull request to the merge queue May 20, 2024
Merged via the queue into servo:main with commit be5b527 May 20, 2024
9 checks passed
@mrobinson mrobinson deleted the font-context-web-fonts branch May 20, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change keying of fonts in the font cache thread
3 participants