Bug 220969 - Ensure FontGenericFamilies uses AtomString where possible
Summary: Ensure FontGenericFamilies uses AtomString where possible
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-26 03:05 PST by Chris Lord
Modified: 2021-02-02 03:06 PST (History)
13 users (show)

See Also:


Attachments
Patch (7.06 KB, patch)
2021-01-26 03:10 PST, Chris Lord
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 2021-01-26 03:05:47 PST
Bug 220801 changed the function prototypes and storage on FontGenericFamilies from AtomString to String, to allow for easier cross-thread copies. We should still ensure that the actual storage is AtomString where possible though, to avoid string duplication.
Comment 1 Chris Lord 2021-01-26 03:10:12 PST
Created attachment 418387 [details]
Patch
Comment 2 Darin Adler 2021-01-26 09:33:28 PST
Comment on attachment 418387 [details]
Patch

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

> Source/WebCore/page/SettingsBase.cpp:98
> +    bool changes = fontGenericFamilies().setStandardFontFamily(AtomString(family), script);

I think we need a why comment explaining why we convert a string to an AtomString even though we are then calling a function that takes a String. Otherwise it just seems like a mistake to be undone.

Obviously we don’t want to repeat that comment over and over again.

And we should make sure we are thinking clearly about the benefit here; writing the comment helps us be explicit about the benefit we think accrues. We’re thinking that converting to AtomString lets us save memory because we think the font family names will be repeated and so it’s good to share the storage. That works across multiple Settings objects, each of which has its own FontGenericFamilies object. And the code goes here because here we know it’s running on the main thread, whereas other uses of FontGenericFamilies can be across threads?

Writing this out somewhere is needed; otherwise the code is too subtle to keep working.
Comment 3 Radar WebKit Bug Importer 2021-02-02 03:06:32 PST
<rdar://problem/73872757>