[Cocoa] Automatically relayout the page when new fonts are installed
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>