Bug 238483 - [Cocoa] Automatically relayout the page when new fonts are installed
Summary: [Cocoa] Automatically relayout the page when new fonts are installed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 238641 (view as bug list)
Depends on: 238690
Blocks:
  Show dependency treegraph
 
Reported: 2022-03-28 19:50 PDT by Myles C. Maxfield
Modified: 2022-04-02 22:39 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.70 KB, patch)
2022-03-28 19:54 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (3.74 KB, patch)
2022-03-28 19:56 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (6.41 KB, patch)
2022-03-28 20:52 PDT, Myles C. Maxfield
cdumez: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for relanding (12.50 KB, patch)
2022-04-01 21:44 PDT, Myles C. Maxfield
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for relanding (12.68 KB, patch)
2022-04-02 00:11 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2022-03-28 19:50:06 PDT
[Cocoa] Automatically relayout the page when new fonts are installed
Comment 1 Myles C. Maxfield 2022-03-28 19:54:14 PDT
Created attachment 455983 [details]
Patch
Comment 2 Myles C. Maxfield 2022-03-28 19:56:34 PDT
Created attachment 455984 [details]
Patch
Comment 3 Myles C. Maxfield 2022-03-28 20:52:04 PDT
Created attachment 455985 [details]
Patch
Comment 4 Chris Dumez 2022-03-28 21:43:01 PDT
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?
Comment 5 Myles C. Maxfield 2022-03-29 14:49:50 PDT
Committed r292072 (249003@trunk): <https://commits.webkit.org/249003@trunk>
Comment 6 Radar WebKit Bug Importer 2022-03-29 14:50:20 PDT
<rdar://problem/91006654>
Comment 7 Darin Adler 2022-03-29 15:02:02 PDT
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 8 Myles C. Maxfield 2022-03-29 15:55:52 PDT
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.
Comment 9 WebKit Commit Bot 2022-04-01 17:37:44 PDT
Re-opened since this is blocked by bug 238690
Comment 10 Myles C. Maxfield 2022-04-01 21:44:22 PDT
Created attachment 456444 [details]
Patch for relanding
Comment 11 Myles C. Maxfield 2022-04-02 00:11:23 PDT
Created attachment 456453 [details]
Patch for relanding
Comment 12 Myles C. Maxfield 2022-04-02 00:11:59 PDT
*** Bug 238641 has been marked as a duplicate of this bug. ***
Comment 13 Darin Adler 2022-04-02 09:31:16 PDT
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.
Comment 14 Myles C. Maxfield 2022-04-02 22:39:42 PDT
Committed r292274 (249172@trunk): <https://commits.webkit.org/249172@trunk>