193036 2018-12-26 05:10:50 -0800 [FreeType] Restore conditional compilation logic for recent HarfBuzz refactoring 2018-12-27 02:23:09 -0800 1 1 1 Unclassified WebKit Text WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jmason webkit-unassigned cgarcia commit-queue don.olmstead mcatanzaro mmaxfield webkit-bug-importer oldest_to_newest 1490807 0 jmason 2018-12-26 05:10:50 -0800 Recent changes were made (bug# 192589) to refactor HarfBuzz font creation from OpenTypeMathData to FontPlatformData. In the process, conditional compilation directives were lost. The attached patch restores the conditional compilation logic. 1490808 1 358077 jmason 2018-12-26 05:15:18 -0800 Created attachment 358077 Patch 1490815 2 358077 mcatanzaro 2018-12-26 10:16:59 -0800 Comment on attachment 358077 Patch r=me to get back to the previous status quo, but it looks problematic. Jim, what port are you building, and with what build flags? Hey Don, here's another one. ENABLE(OPENTYPE_MATH) is not a valid feature flag, but it's being used throughout several files under platform/graphics/opentype. It's defined only in OptionsGTK.cmake using cpp flags: add_definitions(-DENABLE_OPENTYPE_MATH=1) Then looking at USE(HARFBUZZ), my initial reaction is confusion. Surely USE(FREETYPE) must imply USE(HARFBUZZ), but it looks like USE(HARFBUZZ) is only set in OptionsPlayStation.cmake, which is nuts because GTK and WPE both use harfbuzz too, so it must be set somewhere else, but I can't find it. It's used in FontCascade.cpp, HbUniquePtr.h, and OpenTypeMathData.[cpp,h] so that's surely not enough places to actually build if harfbuzz were actually not present. 1490820 3 jmason 2018-12-26 11:03:31 -0800 (In reply to Michael Catanzaro from comment #2) > Jim, > what port are you building, and with what build flags? > > Hey Don, here's another one. ENABLE(OPENTYPE_MATH) is not a valid feature > flag, but it's being used throughout several files under > platform/graphics/opentype. It's defined only in OptionsGTK.cmake using cpp > flags Indeed, OPENTYPE_MATH is being set for me automatically, as I am building using an older version of HarfBuzz. OptionsGTK.cmake sets the flag if HarfBuzz is older than version 1.3.3, and I am using 1.0.6. When the code was in OpenTypeMathData, it was compiled according to the construct: #if ENABLE(OPENTYPE_MATH) #elif USE(HARFBUZZ) ..code that was refactored lived here.. Thus, the code was compiled only if USE(HARFBUZZ) && !ENABLE(OPENTYPE_MATH). Conditioning compilation on !ENABLE(OPENTYPE_MATH) is necessary, as the code references an API that is not exposed by the earlier versions of HarfBuzz. 1490828 4 358077 commit-queue 2018-12-26 12:42:54 -0800 Comment on attachment 358077 Patch Clearing flags on attachment: 358077 Committed r239554: <https://trac.webkit.org/changeset/239554> 1490829 5 commit-queue 2018-12-26 12:42:56 -0800 All reviewed patches have been landed. Closing bug. 1490830 6 webkit-bug-importer 2018-12-26 12:43:23 -0800 <rdar://problem/46951923> 1490891 7 jmason 2018-12-27 02:23:09 -0800 Thanks for the quick review, Michael. To summarize, !ENABLE(OPENTYPE_MATH) is the active part of this fix. Without it, users of older versions of HarfBuzz encounter effectively dead code that does not compile. USE(HARFBUZZ) was included in the fix, as it was part of the original logic. Whether it is necessary or redundant in other situations, I cannot say. However, I can say in this specific case, it is unnecessary. 358077 2018-12-26 05:15:18 -0800 2018-12-26 12:42:54 -0800 Patch bug-193036-20181226131517.patch text/plain 2035 jmason SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIzOTU1MikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDE4LTEyLTI2ICBKaW0gTWFz b24gIDxqbWFzb25AaWJpbnguY29tPgorCisgICAgICAgIFtGcmVlVHlwZV0gUmVzdG9yZSBjb25k aXRpb25hbCBjb21waWxhdGlvbiBsb2dpYyBmb3IgcmVjZW50IEhhcmZCdXp6IHJlZmFjdG9yaW5n CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTMwMzYK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBsYXRm b3JtL2dyYXBoaWNzL0ZvbnRQbGF0Zm9ybURhdGEuaDoKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFw aGljcy9mcmVldHlwZS9Gb250UGxhdGZvcm1EYXRhRnJlZVR5cGUuY3BwOgorCiAyMDE4LTEyLTI0 ICBGdWppaSBIaXJvbm9yaSAgPEhpcm9ub3JpLkZ1amlpQHNvbnkuY29tPgogCiAgICAgICAgIFJl bW92ZSAidXNpbmcgbmFtZXNwYWNlIHN0ZDsiCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9y bS9ncmFwaGljcy9Gb250UGxhdGZvcm1EYXRhLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3MvRm9udFBsYXRmb3JtRGF0YS5oCShyZXZpc2lvbiAyMzk1NTIp CisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9Gb250UGxhdGZvcm1EYXRhLmgJ KHdvcmtpbmcgY29weSkKQEAgLTE2OCw3ICsxNjgsOSBAQCBwdWJsaWM6CiAjZW5kaWYKIAogI2lm IFVTRShGUkVFVFlQRSkKKyNpZiBVU0UoSEFSRkJVWlopICYmICFFTkFCTEUoT1BFTlRZUEVfTUFU SCkKICAgICBIYlVuaXF1ZVB0cjxoYl9mb250X3Q+IGNyZWF0ZU9wZW5UeXBlTWF0aEhhcmZCdXp6 Rm9udCgpIGNvbnN0OworI2VuZGlmCiAgICAgYm9vbCBoYXNDb21wYXRpYmxlQ2hhcm1hcCgpIGNv bnN0OwogICAgIEZjUGF0dGVybiogZmNQYXR0ZXJuKCkgY29uc3Q7CiAgICAgYm9vbCBpc0ZpeGVk V2lkdGgoKSBjb25zdCB7IHJldHVybiBtX2ZpeGVkV2lkdGg7IH0KSW5kZXg6IFNvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL2ZyZWV0eXBlL0ZvbnRQbGF0Zm9ybURhdGFGcmVlVHlwZS5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZnJlZXR5 cGUvRm9udFBsYXRmb3JtRGF0YUZyZWVUeXBlLmNwcAkocmV2aXNpb24gMjM5NTUyKQorKysgU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZnJlZXR5cGUvRm9udFBsYXRmb3JtRGF0YUZy ZWVUeXBlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjk2LDYgKzI5Niw3IEBAIFJlZlB0cjxTaGFy ZWRCdWZmZXI+IEZvbnRQbGF0Zm9ybURhdGE6Om8KICAgICByZXR1cm4gU2hhcmVkQnVmZmVyOjpj cmVhdGUoV1RGTW92ZShkYXRhKSk7CiB9CiAKKyNpZiBVU0UoSEFSRkJVWlopICYmICFFTkFCTEUo T1BFTlRZUEVfTUFUSCkKIEhiVW5pcXVlUHRyPGhiX2ZvbnRfdD4gRm9udFBsYXRmb3JtRGF0YTo6 Y3JlYXRlT3BlblR5cGVNYXRoSGFyZkJ1enpGb250KCkgY29uc3QKIHsKICAgICBDYWlyb0Z0RmFj ZUxvY2tlciBjYWlyb0Z0RmFjZUxvY2tlcihtX3NjYWxlZEZvbnQuZ2V0KCkpOwpAQCAtMzA5LDUg KzMxMCw2IEBAIEhiVW5pcXVlUHRyPGhiX2ZvbnRfdD4gRm9udFBsYXRmb3JtRGF0YToKIAogICAg IHJldHVybiBIYlVuaXF1ZVB0cjxoYl9mb250X3Q+KGhiX2ZvbnRfY3JlYXRlKGZhY2UuZ2V0KCkp KTsKIH0KKyNlbmRpZgogCiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCg==