Bug 234049 - Move FontFamilySpecificationCoreText's fontMap to FontCache
Summary: Move FontFamilySpecificationCoreText's fontMap to FontCache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cameron McCormack (:heycam)
URL:
Keywords: InRadar
Depends on:
Blocks: 233488
  Show dependency treegraph
 
Reported: 2021-12-08 18:14 PST by Cameron McCormack (:heycam)
Modified: 2022-07-21 14:32 PDT (History)
11 users (show)

See Also:


Attachments
Patch (24.58 KB, patch)
2021-12-08 18:16 PST, Cameron McCormack (:heycam)
mmaxfield: review+
Details | Formatted Diff | Diff
Patch (23.77 KB, patch)
2022-07-20 21:16 PDT, Matt Woodrow
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2021-12-08 18:14:46 PST
Move FontFamilySpecificationCoreText's fontMap to FontCache
Comment 1 Cameron McCormack (:heycam) 2021-12-08 18:16:26 PST
Created attachment 446469 [details]
Patch
Comment 2 Myles C. Maxfield 2021-12-10 15:11:19 PST
Comment on attachment 446469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446469&action=review

> Source/WebCore/page/cocoa/MemoryReleaseCocoa.mm:51
> +    // FIXME: Should clear this on all workers.

I think some members on the team prefer for FIXMEs to have Bugzilla links

> Source/WebCore/platform/graphics/cocoa/FontFamilySpecificationCoreText.cpp:-39
> -struct FontFamilySpecificationKey {

It's kind of sad that this has to live in a different file, far from its use. This cache isn't really reusable, and FontFamilySpecificationCoreText.cpp is already fairly focused and cohesive with this in it. Can you think of a way to keep this code closer to its use?
Comment 3 Cameron McCormack (:heycam) 2021-12-10 15:26:33 PST
The obvious thing would be to make the HashMap be heap allocated and then there'd be no need for the hash traits to be in a header.  What do you think of the balance of having the HashMap be in a heap allocation versus keeping it all in the same file?
Comment 4 Myles C. Maxfield 2021-12-10 16:00:44 PST
🤔
Comment 5 Myles C. Maxfield 2021-12-10 16:01:17 PST
This doesn't seem worth a heap allocation, I think
Comment 6 Radar WebKit Bug Importer 2021-12-15 18:15:17 PST
<rdar://problem/86552797>
Comment 7 Matt Woodrow 2022-07-20 21:16:20 PDT
Created attachment 461073 [details]
Patch
Comment 8 EWS 2022-07-21 14:32:23 PDT
Committed 252710@main (498e08540016): <https://commits.webkit.org/252710@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 461073 [details].