Bug 220801

Summary:

Move FontGenericFamilies storage from SettingsBase to Settings::Values

Product:

WebKit

Reporter:

Chris Lord <clord>

Component:

WebCore Misc.

Assignee:

Chris Lord <clord>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annulen, cdumez, darin, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, jer.noble, macpherson, menard, mmaxfield, ryuan.choi, sergio, simon.fraser, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=220969

Bug Depends on:

Bug Blocks:

202793

Attachments:

Description	Flags
Patch	ews-feeder: commit-queue-
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none

Chris Lord

Reported 2021-01-21 09:21:49 PST

Currently, font generic families are stored on the Settings base class, which means they don't get transferred over to Workers via Settings::Values - this is necessary for OffscreenCanvas, and perhaps there are other reasons this would be desirable (it would seem logical to keep all Settings storage in Settings::Values rather than just almost-all).

Attachments
Patch (28.00 KB, patch) 2021-01-21 09:31 PST, Chris Lord	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (28.18 KB, patch) 2021-01-21 09:38 PST, Chris Lord	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (30.18 KB, patch) 2021-01-21 09:53 PST, Chris Lord	no flags	Details Formatted Diff Diff
Patch (31.12 KB, patch) 2021-01-22 02:01 PST, Chris Lord	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Chris Lord

Comment 1 2021-01-21 09:31:26 PST

Created attachment 418051 [details] Patch

Chris Lord

Comment 2 2021-01-21 09:38:41 PST

Created attachment 418052 [details] Patch

Chris Lord

Comment 3 2021-01-21 09:53:52 PST

Created attachment 418055 [details] Patch

Myles C. Maxfield

Comment 4 2021-01-21 11:54:34 PST

Comment on attachment 418055 [details] Patch r=mews

Chris Lord

Comment 5 2021-01-22 02:01:58 PST

Created attachment 418120 [details] Patch

EWS

Comment 6 2021-01-22 02:59:57 PST

Committed r271742: <https://trac.webkit.org/changeset/271742> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418120 [details].

Radar WebKit Bug Importer

Comment 7 2021-01-22 03:01:37 PST

<rdar://problem/73492040>

Darin Adler

Comment 8 2021-01-22 11:48:52 PST

Here are the parts of this we want to be careful about: 1) Performance cost, slowing down by calling AtomString repeatedly. If the cost is at all an issue, it can be minimized by "atomizing" when setting family names even though the storage is not done as AtomString. It’s quick to detect the AtomString bit. That can be done by converting to AtomString and back to a String. 2) Memory cost, is there some path that ends up accumulating multiple copies of the same family name. There are multiple ways to make sure this doesn’t occur. When using AtomString this is automatically taken care of, we also share a single string. Without AtomString we need to double check that we don’t unnecessarily end up with multiple copies of the same string in memory. Could explicitly optimize with things like checks for strings that happen to be equal. When we do an isolated copy we always guarantee we have fresh strings, so we might have to actively do something so we don’t end up with many multiple copies. I doubt either of those two are a big problem, but both are worth thinking through or measuring.

Myles C. Maxfield

Comment 9 2021-01-25 14:19:03 PST

Looks like this broke the internal WatchOS build

Darin Adler

Comment 10 2021-01-25 14:55:26 PST

Not sure this change has anything to do with the breakage I saw.

Note You need to log in before you can comment on or make changes to this bug.