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

Description Chris Lord 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).

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

Created attachment 418051 [details]
Patch

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

Created attachment 418052 [details]
Patch

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

Created attachment 418055 [details]
Patch

Comment 4 Myles C. Maxfield 2021-01-21 11:54:34 PST

Comment on attachment 418055 [details]
Patch

r=mews

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

Created attachment 418120 [details]
Patch

Comment 6 EWS 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].

Comment 7 Radar WebKit Bug Importer 2021-01-22 03:01:37 PST

<rdar://problem/73492040>

Comment 8 Darin Adler 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.

Comment 9 Myles C. Maxfield 2021-01-25 14:19:03 PST

Looks like this broke the internal WatchOS build

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

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