Bug 199769

<rdar://problem/49390297>

Created attachment 374055 [details] Patch

Created attachment 374062 [details] Patch

Comment on attachment 374062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374062&action=review > Source/WTF/wtf/Platform.h:1536 > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000) No need for iOS version check, we only support building for 12.2 and newer). I *think* that this check as is disables the feature on watchOS and tvOS, which may or may not be desirable. But let’s be explicit about it if we want this.

Comment on attachment 374062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374062&action=review > Source/WTF/wtf/Platform.h:1537 > +#define HAVE_DISALLOW_USER_INSTALLED_FONTS 1 “Have disallowed” doesn’t make much sense.

Thanks for the review, I'll update it accordingly. I tried coming with a test but failed to do it so far, if anybody has an idea, let me know. It also seems that styles keep a shouldAllowUserInstalledFonts() that is not always in sync with the corresponding setting when changed through internals API.

(In reply to Alexey Proskuryakov from comment #4) > Comment on attachment 374062 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374062&action=review > > > Source/WTF/wtf/Platform.h:1536 > > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000) > > No need for iOS version check, we only support building for 12.2 and newer). OK > I *think* that this check as is disables the feature on watchOS and tvOS, > which may or may not be desirable. But let’s be explicit about it if we want > this. I think it is fine as is. > > Source/WTF/wtf/Platform.h:1537 > > +#define HAVE_DISALLOW_USER_INSTALLED_FONTS 1 > > “Have disallowed” doesn’t make much sense. Will change to HAVE(CAN_DISALLOW_USER_INSTALLED_FONTS)

Created attachment 374125 [details] Patch

Comment on attachment 374125 [details] Patch Rejecting attachment 374125 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 374125, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WTF/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/12746368

Created attachment 374169 [details] Patch for landing

Created attachment 374195 [details] patch

Created attachment 374213 [details] With tests

Created attachment 374218 [details] With tests

Comment on attachment 374218 [details] With tests View in context: https://bugs.webkit.org/attachment.cgi?id=374218&action=review > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:842 > +#if !HAVE(CAN_DISALLOW_USER_INSTALLED_FONTS) Nit: How about HAVE(DISALLOWABLE_USER_INSTALLED_FONTS) > Tools/WebKitTestRunner/TestController.cpp:1399 > + else if (key == "allowUserInstalledFonts") I'm amazed this didn't exist already.

Comment on attachment 374218 [details] With tests View in context: https://bugs.webkit.org/attachment.cgi?id=374218&action=review > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:573 > + return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts()); This should have already been called before this function. Why not put this call at the end of lookupFallbackFont()? It would be more performant there.

Comment on attachment 374218 [details] With tests View in context: https://bugs.webkit.org/attachment.cgi?id=374218&action=review > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:99 > + 41D5B62622DD9D36000F4C4A /* FakeHelvetica-SingleExtendedCharacter.ttf in Resources */ = {isa = PBXBuildFile; fileRef = 41D5B62522DD9D36000F4C4A /* FakeHelvetica-SingleExtendedCharacter.ttf */; }; Woah, how did you build this font?

(In reply to Myles C. Maxfield from comment #15) > Comment on attachment 374218 [details] > With tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374218&action=review > > > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:573 > > + return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts()); > > This should have already been called before this function. Why not put this > call at the end of lookupFallbackFont()? It would be more performant there. That is the first thing I did to verify this was the issue. I moved this wrapping so that this was making sure preparePlatformFont would always return a 'safe' font. For instance, there is no guarantee to have the same fontDescription.shouldAllowUserInstalledFonts value. I guess we could beef-up the optimisation if-check to validate this, or use an ASSERT. Is it perf sensitive? > View in context: > https://bugs.webkit.org/attachment.cgi?id=374218&action=review > > > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:99 > > + 41D5B62622DD9D36000F4C4A /* FakeHelvetica-SingleExtendedCharacter.ttf in Resources */ = {isa = PBXBuildFile; fileRef = 41D5B62522DD9D36000F4C4A /* FakeHelvetica-SingleExtendedCharacter.ttf */; }; > > Woah, how did you build this font? I used FontForge, my first and probably last work on editing fonts.

(In reply to Myles C. Maxfield from comment #14) > Comment on attachment 374218 [details] > With tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374218&action=review > > > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:842 > > +#if !HAVE(CAN_DISALLOW_USER_INSTALLED_FONTS) > > Nit: How about HAVE(DISALLOWABLE_USER_INSTALLED_FONTS) OK > > Tools/WebKitTestRunner/TestController.cpp:1399 > > + else if (key == "allowUserInstalledFonts") > > I'm amazed this didn't exist already. I think we should probably modify all existing tests to use this one and not the internals settings one.

(In reply to youenn fablet from comment #17) > (In reply to Myles C. Maxfield from comment #15) > > Comment on attachment 374218 [details] > > With tests > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=374218&action=review > > > > > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:573 > > > + return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts()); > > > > This should have already been called before this function. Why not put this > > call at the end of lookupFallbackFont()? It would be more performant there. > > That is the first thing I did to verify this was the issue. > > I moved this wrapping so that this was making sure preparePlatformFont would > always return a 'safe' font. > For instance, there is no guarantee to have the same > fontDescription.shouldAllowUserInstalledFonts value. > I guess we could beef-up the optimisation if-check to validate this, or use > an ASSERT. > > Is it perf sensitive? Or maybe optimize createFontForInstalledFonts in case font already has the right attribute.

Comment on attachment 374218 [details] With tests View in context: https://bugs.webkit.org/attachment.cgi?id=374218&action=review >>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:573 >>> + return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts()); >> >> This should have already been called before this function. Why not put this call at the end of lookupFallbackFont()? It would be more performant there. > > That is the first thing I did to verify this was the issue. > > I moved this wrapping so that this was making sure preparePlatformFont would always return a 'safe' font. > For instance, there is no guarantee to have the same fontDescription.shouldAllowUserInstalledFonts value. > I guess we could beef-up the optimisation if-check to validate this, or use an ASSERT. > > Is it perf sensitive? Creating fonts affects PLT and is perf sensitive. I like the ASSERT() idea. Can we put the createFontForInstalledFonts() call in lookupFallbackFont() and add an ASSERT() in preparePlatformFont()? >>> Tools/WebKitTestRunner/TestController.cpp:1399 >>> + else if (key == "allowUserInstalledFonts") >> >> I'm amazed this didn't exist already. > > I think we should probably modify all existing tests to use this one and not the internals settings one. Is there an internals settings one? There's an internals function to install some fonts, but none that I can see to disable user-installed fonts. In general, I believe internals is preferable to the <!-- webkit-test-runner [ allowUserInstalledFonts=false ] --> thing. If we can migrate to that, we should. (Also, moving to internals should require 0 lines of code since all Settings are already exposed to internals already. You should already just be able to say if (window.internals) window.internals.settings.setShouldAllowUserInstalledFonts(false); And it should just work on ToT.

> In general, I believe internals is preferable to the <!-- webkit-test-runner > [ allowUserInstalledFonts=false ] --> thing. If we can migrate to that, we > should. (Also, moving to internals should require 0 lines of code since all > Settings are already exposed to internals already. You should already just > be able to say > > if (window.internals) > window.internals.settings.setShouldAllowUserInstalledFonts(false); > > And it should just work on ToT. internals.settings.setShouldAllowUserInstalledFonts is affecting the setting and will adjust the setting in some of the places it is stored but not all. Some rendering code is sticking with the old initial value for instance. Hence why I went with the webkit-test-runner option.

(In reply to youenn fablet from comment #21) > > In general, I believe internals is preferable to the <!-- webkit-test-runner > > [ allowUserInstalledFonts=false ] --> thing. If we can migrate to that, we > > should. (Also, moving to internals should require 0 lines of code since all > > Settings are already exposed to internals already. You should already just > > be able to say > > > > if (window.internals) > > window.internals.settings.setShouldAllowUserInstalledFonts(false); > > > > And it should just work on ToT. > > internals.settings.setShouldAllowUserInstalledFonts is affecting the setting > and will adjust the setting in some of the places it is stored but not all. > Some rendering code is sticking with the old initial value for instance. > Hence why I went with the webkit-test-runner option. I'm surprised at the idea that, because something is buggy, we should build a new way of doing the same thing, rather than just fixing the bug(s). Especially because the replacement method is less preferable than the original method.

Created attachment 374252 [details] Patch

Created attachment 374263 [details] Patch

Created attachment 374281 [details] Patch

(In reply to youenn fablet from comment #25) > Created attachment 374281 [details] > Patch Made sure that the cached font descriptors in FontDatabase have exactly the right fallback option.

Comment on attachment 374281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374281&action=review > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:572 > + return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts()); I thought we needed to guard this; only call it when it's actually necessary. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:882 > + addAttributesForInstalledFonts(attributes.get(), allowUserInstalledFonts); I don't think this is actually necessary. You're already calling this in preparePlatformFont().

Comment on attachment 374281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374281&action=review >> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:572 >> + return createFontForInstalledFonts(originalFont, fontDescription.shouldAllowUserInstalledFonts()); > > I thought we needed to guard this; only call it when it's actually necessary. createFontForInstalledFonts is a no-op if the fallback attribute is matching the policy. >> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:882 >> + addAttributesForInstalledFonts(attributes.get(), allowUserInstalledFonts); > > I don't think this is actually necessary. You're already calling this in preparePlatformFont(). It does help as preparePlatformFont will in most cases become a no-op, especially since we are caching these descriptors, this ensure we do not change the attribute again and again in preparePlatformFont

Comment on attachment 374281 [details] Patch Clearing flags on attachment: 374281 Committed r247566: <https://trac.webkit.org/changeset/247566>

All reviewed patches have been landed. Closing bug.