[webkit-reviews] review granted: [Bug 220969] Ensure FontGenericFamilies uses AtomString where possible : [Attachment 418387] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 220969: Ensure FontGenericFamilies uses AtomString where possible
https://bugs.webkit.org/show_bug.cgi?id=220969

Attachment 418387: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 418387
  --> https://bugs.webkit.org/attachment.cgi?id=418387
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.