[webkit-reviews] review granted: [Bug 233986] Move FontDatabases to FontCache : [Attachment 446313] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Cameron McCormack (:heycam)
<heycam at apple.com>'s request for review:
Bug 233986: Move FontDatabases to FontCache
https://bugs.webkit.org/show_bug.cgi?id=233986

Attachment 446313: Patch

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




--- Comment #2 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 446313
  --> https://bugs.webkit.org/attachment.cgi?id=446313
Patch

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

> Source/WebCore/ChangeLog:15
> +	   This patch moves the FontDatabase instances to live on the
FontCache,
> +	   of which we already have one per thread.

Yes I think this is the right thing to do.

> Source/WebCore/ChangeLog:17
> +	   We also move the FontDatabase definition to a separate file, to
reduce

"We"?

> Source/WebCore/platform/graphics/FontCache.h:186
> +#if PLATFORM(COCOA)

I think we generally don't guard declarations (as opposed to definitions) if
they're not called.

> Source/WebCore/platform/graphics/FontCache.h:213
> +    FontDatabase m_databaseAllowingUserInstalledFonts {
AllowUserInstalledFonts::Yes };
> +    FontDatabase m_databaseDisallowingUserInstalledFonts {
AllowUserInstalledFonts::No };

��

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1054
> +    auto& fontDatabase = database(allowUserInstalledFonts);

thanks

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1145
>	   FontCache::invalidateAllFontCaches();

The implementation of this will execute the above two lines?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1253
> +    auto& fontDatabase =
database(fontDescription.shouldAllowUserInstalledFonts());

I kind of think the name of this function is too generic. Web browsers have
lots of databases inside them.