Bug 238483

Created attachment 455983 [details]
Patch

Created attachment 455984 [details]
Patch

Created attachment 455985 [details]
Patch

Comment on attachment 455985 [details]
Patch

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

r=me

> Source/WebCore/page/Page.cpp:392
> +    {

nit: Not convinced we need this scope.

> Source/WebCore/page/Page.cpp:396
> +                forEachPage([](Page& page) {

nit: Could use auto&

> LayoutTests/fast/text/install-font-style-recalc.html:4
> +<script src="../../resources/js-test-pre.js"></script>

Can we just include js-test.js..

> LayoutTests/fast/text/install-font-style-recalc.html:20
> +<script src="../../resources/js-test-post.js"></script>

.. and not include the -post.js version?

Committed r292072 (249003@trunk): <https://commits.webkit.org/249003@trunk>

<rdar://problem/91006654>

Comment on attachment 455985 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        FontCache::invalidateAllFontCaches() can't do this directly because it's in platform/ and
> +        therefore isn't allowed to know what Pages are. Instead, this patch takes a process-global
> +        callback and calls that instead. This callback is set at initialization time.

Why is this so different from how SettingsBase::setStandardFontFamily does things?

> Source/WebCore/page/Page.cpp:393
> +        static bool fontCacheInvalidationCallbackRegistered = false;

Looks like addOnlineStateChangeListener is another case of one-time initialization in the Page constructor. Surprised there are only these two things. I suggest grouping this with that in a separate function that can use a single global boolean to call it once rather than using separate booleans for each bit of one-time initialization.

Comment on attachment 455985 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        callback and calls that instead. This callback is set at initialization time.
> 
> Why is this so different from how SettingsBase::setStandardFontFamily does things?

I looked at that when investigating this bug, but I didn't think that model would work because SettingsBase holds a `Page* m_page;`, but this CFNotification is delivered to a static free function. The static function calls into the FontCache::forCurrentThread() which is a singleton in platform/. So, at this point, we don't have a handle to any Page like SettingsBase does; indeed, we can't even speak of Page inside platform/. So, I opted to implement this using this new stored Function mechanism.

>> Source/WebCore/page/Page.cpp:393
>> +        static bool fontCacheInvalidationCallbackRegistered = false;
> 
> Looks like addOnlineStateChangeListener is another case of one-time initialization in the Page constructor. Surprised there are only these two things. I suggest grouping this with that in a separate function that can use a single global boolean to call it once rather than using separate booleans for each bit of one-time initialization.

Right, yes, good idea. I missed that when implementing this. I'll do it in a follow-up patch.

Re-opened since this is blocked by bug 238690

Created attachment 456444 [details]
Patch for relanding

Created attachment 456453 [details]
Patch for relanding

*** Bug 238641 has been marked as a duplicate of this bug. ***

Comment on attachment 456453 [details]
Patch for relanding

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

> Source/WebCore/page/Page.h:968
> +    void firstTimeInitialization();

I recommend adding "static" here.

Committed r292274 (249172@trunk): <https://commits.webkit.org/249172@trunk>