234451 2021-12-17 16:03:56 -0800 Use character names instead of hex codes in FontCascade.h 2021-12-21 01:53:40 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mmaxfield mmaxfield darin dino webkit-bug-importer oldest_to_newest 1825239 0 mmaxfield 2021-12-17 16:03:56 -0800 Use character names instead of hex codes in FontCascade.h 1825243 1 447489 mmaxfield 2021-12-17 16:07:32 -0800 Created attachment 447489 Patch 1825502 2 ews-feeder 2021-12-19 17:13:45 -0800 Committed r287250 (245408@main): <https://commits.webkit.org/245408@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447489. 1825503 3 webkit-bug-importer 2021-12-19 17:14:16 -0800 <rdar://problem/86703692> 1825504 4 447489 darin 2021-12-19 17:25:05 -0800 Comment on attachment 447489 Patch I agree that these are easier to understand when it's ==. But when using <= and >= I am not so sure. 1825661 5 mmaxfield 2021-12-20 11:31:36 -0800 I think it could go either way. I did them here for consistency, but I’m happy to put them back if people (or you) think it’s better. 1825677 6 darin 2021-12-20 12:10:39 -0800 May seem like overkill, but I would have made named functions for those sets of characters rather than using <= >= inline in that function. Might even put those named functions in CharacterNames.h. 1825929 7 447489 darin 2021-12-21 01:53:40 -0800 Comment on attachment 447489 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447489&action=review > Source/WebCore/platform/graphics/FontCascade.h:264 > + static bool treatAsZeroWidthSpaceInComplexScript(UChar32 c) { return c < space || (c >= deleteCharacter && c < noBreakSpace) || c == softHyphen || c == zeroWidthSpace || (c >= leftToRightMark && c <= rightToLeftMark) || (c >= leftToRightEmbed && c <= rightToLeftOverride) || c == zeroWidthNoBreakSpace || c == objectReplacementCharacter; } It seems like we should change these: c < space -> isC0ControlCharacter(c) (c >= leftToRightMark && c <= rightToLeftMark) || (c >= leftToRightEmbed && c <= rightToLeftOverride) -> isDirectionalFormattingCharacter(c) But to deserve the name isDirectionalFormattingCharacter, it would include 5 other characters <https://www.unicode.org/reports/tr9/#Directional_Formatting_Characters>. It seems clear that those 5 other characters also should also be treated as zero-width space. Unless maybe it’s not so important to have that be an exhaustive list. Unclear what the importance of including characters here is, and how we test it. Presumably it works around incorrectly constructed fonts where those characters aren’t zero-width? If we were not using an isDirectionalFormattingCharacter function, then we'd want to do this: (c >= leftToRightMark && c <= rightToLeftMark) -> c == leftToRightMark || c == rightToLeftMark But more importantly, what character are covered in the range leftToRightEmbed through rightToLeftOverride is not self-explanatory. 447489 2021-12-17 16:07:32 -0800 2021-12-19 17:13:46 -0800 Patch bug-234451-20211217160732.patch text/plain 2689 mmaxfield U3VidmVyc2lvbiBSZXZpc2lvbjogMjg3MjA4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggM2E3ZmJhNTc5YWM1NDVj MDk3ZWYwNWI4NzA3MDA2NThhYjg0MTZkMy4uYTdjYjY4YTNlY2Y5YWYwNTcxNTA1OWJmYTM3YTRl ODI5MzNkODRkMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDIxLTEyLTE3ICBNeWxl cyBDLiBNYXhmaWVsZCAgPG1tYXhmaWVsZEBhcHBsZS5jb20+CisKKyAgICAgICAgVXNlIGNoYXJh Y3RlciBuYW1lcyBpbnN0ZWFkIG9mIGhleCBjb2RlcyBpbiBGb250Q2FzY2FkZS5oCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMzQ0NTEKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBTYXlpbmcgc29tZXRoaW5n IGxpa2UgImMgPT0gemVyb1dpZHRoTm9uSm9pbmVyIiBpcyBtdWNoIG1vcmUgY2xlYXIgdGhhbiAi YyA9PSAweDIwMGMiLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cyBiZWNhdXNlIHRoZXJlIGlzIG5v IGJlaGF2aW9yIGNoYW5nZS4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL0ZvbnRDYXNj YWRlLmg6CisgICAgICAgIChXZWJDb3JlOjpGb250Q2FzY2FkZTo6dHJlYXRBc1NwYWNlKToKKyAg ICAgICAgKFdlYkNvcmU6OkZvbnRDYXNjYWRlOjp0cmVhdEFzWmVyb1dpZHRoU3BhY2UpOgorICAg ICAgICAoV2ViQ29yZTo6Rm9udENhc2NhZGU6OnRyZWF0QXNaZXJvV2lkdGhTcGFjZUluQ29tcGxl eFNjcmlwdCk6CisKIDIwMjEtMTItMTcgIE15bGVzIEMuIE1heGZpZWxkICA8bW1heGZpZWxkQGFw cGxlLmNvbT4KIAogICAgICAgICBSZWZhY3RvciBXaWR0aEl0ZXJhdG9yOjphcHBseUNTU1Zpc2li aWxpdHlSdWxlcygpIHRvIGJlIGEgbGl0dGxlIG1vcmUgZWxlZ2FudApkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvRm9udENhc2NhZGUuaCBiL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL0ZvbnRDYXNjYWRlLmgKaW5kZXggNmFmNTgxMmQ4YTcyMmI4 ZmRiZDM5MzgxYmQ2NDgzMjc2OWU0ZmEzYS4uYzM0NTY4MDE3ZmNkZTk4M2U2ZjVhYzcwZWUxNzgz YTlmNzNlOTA1MyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mv Rm9udENhc2NhZGUuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9Gb250 Q2FzY2FkZS5oCkBAIC0yNTksOSArMjU5LDkgQEAgcHVibGljOgogICAgIHN0YXRpYyBDb2RlUGF0 aCBzX2NvZGVQYXRoOwogCiAgICAgRm9udFNlbGVjdG9yKiBmb250U2VsZWN0b3IoKSBjb25zdDsK LSAgICBzdGF0aWMgYm9vbCB0cmVhdEFzU3BhY2UoVUNoYXIzMiBjKSB7IHJldHVybiBjID09ICcg JyB8fCBjID09ICdcdCcgfHwgYyA9PSAnXG4nIHx8IGMgPT0gbm9CcmVha1NwYWNlOyB9Ci0gICAg c3RhdGljIGJvb2wgdHJlYXRBc1plcm9XaWR0aFNwYWNlKFVDaGFyMzIgYykgeyByZXR1cm4gdHJl YXRBc1plcm9XaWR0aFNwYWNlSW5Db21wbGV4U2NyaXB0KGMpIHx8IGMgPT0gMHgyMDBjIHx8IGMg PT0gMHgyMDBkOyB9Ci0gICAgc3RhdGljIGJvb2wgdHJlYXRBc1plcm9XaWR0aFNwYWNlSW5Db21w bGV4U2NyaXB0KFVDaGFyMzIgYykgeyByZXR1cm4gYyA8IDB4MjAgfHwgKGMgPj0gMHg3RiAmJiBj IDwgMHhBMCkgfHwgYyA9PSBzb2Z0SHlwaGVuIHx8IGMgPT0gemVyb1dpZHRoU3BhY2UgfHwgKGMg Pj0gMHgyMDBlICYmIGMgPD0gMHgyMDBmKSB8fCAoYyA+PSAweDIwMmEgJiYgYyA8PSAweDIwMmUp IHx8IGMgPT0gemVyb1dpZHRoTm9CcmVha1NwYWNlIHx8IGMgPT0gb2JqZWN0UmVwbGFjZW1lbnRD aGFyYWN0ZXI7IH0KKyAgICBzdGF0aWMgYm9vbCB0cmVhdEFzU3BhY2UoVUNoYXIzMiBjKSB7IHJl dHVybiBjID09IHNwYWNlIHx8IGMgPT0gdGFiQ2hhcmFjdGVyIHx8IGMgPT0gbmV3bGluZUNoYXJh Y3RlciB8fCBjID09IG5vQnJlYWtTcGFjZTsgfQorICAgIHN0YXRpYyBib29sIHRyZWF0QXNaZXJv V2lkdGhTcGFjZShVQ2hhcjMyIGMpIHsgcmV0dXJuIHRyZWF0QXNaZXJvV2lkdGhTcGFjZUluQ29t cGxleFNjcmlwdChjKSB8fCBjID09IHplcm9XaWR0aE5vbkpvaW5lciB8fCBjID09IHplcm9XaWR0 aEpvaW5lcjsgfQorICAgIHN0YXRpYyBib29sIHRyZWF0QXNaZXJvV2lkdGhTcGFjZUluQ29tcGxl eFNjcmlwdChVQ2hhcjMyIGMpIHsgcmV0dXJuIGMgPCBzcGFjZSB8fCAoYyA+PSBkZWxldGVDaGFy YWN0ZXIgJiYgYyA8IG5vQnJlYWtTcGFjZSkgfHwgYyA9PSBzb2Z0SHlwaGVuIHx8IGMgPT0gemVy b1dpZHRoU3BhY2UgfHwgKGMgPj0gbGVmdFRvUmlnaHRNYXJrICYmIGMgPD0gcmlnaHRUb0xlZnRN YXJrKSB8fCAoYyA+PSBsZWZ0VG9SaWdodEVtYmVkICYmIGMgPD0gcmlnaHRUb0xlZnRPdmVycmlk ZSkgfHwgYyA9PSB6ZXJvV2lkdGhOb0JyZWFrU3BhY2UgfHwgYyA9PSBvYmplY3RSZXBsYWNlbWVu dENoYXJhY3RlcjsgfQogICAgIHN0YXRpYyBib29sIGNhblJlY2VpdmVUZXh0RW1waGFzaXMoVUNo YXIzMik7CiAKICAgICBzdGF0aWMgaW5saW5lIFVDaGFyIG5vcm1hbGl6ZVNwYWNlcyhVQ2hhciBj aGFyYWN0ZXIpCg==